`Strict.alterFEager` too lazy when inserting new key

[sjakobi]

unordered-containers/Data/HashMap/Internal/Strict.hs

Lines 384 to 422 in 7237826

	-- | This is the default version of alterF that we use in most non-trivial
	-- cases. It's called "eager" because it looks up the given key in the map
	-- eagerly, whether or not the given function requires that information.
	alterFEager :: (Functor f, Eq k, Hashable k)
	=> (Maybe v -> f (Maybe v)) -> k -> HashMap k v -> f (HashMap k v)
	alterFEager f !k !m = (<$> f mv) $ \fres ->
	case fres of
	------------------------------
	-- Delete the key from the map.
	Nothing -> case lookupRes of
	-- Key did not exist in the map to begin with, no-op
	Absent -> m
	-- Key did exist, no collision
	Present _ collPos -> deleteKeyExists collPos h k m
	------------------------------
	-- Update value
	Just v' -> case lookupRes of
	-- Key did not exist before, insert v' under a new key
	Absent -> insertNewKey h k v' m
	-- Key existed before, no hash collision
	Present v collPos -> v' `seq`
	if v `ptrEq` v'
	-- If the value is identical, no-op
	then m
	-- If the value changed, update the value.
	else insertKeyExists collPos h k v' m
	where !h = hash k
	!lookupRes = lookupRecordCollision h k m
	!mv = case lookupRes of
	Absent -> Nothing
	Present v _ -> Just v
	{-# INLINABLE alterFEager #-}

The problem is in lines 404–407: v' is never forced.

I wonder whether confusion about the strictness of insertNewKey may have led to this bug. Although this function doesn't have a module prefix, it is imported from Data.HashMap.Internal. The code author may have thought that they were using a local, strict version of insertNewKey.

[sjakobi]

Judging by the commit message of 85e11c7, the strictness properties of this code already seemed dubious when it was introduced:

Open questions/follow-ups:

• [...]
• Verify that strictness is enforced properly in Data.HashMap.Strict.alterF.

It's pretty disappointing that no one even bothered to record this task on the issue tracker. :(