New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Which throwM to implement MonadThrow? #31

Open
afcowie opened this Issue Aug 14, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@afcowie

afcowie commented Aug 14, 2018

Ideally, you'd be able to

 import Control.Exception.Safe

and be on your way; "drop-in replacement" or whatever. To that end, safe-exceptions re-exports MonadThrow, which is nice. It does not, however, export the throwM method of MonadThrow, instead having its own throwM function (which is just a convenience alias for throw).

The catch is that if you need to implement your own instance of MonadThrow,

instance MonadThrow Program where
      throwM = liftIO . throwM

you end up with

src/Program.hs:108:5-10: error:
    ‘throwM’ is not a (visible) method of class ‘MonadThrow’
    |
108 |     throwM = liftIO . throwM
    |     ^^^^^^

which is a tad inconvenient. I was about to send you a pull request changing the export to MonadThrow(throwM) when I realized that there is already a throwM and its not the class method. So I'm not sure what to do here. Having to do

import Control.Exception.Safe hiding (throwM)
import Control.Monad.Catch (MonadThrow(throwM))

seems likely to not be what you had in mind. How can I help put this right?

AfC

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Aug 14, 2018

Member
Member

snoyberg commented Aug 14, 2018

@afcowie

This comment has been minimized.

Show comment
Hide comment
@afcowie

afcowie Aug 14, 2018

Ah. I see the problem.

Does it have to be MonadThrow from exceptions Control.Monad.Catch? I mean, given that safe-exceptions is supposed to be a replacement / best practice / opinionated whatever can we just make a MonadThrow (and I'm assuming MonadCatch etc) that has the same shape but is homed in your library rather than re-exported from the other one?

afcowie commented Aug 14, 2018

Ah. I see the problem.

Does it have to be MonadThrow from exceptions Control.Monad.Catch? I mean, given that safe-exceptions is supposed to be a replacement / best practice / opinionated whatever can we just make a MonadThrow (and I'm assuming MonadCatch etc) that has the same shape but is homed in your library rather than re-exported from the other one?

@afcowie

This comment has been minimized.

Show comment
Hide comment
@afcowie

afcowie Aug 14, 2018

So what I did for now is:

import qualified Control.Exception.Safe as Safe (bracket, throw)
import qualified Control.Monad.Catch as Original (MonadThrow(throwM))

...

instance Original.MonadThrow Program where
    throwM = liftIO . Safe.throw

which compiles and appears to run right. I'd still vote for you implementing your own MonadThrow in safe-exceptions if that were to be an option.

afcowie commented Aug 14, 2018

So what I did for now is:

import qualified Control.Exception.Safe as Safe (bracket, throw)
import qualified Control.Monad.Catch as Original (MonadThrow(throwM))

...

instance Original.MonadThrow Program where
    throwM = liftIO . Safe.throw

which compiles and appears to run right. I'd still vote for you implementing your own MonadThrow in safe-exceptions if that were to be an option.

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Aug 14, 2018

Member

That would be a massive breaking API change and cause a lot of difficulty for users of this package hoping to use existing MonadThrow instances.

Member

snoyberg commented Aug 14, 2018

That would be a massive breaking API change and cause a lot of difficulty for users of this package hoping to use existing MonadThrow instances.

@afcowie

This comment has been minimized.

Show comment
Hide comment
@afcowie

afcowie Aug 14, 2018

Hey sure. So close this, obviously, but is what I wrote above the right way to work around this, and if so, do you want me to update the documentation?

afcowie commented Aug 14, 2018

Hey sure. So close this, obviously, but is what I wrote above the right way to work around this, and if so, do you want me to update the documentation?

@snoyberg

This comment has been minimized.

Show comment
Hide comment
@snoyberg

snoyberg Aug 14, 2018

Member

Yes, that approach should work, and a doc PR would be welcome. I'll leave this issue open for the doc aspect.

Member

snoyberg commented Aug 14, 2018

Yes, that approach should work, and a doc PR would be welcome. I'll leave this issue open for the doc aspect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment