error is used in balanceR, maybe elsewhere

[simonmichael]

Apparently balanceR functions in eg Data.Map.Internal and Data.Set.Internal can fail with a message like "Failure in Data.Map.balanceR" (related: #902). And this brought down (briefly) much of the Cardano blockchain: IntersectMBO/cardano-node#4826

Isn't it a bug to have such partial code in containers ?

[treeowl]

If our invariants are violated by someone external using internal functions incorrectly, then crashing with an error is the best outcome you can hope for. If you can reproduce the error without using any internal functions, then that's a bug in containers. Otherwise, you broke it and you get to keep both pieces.

[konsumlamm]

Using error is no bug by itself. There obviously is a bug somewhere, but it's not clear wether it is in containers or in Cardano. Cardano uses Data.Set.Internal, which allows to break the internal invariants, which can trigger the error. See IntersectMBO/cardano-node#4826 (comment) for possible places where that might happen. Without a simple reproducible example, there is not much we can do here.

[simonmichael]

Is this invariant sufficiently documented in the public API docs, eg https://hackage.haskell.org/package/containers-0.6.6/docs/Data-Map.html ?

How feasible would it be to provide a safer [default] API that can't be misused this way ?

I'm asking just because we all want to increase the ease of building reliable software with Haskell, containers is a very foundational library, and people often bemoan the inclusion of partial functions in base leading coders astray.

[treeowl]

The internals are not in the public API. The invariant is documented in the source code. If you'd like to add or improve documentation, such a contribution will always be welcome. Similarly, if anyone has the time to add Liquid Haskell annotations proving that we maintain the invariants, those would be fantastic.

[simonmichael]

(I was assuming they are not calling containers' internal APIs, and indeed I find no reference to Data.Map.Internal or Data.Set.Internal in cardano-node).

[jwaldmann]

that's the reference? https://github.com/input-output-hk/cardano-ledger/blob/master/libs/cardano-data/src/Data/MapExtras.hs#L32 ?

import Data.Map.Internal (Map (..), balanceL, balanceR, glue, link, link2)

[treeowl]

@simonmichael That's because the internals are being used in some other Cardano package, if I understand the original ticket. Doesn't cardano-node import other Cardano things?

[treeowl]

It wouldn't be bad to add Haddocks for balanceL and balanceR. They're documented in source, but it's not the most accessible documentation.

[meooow25]

My 2c is that we should not have Haddocks for such functions. It would encourage usage of the internal modules, which is not a good thing. Instead,

• If someone want a missing functionality, they should create an issue to add it to the public module.
• If the functionality is too niche to be added, they are forced to use the internal module. In that case it is best that they read the source code to understand what's going on, not just the Haddocks.