haskell / containers

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

should fromAscList check its precondition? #633

Open jwaldmann opened 5 years ago

jwaldmann commented 5 years ago

Currently we have

import Data.Set
member 0 $ fromAscList [2,0]    -- value is False

since https://hackage.haskell.org/package/containers-0.6.0.1/docs/Data-Set.html#v:fromAscList does not check monotonicity. Should it? Could it? (I mean, how much extra time would this need?) Was this ever discussed?

(similar question: https://github.com/twanvl/multiset/issues/9#issuecomment-490891786 )

3noch commented 5 years ago

The docs clearly say The precondition (input list is ascending) is not checked. so I think it's safe to say it was discussed. Would it be worth adding fromAscListChecked? Maybe. The point of this function is largely performance. If you don't know for certain that your input is ascending, you should use fromList.

treeowl commented 5 years ago

It should be a matter of a few minutes to strengthen the constraint from Eq to Ord (a potentially breaking change; others' code may have to be modified to adjust, but likely not a lot of code will break, and the fix is always easy) and use compare instead of Eq. Then benchmark fromListWith with several element types and list lengths and see if we can get away with it performance-wise.

adamgundry commented 8 months ago

See also #981. I think it would make sense to have a Cabal flag that turns on precondition checks for debugging purposes, without needing to modify the calling code (in case it is deep inside some dependency).