andrewthad / impure-containers

Mutable containers in haskell
BSD 3-Clause "New" or "Revised" License
6 stars 5 forks source link

Is Maybe.Unsafe robust? #12

Closed AndrasKovacs closed 4 years ago

AndrasKovacs commented 4 years ago

A few years ago I tried to implement the same "nullable" optimized version of Maybe. I used something like the following for null pointers:

data Null = Null

However, when I tried to test this as imported from another package, I ran into mysterious crashes and errors. I assumed that for some reason this "nullable" trick does not work and didn't pursue it further. But I see it here; so my question is: is this robustly usable, and do you have any ideas why my data Null implementation could have possibly crashed?

It is also possible that I simply made some unrelated error in my own attempt, but I recall that I did not think that to be likely.

chessai commented 4 years ago

Data.Maybe.Unsafe can have problems with nested Maybes. There are perhaps some other issues. To my knowledge it is a very old iteration of https://github.com/mckeankylej/unpacked-maybe, which is safe and fast, and which i recommend over the unsafe maybe implementation here.

AndrasKovacs commented 4 years ago

@chessai the nullable Maybe can be more efficient than the unboxed sum version, especially when embedded into recursive structures. E.g. I see this usage in this package in Data.Trie.Immutable.Bits.

chessai commented 4 years ago

I know. I wasn't saying the approach in that package is more efficient than the unsafe approach, but it is safe. The safe variant should be preferred by end users. The Data.Maybe.Unsafe implementation could be used, but with plenty of caution.

chessai commented 4 years ago

It would be useful to note that it's hard to estimate how many people actually use this library in the first place. Neither @andrewthad nor i use it any longer, and almost all of the code does not reflect how we would approach writing code to the same end today. @andrewthad himself has expressed to me sentiments along the lines that he views this library as a collection of bad experiments.

All this is to say, whether or not something is used within this library is no indicator that it is a good idea.

chessai commented 4 years ago

If your definition of robust includes that it should not crash, then the approach in this library does not satisfy that definition, because it can be made to crash in relatively easy ways. The approach in unpacked-maybe does satisfy that definition of robust.

AndrasKovacs commented 4 years ago

My definition of robust in this particular case: should not crash whenever used with monomorphic non-UnsafeMaybe element type, imported from external package, only using exported "safe" API.

My old attempt did not even satisfy this condition! That's why I'm interested in the experiences here.

andrewthad commented 4 years ago

It would be really helpful to actually see the implementation of Maybe that led to crashes. I'm assuming that it has been lost with time. I was introduced to a variant this trick years ago by @ryantrinkle as a potential optimization for reflex, but I don't see it in the source code for reflex anywhere. Perhaps it is unsound.

Relatedly, I was thinking the other day that after finishing implementing https://github.com/ghc-proposals/ghc-proposals/blob/master/proposals/0203-pointer-rep.rst, I should really look at seeing if it's possible to add this maybe-of-a-lifted value trick straight to GHC. I think that with:

data RuntimpRep
  = BoxedRep Levity
  | MaybeBoxedRep Levity
  | IntRep
  | ...

data BuiltinMaybe :: forall (v :: Levity). TYPE v -> TYPE ('MaybeBoxedRep v)

This doesn't have the nesting issues because the kind system prevents nesting. But anyway, back to the original question. I would recommend not using Maybe.Unsafe and using unpacked-maybe instead. The latter is definitely safe, and it only costs an extra machine word of space in each data constructor it gets used in, and it doesn't introduce more indirections.

AndrasKovacs commented 4 years ago

@andrewthad yeah, I don't have the old code around. I guess I'll just drop this until (and if) BuiltinMaybe becomes available.