Safer dicts

[hesselink]

This uses the trick @sebastiaanvisser and I found while working on the extensible dictionaries: encoding the return type of the dictionaries with a type level Maybe, using Nothing to indicate no dictionaries. Then we can use FromMaybe to sometimes choose polymorphism and sometimes choose () for the empty list.

On top of that, I could also implement Reason_ as Reason Void instead of Reason (), which failed before because the Dicts had a () as a return type for the empty case.

This does make things quite a bit more complicated internally, but externally it makes things a bit simpler: all the some* combinators are gone. It's also more correct. What do the rest of you think about it?

I might make the list in Dicts a non-empty list as well, and see if that solves more problems.

[bergmark]

I think the extra complexity is far outweighed by the benefits. I also think using a non empty list would be nice, then we can at least include one thing to make the internals simpler along with this ;-)

Regarding no errors: "The only remaining wart is that the XmlPickler instance requires an unpickler, but there's not much to be done about that." Can you please explain this further?

Either way this is a breaking change, right? Is there any way to make it less breaking, and is it useful to do so? At the very least we should include all major changes in the changelog so I'd rather see those up front before we merge :-)

[hesselink]

Regarding no errors: "The only remaining wart is that the XmlPickler instance requires an unpickler, but there's not much to be done about that." Can you please explain this further?

The issue is this: writing pretty printers for Void is trivial, because you get passed a Void, so you can just call magic. Essentially this function can never be called, so if it is, you must get bottom, and calling magic forces the error. Going the other way and writing a parser for Void is impossible however, since you cannot construct Void without error/undefined. So ideally, I'd only define the pretty printer for xml, not the parser (like for JSON, where Void only has ToJSON, not FromJSON), but this is not possible since they're bundled in the pickler. So the only thing I can do is fail at parse time, which isn't ideal, since now it seems the parser might succeed sometimes, where it never will. This also means you can compose it to make parsers of e.g. [Void] accidentally.

Either way this is a breaking change, right? Is there any way to make it less breaking, and is it useful to do so? At the very least we should include all major changes in the changelog so I'd rather see those up front before we merge :-)

Yes, this is a breaking change. I already made an effort to cause the least amount of breakage: I deprecated functions instead of removing them, and I had to move Void but also re-exported it from the old spot. So I think most users shouldn't see any breakage, hopefully. I plan to test with our code base to see if this is actually true. And of course I'll update the CHANGELOG when merging.

[bergmark]

Sounds good!

[hesselink]

I tested this this morning. There were lots of warnings about the some* combinators being deprecated, but that is to be expected. There were a couple of places where we used Reason () explicitly instead of Reason_, but those were easily fixed. I also found three bugs, so that is a good motivation for using this feature. However, the change of the errors to use Void when there are none was a bit problematic:

• There was one spot where we used a generated client in API server code. The client still used ()s, but the API expected Void instead. Also, I couldn't regenerate the client since the generation depended on the API that didn't compile.
• The generated client code in fact isn't correct yet (uses () instead of Void). I have a work in progress commit to fix this, but that runs into the fact that we use FromJSON on the error type, and there is no instance for Void. I have to think about how we would avoid this. We can't just call error directly instead of FromJSON, since it generates a Reason Void, not a Void, and we still want to parse the standard reasons. However, I also don't want to provide the instance FromJSON Void.

I'll think about the errors stuff a bit. If we can't figure it out, we could always release only the first commit (changing the dicts to use the Maybe) and not the Void errors stuff, but since both are breaking changes, it would be nice to be able to batch release them.

[hesselink]

I've come to the conclusion that the FromJSON instance isn't that bad. It will just always fail with a parse error if you hit it, which is ok. It's needed for the derived instances like Reason a, so we should have it. I'm trying to add all class instances that Reason also has, to make sure things work out of the box. I've added Show and Generic. The one I'm not sure about is Eq. Should (==) be implemented asmagic,const magicorEQ`?

Other than that, this is good to go. It will probably break some things, but the only tricky thing was our proxying using generated clients, and I doubt anyone else is doing that. Do we have any other breaking changes scheduled? My extensible dictionaries stuff isn't nearly done, so I don't think we should wait for that.

[bergmark]

I don't know of any other breaking changes that are close to being merged.

[bergmark]

@hesselink what's the status of this?

[hesselink]

It's still unmerged, but we should probably merge it. There were some other PRs going in, probably a good idea to do all of them together.