Skip to content
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

Replace MonadCatch with the one in the exceptions library? #3

Closed
RyanGlScott opened this issue Jul 23, 2015 · 1 comment
Closed

Replace MonadCatch with the one in the exceptions library? #3

RyanGlScott opened this issue Jul 23, 2015 · 1 comment

Comments

@RyanGlScott
Copy link
Member

Currently, kure defines its own MonadCatch class, which I think could be improved in a couple of ways:

  1. There should be MonadCatch instances for all the data types in transformers to make it amenable for use with monad transformer stacks.
  2. Instead of having the function argument to catchM be a String, which doesn't tell you much about the specific error is, catchM should provide an Exception as an argument. (For example, this would allow us to dispatch different types of errors more easily in hermit-shell.)

One way to fix both of these would be using the existing MonadCatch class defined in the exceptions library, which is pretty widely used. MonadCatch is defined as follows:

class MonadThrow m => MonadCatch m where
  catch :: Exception e => m a -> (e -> m a) -> m a 

exceptions specifies the following law for MonadCatch instances: catch (throwM e) f = f e, where throwM is defined as:

class Monad m => MonadThrow m where
  throwM :: Exception e => e -> m a
  -- obeys the following law
  -- throwM e >> x = throwM e

This more-or-less conforms to the semantics of the existing MonadCatch in kure, so they should be compatible. We'd have to change the semantics of several MonadCatch instances in kure in order to make this change work (e.g., we have have to show an Exception in places where kure is expecting a String), but nothing too drastic.

@Sculthorpen, what are your thoughts on this proposal?

@Sculthorpen
Copy link
Member

Yes, I agree that (2) is the right thing to do. This has been my TODO list for about 3 years, so I'm happy for you to go ahead and trial this. This will be a breaking change for KURE, so we should try and resolve any other outstanding issues regarding exceptions at the same time. A few things to consider:

(1) It should now be possible to clean up the implementation of "withPattFailMsg", which is currently a hack.
(2a) We should have a specific exception for congruence combinator failure. We can't force the user to use it, but we can advise the user to do so.
(2b) We should introduce a new operator, e.g. (<+>), intended for combining uses for congruence combinators. This should behave similarly to (<+), except for propagation of error messages. Whereas (<+) returns the right-most error message, (<+>) should return the error message that is not a congruence combinator failure (if any). See also the discussion in (ku-fpg/hermit-shell#23). This may have uses beyond congruence combinators, and we could consider generalising to any mutually exclusive operations, but we can consider that later (maybe have a class for Exceptions from mutually exclusive operations, so that we can generalise from congruence combinator failure exceptions?).
(3) The modFailMsg/prefixFailMsg/etc... functions need to be rethought. In general, do we want KURE combinators that currently take an exception String to take a String argument or an arbitrary Exception?
(4) I don't think there should be any need to "show" exceptions in the KURE library. The KURE combinators generate (and occasionally modify) exceptions but don't usually consume them.

I suggest you trial these changes, and then I'll have a look and we can discuss further. It would be good to update the examples that are packaged with KURE, and then add some more tests (that do interesting things with exceptions) to one of the examples so that we can see the new exceptions in action before trying to use it in HERMIT.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants