MonadThrow vs MonadError

[srid]

The problem with using MonadThrow is that the caller can theoretically expect just about any exceptions to be thrown. Whereas with MonadError e, the now-small range of errors that the caller can expect to handle is known at the type level (and, not to mention, in the function documentation).

Would you be open to a PR changing all the functions that currently use MonadThrow m to using MonadError PathError m?

[NorfairKing]

cc @chrisdone

[chrisdone]

@srid Have you tried to define and use such an instance?

[srid]

@chrisdone If I understand you correctly, you are asking how I would use the functions of the path API running in a monad with the MonadError PathError constraint?

To give an example I have code in my project that already uses MonadError AppError. From there I'd simply use withExcept to convert PathError to my custom AppError:

myFunc :: MonadError AppError m => Path a File -> m Text
myFunc somePath = do 
    ext <- withExcept AppError_PathError 
      $ fileExtension somePath
    ...
    pure undefined

But with MonadThrow I'm forced to handle the exception:

myFunc :: MonadError AppError m => Path a File -> m Text
myFunc somePath = do 
    ext <- withExcept AppError_PathErrorString
      $ liftEither
      $ first show
      $ fileExtension somePath
    ...
    pure undefined

The main problem here is that the API can throw an arbitrary non-Path exceptions. But with MonadError you can specify the exact error type the API is supposed to return.

[chrisdone]

@srid No, I'm asking whether you have tried to implement and use your proposed instance, MonadError PathError m.

[srid]

Still not 100% sure what you are asking, I'm afraid. But see further below for a sample of the change I'd make in the aforementioned PR.

Assuming PathException will eventually be be renamed to PathError, user code would invoke this function as follows:

case stripProperPrefix a b of 
  Left pathErr -> -- handle error here
  Right relPath -> -- handle the result 

Personally, if I'm consuming the PathError in a larger error data type, I'd do this:

data MyErrorType
  = MyErrorType_PathError PathException

foo :: MonadError MyErrorType m => ... -> m (...)
...
  relPath <- withExcept MyErrorType_PathError
    $ stripProperPrefix a b

In both the scenarios the Either instance of MonadError is used. Which means, the library could alternatively simply return Either PathError a instead of using a type class constraint.

---

diff --git a/src/Path/Include.hs b/src/Path/Include.hs
index 63dba5d..55a0bbd 100644
--- a/src/Path/Include.hs
+++ b/src/Path/Include.hs
@@ -22,6 +22,7 @@
 {-# LANGUAGE PatternGuards #-}
 {-# LANGUAGE DeriveDataTypeable #-}
 {-# LANGUAGE EmptyDataDecls #-}
+{-# LANGUAGE FlexibleContexts #-}
 {-# LANGUAGE FlexibleInstances #-}

 module Path.PLATFORM_NAME
@@ -89,12 +90,13 @@ module Path.PLATFORM_NAME
 import           Control.Exception (Exception(..))
 import           Control.Monad (liftM, when)
 import           Control.Monad.Catch (MonadThrow(..))
+import           Control.Monad.Except (MonadError(..))
 import           Data.Aeson (FromJSON (..), FromJSONKey(..))
 import qualified Data.Aeson.Types as Aeson
 import           Data.Data
+import           Data.Either (isRight)
 import qualified Data.Text as T
 import           Data.List
-import           Data.Maybe
 import           Language.Haskell.TH
 import           Language.Haskell.TH.Syntax (lift)
 import           Language.Haskell.TH.Quote (QuasiQuoter(..))
@@ -304,12 +306,12 @@ infixr 5 </>
 -- In other words the bases must match.
 --
 -- @since 0.6.0
-stripProperPrefix :: MonadThrow m
+stripProperPrefix :: MonadError PathException m
          => Path b Dir -> Path b t -> m (Path Rel t)
 stripProperPrefix (Path p) (Path l) =
   case stripPrefix p l of
-    Nothing -> throwM (NotAProperPrefix p l)
-    Just "" -> throwM (NotAProperPrefix p l)
+    Nothing -> throwError (NotAProperPrefix p l)
+    Just "" -> throwError (NotAProperPrefix p l)
     Just ok -> return (Path ok)

 -- | Determines if the path in the first parameter is a proper prefix of the
@@ -323,7 +325,7 @@ stripProperPrefix (Path p) (Path l) =
 --
 -- @since 0.6.0
 isProperPrefixOf :: Path b Dir -> Path b t -> Bool
-isProperPrefixOf p l = isJust (stripProperPrefix p l)
+isProperPrefixOf p l = isRight (stripProperPrefix p l)

 -- | Take the parent path component from a path.
 --
@@ -804,7 +806,7 @@ type PathParseException = PathException

 {-# DEPRECATED stripDir "Please use stripProperPrefix instead." #-}
 -- | Same as 'stripProperPrefix'.
-stripDir :: MonadThrow m
+stripDir :: MonadError PathException m
          => Path b Dir -> Path b t -> m (Path Rel t)
 stripDir = stripProperPrefix

[chrisdone]

@srid Path is used in a bunch of codebases (including stack) in a IO or Maybe or Either context, as the m in MonadThrow m is overloaded.

Having all functions return Either would be a significantly breaking change to impose on end-users, and would sacrifice the convenience mentioned in the previous paragraph.

Using MonadError would not permit the continued functioning of such end-user code, because of the functional dependency m -> e in MonadError which says that the monad determines the type of exception. There is therefore only one exception (IOException) for IO, () for Maybe, etc. meaning you could not use your proposed stripDir with Maybe or IO.

In any result, end-users' code would be broken. That's a certainty.

However, I always like a direction of more explicit types.

I wouldn't be against including duplicate functions (one defined in terms of another) such as stripProperPrefixError for a MonadError version. There aren't so many functions in the library, so this isn't so bad.

Existing users can continue without having their working code broken, and new users have the choice of either. Both forms of handling error conditions existing in the ecosystem, so it's not bad to represent both.

[srid]

Thanks; I hadn't thought of the impact of fundeps in detail here.

The original proposal was not API backwards compatible in any case. I figured that existing users could migrate their code in the following manner to get back the original behaviour (using Either e instance for MonadError):

main = do 
  relPath <- either throwM pure $ stripDir a b

As in: oldFunc = either throwM pure . newFunc

---

It seems to me that functions like stripProperPrefix were designed with the expectation that callers shouldn't generally have to handle errors (i.e., let it propagate as exceptions). In that sense, I do see why using MonadError in duplicate functions, like stripProperPrefixError (albeit, defined in terms of one another using either throwM pure), makes sense for this library.

[chrisdone]

Cool, then I'm in favor of a PR to add functions with a MonadError constraint. :ok_hand:

[sellout]

I came here to ask about this as well. @srid, have you done any work toward implementing this? If not, I might.

[sellout]

My first thought is to have Path/Include.hs define everything with MonadError, have a new module, Path.Except that looks like what Path currently looks like (just import the correct platform module), and then have Path import Path.Except, reexporting or wrapping in throwM as appropriate.

[srid]

Feel free to take this up.

[sellout]

Oh, alternatively, add parameters to Include.hs for the monad class and the throw method. Then everything is defined directly, and there's no work to keep documentation, etc. in sync.