Add HasCallStack to partial functions

[dwincort]

Can we add HasCallStack to the constraints of partial functions like Map.!? I'd be happy to put together a PR if it's a change that would get accepted.

[treeowl]

Yes, please! I was already thinking about doing that. But please show benchmark results to make sure the performance impact is tolerable.

…

On Jan 11, 2018 12:37 PM, "Daniel Winograd-Cort" ***@***.***> wrote: Can we add HasCallStack to the constraints of partial functions like Map.!? I'd be happy to put together a PR if it's a change that would get accepted. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#489>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzi_XyCCjFQHVVCFlk7idclZNrHCL83ks5tJkbNgaJpZM4RbMQ8> .

[sjakobi]

Related: haskell-unordered-containers/unordered-containers#252

[sjakobi]

Since the analogous work on unordered-containers was very warmly received, I'd love to see progress on this issue.

AFAICT not all changes in #493 are problematic. I think it might be a good first step to extract those into a separate PR that can be merged quickly.

(CC @bollu)

[dwincort]

Oh wow, I totally forgot about this. The project I was working on ended up finding a different way to track errors, and this went to the bottom of my priority list.

I don't have the bandwidth right now to work on this, so if someone else wants to take over, that would be great. Otherwise, I may be able to find some time in a few weeks. I'll make a reminder for myself.

[sjakobi]

For reference: Similar plans for base are discussed in https://gitlab.haskell.org/ghc/ghc/-/issues/17040.

[meooow25]

I looked into this for #1160, some thoughts below.

One argument against adding HasCallStack seen in GHC !11035, GHC !17040 and haskell/core-libraries-committee#5 is that HasCallStack is a debugging mechanism and does not belong in the type of the function. However, HasCallStack has been more widely adopted now, even in base.

The other argument is that it has a performance cost.

• There is a cost since the HasCallstack constraint simply becomes an argument of type CallStack, and the caller of the function is responsible for providing it.
• In case of an error, there is also the cost of appending to the CallStack and passing it to error, but I think we can agree that we don't care about this situation in containers.

If the function is inlined and all the usages of error are in the inlined body, the cost of passing the CallStack disappears. This has also been stated in this comment on GHC !17040. If the caller does not care about the CallStack, they don't pay the cost, though some unwanted CallStack-related code will be inlined and remain. In these cases I think we can follow base and add the HasCallStack constraint. Set.findMin is an example of this.

containers/containers/src/Data/Set/Internal.hs

Lines 781 to 784 in 5190223

	findMin :: Set a -> a
	findMin t
	| Just r <- lookupMin t = r
	| otherwise = error "Set.findMin: empty set has no minimal element"

In case the function is not inlined, it doesn't seem right to force the caller to pass in a CallStack whether they want to or not. This includes functions like Map.!.

containers/containers/src/Data/Map/Internal.hs

Lines 609 to 617 in 5190223

	find :: Ord k => k -> Map k a -> a
	find = go
	where
	go !_ Tip = error "Map.!: given key is not an element in the map"
	go k (Bin _ kx x l r) = case compare k kx of
	LT -> go k l
	GT -> go k r
	EQ -> x
	{-# INLINABLE find #-}

However, unordered-containers did not hesitate to do this for (!): haskell-unordered-containers/unordered-containers#252

@treeowl, your opinion as I understand is that the additional cost is acceptable as long as it is minimal, is that right?

[treeowl]

Yes, I tend to think it's acceptable if minimal. I think the theoretical justification is that it can help an end-user of software using the library write a useful bug report if said software has a bug.