Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flag to introduce pedantic invariant checks? #981

Open
adamgundry opened this issue Jan 11, 2024 · 2 comments
Open

Flag to introduce pedantic invariant checks? #981

adamgundry opened this issue Jan 11, 2024 · 2 comments

Comments

@adamgundry
Copy link
Member

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
Copy link

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
Copy link
Member Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants