Skip to content

Commit efa43a2

Browse files
committed
Restrict alterF constant rule
The `alterF` rule for recognizing that the given function ignores its argument can lead to performance problems for certain functors. Restrict the rule to `Identity`, where it's always a good idea. Fixes #189
1 parent 969fa3f commit efa43a2

File tree

2 files changed

+12
-5
lines changed

2 files changed

+12
-5
lines changed

Data/HashMap/Base.hs

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1178,16 +1178,23 @@ bogus# _ = error "Data.HashMap.alterF internal error: hit bogus#"
11781178
alterFWeird (f Nothing) (f (Just test_bottom)) f
11791179

11801180
-- This rule covers situations where alterF is used to simply insert or
1181-
-- delete in some functor (most likely via Control.Lens.At). We recognize here
1181+
-- delete in Identity (most likely via Control.Lens.At). We recognize here
11821182
-- (through the repeated @x@ on the LHS) that
11831183
--
11841184
-- @f Nothing = f (Just bottom)@,
11851185
--
11861186
-- which guarantees that @f@ doesn't care what its argument is, so
11871187
-- we don't have to either.
1188-
"alterFconstant" forall (f :: Maybe a -> f (Maybe a)) x.
1188+
--
1189+
-- Why only Identity? A variant of this rule is actually valid regardless of
1190+
-- the functor, but for some functors (e.g., []), it can lead to the
1191+
-- same keys being compared multiple times, which is bad if they're
1192+
-- ugly things like strings. This is unfortunate, since the rule is likely
1193+
-- a good idea for almost all realistic uses, but I don't like nasty
1194+
-- edge cases.
1195+
"alterFconstant" forall (f :: Maybe a -> Identity (Maybe a)) x.
11891196
alterFWeird x x f = \ !k !m ->
1190-
fmap (\case {Nothing -> delete k m; Just a -> insert k a m}) x
1197+
Identity (case runIdentity x of {Nothing -> delete k m; Just a -> insert k a m})
11911198

11921199
-- This rule handles the case where 'alterF' is used to do 'insertWith'-like
11931200
-- things. Whenever possible, GHC will get rid of the Maybe nonsense for us.

Data/HashMap/Strict/Base.hs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -307,9 +307,9 @@ impossibleAdjust = error "Data.HashMap.alterF internal error: impossible adjust"
307307
"alterFWeird" forall f. alterF f =
308308
alterFWeird (f Nothing) (f (Just test_bottom)) f
309309

310-
"alterFconstant" forall (f :: Maybe a -> f (Maybe a)) x.
310+
"alterFconstant" forall (f :: Maybe a -> Identity (Maybe a)) x.
311311
alterFWeird x x f = \ !k !m ->
312-
fmap (\case {Nothing -> delete k m; Just a -> insert k a m}) x
312+
Identity (case runIdentity x of {Nothing -> delete k m; Just a -> insert k a m})
313313

314314
"alterFinsertWith" [1] forall (f :: Maybe a -> Identity (Maybe a)) x y.
315315
alterFWeird (coerce (Just x)) (coerce (Just y)) f =

0 commit comments

Comments
 (0)