haskell / containers

Assorted concrete container types
https://hackage.haskell.org/package/containers
315 stars 178 forks source link

Flag to introduce pedantic invariant checks? #981

Open adamgundry opened 8 months ago

adamgundry commented 8 months ago

I was speaking to a client with a large codebase which is somehow breaking the internal invariants of a Map, and wondering how best to track down the offending code. My first guess is that a function with a precondition such as fromDistinctAscList is being misused, or perhaps the internals API is used incorrectly, but it's difficult to figure out where when there are many transitive dependencies in a large codebase.

Would it make sense to have a manual Cabal flag for containers that turns on a "pedantic" mode via CPP, wherein functions like fromDistinctAscList perform a runtime check of their precondition and error out with a callstack if it is not satisfied? Even the internal API could potentially perform invariant checks, to help with situations like #913 where the user has shot themselves in the foot and needs to figure out where they fired from.

Obviously enabling this flag would hurt performance, but it could be very helpful for diagnosis. Potentially it could guard new HasCallStack constraints as well (cf. #489), to avoid any performance worries introducing them by default.

jberryman commented 8 months ago

one wrinkle I found is the Data.Map.Internal.Debug.ordered requires the Ord constraint added to functions which don't have them, and this also breaks e.g. binary https://github.com/kolmodin/binary/blob/master/src/Data/Binary/Class.hs#L658-L660

adamgundry commented 8 months ago

@jberryman good point. For the public API alone, I suspect we could get away with introducing a wrapper datatype that captures the Ord dictionary in the data constructor. But it wouldn't work for the internal API, where you can't get away from the fact that the constructors of Map are exposed and don't have Ord dictionaries available. Maybe with some pattern synonyms one could get away with changing the internal API just a little, but it does seem tricky.