Fix general bracket for ExceptT/ErrorT

[snoyberg]

@int-index pointed out that my previous attempt at generalBracket was, in fact, completely broken for ExceptT. My apologies there, I know the previous PR was already complicated enough. I've gotten a proper test case added now that demonstrates that breakage, and have included a second commit that fixes the type signature for generalBracket.

Note the comments I've added to that commit, in particular that this introduces state/writer discarding. This seems inherent in keeping compatibility with ExceptT, unless someone else can find a better way.

It looks like the previous version never made it to Hackage, which is a Good Thing :)

CC @edsko, who I'm guessing will have some thoughts.

[edsko]

I think it's pity the type is less expressive now, but if the old type cannot be given useful instances then we're not left with much choice. Unfortunately I am currently travelling abroad and can't look into this PR in more detail right now, apologies!

[int-index]

Can the maintainers review/merge this PR or propose an alternative solution? The added test case (https://github.com/ekmett/exceptions/pull/60/commits/1d67d034af8ba9ca81d29a329cefb8bd4efb0d8b) is important.

[snoyberg]

Thanks for the feedback @RyanGlScott, I've added another commit which I believe addresses those comments.

[RyanGlScott]

Thanks!

[RyanGlScott]

I have a question for @snoyberg (or anyone else who is interested):

As I'm pondering a new exceptions release with this feature, I'm still wondering if we could do more to help acclimate folks to the introduction of generalBracket. As noted in https://github.com/ekmett/exceptions/pull/59#discussion_r120475433, there's no default implementation we can give for generalBracket that will work for everything. But perhaps we could at the very least have a migration guide in the changelog which describes a template for generalBracket? Something akin to "here's how you'll likely want to define generalBracket, but watch out for gotchas", such as the instance for ExceptT.

[snoyberg]

I'm not opposed to docs, however: there's no default implementation provided for any of the other class methods in this module either, nor is there any documentation on recommended implementations for any of them. throwM could in theory be given a default implementation in terms of MonadTrans: throwM = lift . throwM. But the same does not hold of catch, mask, uninterruptibleMask, or generalBracket. We can document how some specific instances can be written (such as ReaderT and ExceptT), but this seems to be veering close to a request like "document how to write an instance of MonadTrans for arbitrary transformers."

Perhaps my stance on this whole topic should be clarified: I think the previous definition of MonadMask was dangerous, in that it allowed instances to be written that provided a working mask and uninterruptibleMask, but resulted in completely broken functions like finally. But the overall concept here is a dangerous one, with many opportunities for incorrect instances to slip through (evidence: I already did that myself in these PRs!). If we were to include any kind of documentation on how to write these instances, it might head towards "be very, very careful, write tests, and ask someone to look it over for you."

[RyanGlScott]

Sure, I don't want to suggest that we write up a comprehensive guide on how to write every method of MonadMask. My thinking was more along the lines of: we're introducing a new method to a widely used class that many folks are going to need to implement somehow to avoid -Wall warnings, so perhaps we could save them from some potential frustration by pointing out common pitfalls.

[snoyberg]

I'm not sure if this is what you had in mind, but I've added some docs in #62.

[RyanGlScott]

Excellent! Yes, this is exactly the sort of thing I was envisioning.