Introduce an `HpackException` type, for Hpack exceptions about hpack HOT 6 CLOSED

I have been looking at Stack's exceptions, as part of the Haskell Foundation's Haskell Error Index initiative. That has taken me to the Pantry library, and then from Pantry to Hpack.

For GHC I see value in having identifiable and stable error messages. In the past I've been parsing GHC error messages myself and both the fact that GHC pretty prints them (leading to variations in line breaks and indentation) and that the format is not stable across GHC versions is a pain.

For Hpack I don't have the insight to judge if effort spent here will provide benefits that amortize that effort.

However, from the perspective of a user of the Hpack API I still think it's reasonable to want access to parse errors etc.

At the moment, Hpack uses String as its exception type and ends with System.Exit.die :: String -> IO a. This prints an error message to stderr and then throws an uninformative exception (ExitFailure 1). I propose to introduce an HpackException type for Hpack exceptions, which would produce the error message when the exception is thrown. That type would allow users of the Hpack library to catch the exceptions and give a different output to the one Hpack chooses to deliver, if they wished.

Exceptions are for unexpected situations. That a user has a syntax error in package.yaml is not something out of the ordinary. For that reason I don't think that exceptions are necessarily the best option here. How about adding a new primitive to the API that returns IO (Either String Result) or IO (Either SomeErrorType Result)

from hpack.

@sol, thanks. Currently, Hpack has (extracts):

type Errors = ExceptT String

readPackageConfig :: DecodeOptions -> IO (Either String DecodeResult)

and, where the action is yielded:

hpackResultWithVersion :: Version -> Options -> IO Result
hpackResultWithVersion v (Options options force generateHashStrategy toStdout) = do
  DecodeResult pkg (lines -> cabalVersion) cabalFileName warnings <- readPackageConfig options >>= either die return

That is, Hpack uses String as its exeception type and throws an exception (in hpackResultWithVersion) if there is a syntax error in package.yaml or some other 'exception'.

My pull request (#531) replaces that with:

type Errors = ExceptT HpackException

readPackageConfig :: DecodeOptions -> IO (Either HpackException DecodeResult)

and

hpackResultWithVersion :: Version -> Options -> IO Result
hpackResultWithVersion v (Options options force generateHashStrategy toStdout) = do
  DecodeResult pkg (lines -> cabalVersion) cabalFileName warnings <- readPackageConfig options >>= either throwIO return

that is, rather than hpackResultWithVersion: (1) sending an error message to stderr and throwing uninformative ExitFailure 1 (die); (2) it thows an informative exception (throwIO) - if that is not caught and handled, the exception will send exactly the same error message to stderr as in hpack-0.35.

from hpack.

As an example of what end users experience, pantry-0.8.0 handles exceptions thrown by Hpack. Currently, a user of Stack would receive output like this:

D:\Users\mike\Code\Haskell\foo\package.yaml:61:0: could not find expected ':' while scanning a simple key
Error: [S-305]
Failed to generate a Cabal file using the Hpack library on file:
D:\Users\mike\Code\Haskell\foo\package.yaml

The exception encountered was:

ExitFailure 1

The first line is what Hpack outputs into stderr. The rest is Pantry's handling of the Hpack exception ExitFailure 1.

After pull request #531, the user of Stack would receive this output:

Error: [S-305]
Failed to generate a Cabal file using the Hpack library on file:
D:\Users\mike\Code\Haskell\foo\package.yaml

The exception encountered was:

D:\Users\mike\Code\Haskell\foo\package.yaml:61:0: could not find expected ':' while scanning a simple key

Hpack's message is conveyed in the informative exception.

from hpack.

@mpilgrem I would love to discuss this at length, but the amount of time I'm able to spend on this is limited. For that reason we have to handle this efficiently.

I looked at your PR and I estimate that we will need five rounds of review to get this in shape. To avoid any further confusion I will leave a comment on your PR.

from hpack.

(2) it thows an informative exception (throwIO)

That's what I'm suggesting instead:

How about adding a new primitive to the API that returns IO (Either String Result) or IO (Either SomeErrorType Result)

from hpack.

Addressed by #535.

from hpack.