Add at' and (!~), strict variants of at and (.~) - #1119
Conversation
RyanGlScott
left a comment
There was a problem hiding this comment.
LGTM, aside from two minor suggestions.
| throws :: a -> IO Bool | ||
| throws thunk = do | ||
| r <- try (evaluate thunk >> return ()) :: IO (Either SomeException ()) |
There was a problem hiding this comment.
Slightly simpler:
| throws :: a -> IO Bool | |
| throws thunk = do | |
| r <- try (evaluate thunk >> return ()) :: IO (Either SomeException ()) | |
| throws :: forall a. a -> IO Bool | |
| throws thunk = do | |
| r <- try (evaluate thunk) :: IO (Either SomeException a) |
(Requires enabling ScopedTypeVariables)
| throws :: a -> IO Bool | ||
| throws thunk = do | ||
| r <- try (evaluate thunk >> return ()) :: IO (Either SomeException ()) |
There was a problem hiding this comment.
This catches SomeException, which means that any exception will be caught. Given that the tests below specifically aim to catch uses of undefined, I wonder if we should instead use a more specific exception type like ErrorCall. (It's unlikely to matter too much in these specific tests, but this might avoid some confusion down the road if we expand the test coverage to include other types of exceptions in the future.)
| -- >>> at' 1 ?~ "hello" $ Map.empty | ||
| -- fromList [(1,"hello")] | ||
| at' :: At m => Index m -> Lens' m (Maybe (IxValue m)) | ||
| at' i f = at i (fmap forceJust . f) |
There was a problem hiding this comment.
A problem with this is that at' behaves differently from e.g. Data.Map.Strict.insert. In particular, if the code checks for thunks with nothunks i'm quite sure that this at' will still create a (trivial but nevertheless) thunk inside the container.
I'm not sure this is "the" strict at people really want, IMO this isn't it.
There was a problem hiding this comment.
Thanks for taking a look! I tried to reproduce the thunk with nothunks and it comes back clean. at' looks identical to Data.Map.Strict.insert:
λ> import Control.Lens
λ> import NoThunks.Class
λ> import Control.Exception (evaluate)
λ> import qualified Data.Map.Strict as M
λ> let base = M.fromList [(1,0)] :: M.Map Int Int
λ> m1 <- evaluate (M.insert 1 (id (1+1)) base) -- Data.Map.Strict.insert
λ> m2 <- evaluate (base & at' 1 .~ Just (id (2+2))) -- at'
λ> m3 <- evaluate (base & at 1 .~ Just (id (3+3))) -- lazy at (control)
λ> mapM (noThunks []) [m1, m2, m3]
[Nothing,Nothing,Just (ThunkInfo {thunkContext = ["Int","Map"], thunkInfo = Nothing})]at' and Strict.insert are both Nothing. The lazy at is correctly flagged, so the check is genuinely detecting thunks. The reason at' works: at k is alterF, which has to force the result Maybe to WHNF to choose insert vs delete, and at''s Just $! v rides on that.So the value is forced exactly as it's stored (same for IntMap/HashMap).
That said, your larger point stands: at' is strict by derivation, not by contract. Generic seq over the lazy alterF rather than the container's own Data.Map.Strict.alterF. The fully transparent fix is #1093 (strict Ixed/At instances), but that changes behavior for code relying on the current laziness, so I went opt-in here. If you'd prefer strict-by-contract I can back at' with the per-container strict alterFs. Otherwise, given it's demonstrably thunk-free, I'm happy to leave the generic version. Your call.
# Conflicts: # CHANGELOG.markdown
Closes #944.
Problem
ix/atcan leave an unevaluated thunk in a strictData.Map.Strict/Data.HashMap.Strict, becauseData.Map.Strict.MapandData.Map.Lazy.Mapare the same type — theIxed/Atinstances can't tell which strictness was intended and use lazy inserts. Switching those instances to the strict modules (proposed in #1093) would change behavior for code relying on the current laziness, so this adds opt-in strict combinators instead.Approach
at'is a strictat: setting through it evaluates the new value (to weak head normal form) instead of storing an unevaluated thunk, so a strict container stays thunk-free. It works for anyAttype, affects only writes (not reads), and still obeys the lens laws.(!~)is the same idea for(.~).Two combinators are needed because a strict
(.~)alone can't fixat:at's value is wrapped in aMaybe, and evaluating theJustdoesn't touch the value inside it, soat'forces that value itself.(!~), for its part, forces its argument beforesetruns, so it forces even when the optic matches nothing.For the original
ixcase, the cleanest fix forces the value only when the key is present (likeData.Map.Strict.adjust): sinceix kisat k . traverse, writeat' k . traverse .~ v. TheData.Map.Lensdocs point to this.