Fix #530 Introduce HpackError type, for Hpack errors

[mpilgrem]

The Show instance for HpackException preserves the existing error messages of all existing Hpack exceptions.

Moves ProgramName from Hpack.Config to new module Hpack.ProgramName because Hpack.Exception needs to import the type and Hpack.Config now imports Hpack.Exception.

Also bumps Hpack's stack.yaml to use lts-20.0 (GHC 9.2.5) rather than lts-15.11 (GHC 8.8.3).

[mpilgrem]

I see this proposed change interferes with the test suite. I'll have a look at that.

[mpilgrem]

@sol, I have made - I hope - the changes you requested.

[sol]

The next question is, (in pantry) do you ever look at HpackException, or do you just convert it to a String? If the latter is true, then we could return Either String Result instead of Either HpackException Result, and leave the rest of Hpack unchanged.

Now, you already went through the trouble of changing the error type, and there are certainly aspects of this that I like and that I think are cleaner than what we currently have. Sticking with String would just give us the biggest bang for the buck + it does not introduce any breaking changes to the API.

If we decide to go with HpackException, then this are the things I want you to look into:

1. Do we need to retain ParseException, or can we use String instead? This will allow us to retain the Eq instance. I think the current approach is also problematic for hpack-dhall. I left some comments regarding this point (all the comments that are preceded by (1)).
2. When I originally looked at this code I had the Impression that we use HpackException too liberal instead of types that more accurately represent the domain. But looking at it again, I only see one place where we might want to change things (the comment preceded with (2)).
3. Is ProgramName used for anything else but putting it into HpackException? If not, I think we can make Hpack.Config agnostic to ProgramName by:
   1. Remove the Exception instance and rename HpackException to HpackError
   2. Have renderHpackError :: ProgramName -> HpackError -> String instead of displayException

Please also try to minimize the diff of your changes; avoid any code reformatting. This will help me a lot when reviewing your changes.

[sol]

(1) can we leave this as String? The reason that this is configurable in the first place is so that it can be used with other things than yaml.

[mpilgrem]

I'll address this in a comment below.

[sol]

(1) Can we use String instead of ParseException here?

[mpilgrem]

I'll address this in a comment below, but the data constructors of type HpackError (as it now is) now only use values of types that are instances of Eq, so that HpackError can be an instance of Eq.

[sol]

(1) Can we keep String here and wrap it in HpackParseException at the call site?

[mpilgrem]

I'll address this in a comment below.

[mpilgrem]

@sol, I have:

• I hope, taken on board your comments about using code format that minimises Git diffs.
• replaced HpackException, as an instance of Exception, with HpackError exported by module Hpack.Error; and introduced renderHpackError :: ProgramName -> HpackError -> String.
• ProgramName is only used by renderHpackError and so is removed from the functions in Hpack.Config where it is not needed, including the elimination of the related data constructor of DecodeOptions and the function setProgramName.
• ProgramName is now also exported by Hpack.Error. I have also introduced (and applied) the constant hpackProgName :: ProgramName (ProgramName "hpack"), exported by Hpack.Error, for convenience.
• HpackError is an instance of Eq. So, I have replaced the value of type ParseException with values of types that are instances of Eq.
• The existing Hpack only handles expressly three of the eleven data constructors of ParseException. The rest are bundled into a show err output. Part of my original motivation was not to lose the information potentially contained in a value of type ParseException but, for now, I have retained Hpack's original approach. The mapping between a value of type ParseException and a value of type HpackError is done in Hpack.Yaml.toHpackError. I named the four relevant data constructors of HpackError to reflect the names of the data constructors of ParseException (eg HpackParseYamlParseException for InvalidYaml (Just (YamlParseException{..}))). However, perhaps it would be better for HpackError to have data constructors corresponding to each of the data constructors of ParseException and use HpackParseOtherException only to correspond with Data.Yaml.OtherParseException SomeException; it would make HpackError more 'future proof' - and if the other data constructors of ParseException are never (or only rarely) encountered, no harm is done.

I have retained HpackError as my suggestion for Hpack's error type. If String were retained as Hpack's error type, it would cut across my primary motivation for this pull request, which was to allow users of the Hpack library (I had in mind Stack, via Pantry) to be more expressive/less terse in its reporting of Hpack's errors than Hpack itself currently chooses to be. I appreciate that is a breaking change to Hpack's API in respect of the type of decodeOptionsDecode :: Filepath -> IO (Either HpackError ([String], Value)).

I have looked at the package hpack-dhall and see the issue with Hpack.Dhall.fileToJson:

-- | A file decoder for hpack. This should evaluate to a single record with
-- hpack's top-level <https://github.com/sol/hpack#top-level-fields fields>.
fileToJson
    :: FilePath -- ^ Path to a @.dhall@ file
    -> IO (Either String ([String], Value))
fileToJson file =
    liftIO (T.readFile file)
    >>= textToJson (inputSettings file)

If this pull request were accepted, I would raise a corresponding pull request to adapt hpack-dhall. I have included the data constructor HpackOtherException for HpackError to allow functions such as fileToJson to continue to use String as an error/exception type. So, I think fileToJson would become something like (not tested):

-- | A file decoder for hpack. This should evaluate to a single record with
-- hpack's top-level <https://github.com/sol/hpack#top-level-fields fields>.
fileToJson
    :: FilePath -- ^ Path to a @.dhall@ file
    -> IO (Either HpackError ([String], Value))
fileToJson file =
    liftIO (T.readFile file)
    >>= textToJson (inputSettings file) >>= either (Left . HpackOtherException file) id

[sol]

@mpilgrem sorry for the delay on this. I hope I'll be able to get back to you soon.

[sol]

@mpilgrem again, sorry for the delay on this.

1. The scenario you describe in Introduce an HpackException type, for Hpack exceptions #530 (comment) I think can be addressed by hpackResultWithError :: Options -> IO (Either String Result). That's an easy change and that is what we will do by default.

2. As for HpackError, if you want to allocate individual error codes for different Hpack failure scenarios in stack/pantry then I am happy to accept the required changes to Hpack. However, in that case I want to make sure that we end up with something that is adequate for stack/pantry. For that reason I would want to see a corresponding PR to pantry before discussing this further.

3. After giving this some more thought, I'm inclined to avoid breakage as much as possible. I'm confident that we can provide what stack/pantry needs, but we will try to do so in a backwards compatible way.

4. If we end up going with (1) and not expose HpackError then I may still want to cherry pick some of your internal changes, but let's disregard this for now; let's not fall for the sunk-cost fallacy.

@mpilgrem if you're inclined to do (2), can you open a corresponding PR for pantry? This will give me more context. For now, focus on what exactly stack/pantry need; don't burn cycles on backwards compat etc.

[sol]

if the other data constructors of ParseException are never (or only rarely) encountered, no harm is done.

I hinted at this before, from my perspective you want to avoid using types that do not accurately represent the domain.

You wouldn't use Maybe As the return type of a function in situations where you never return Nothing, where you only ever have Just-values.

You wouldn't use Either where you only ever have Right-values.

In the same way you wouldn't want to add constructors to HpackError that you can't ever encounter.

[mpilgrem]

@sol, at the moment, the PR in Pantry was this one (which is reflected in pantry-0.8.0): commercialhaskell/pantry#72. There is not much for Pantry to actually 'handle' from hpack-0.35.0.

To take a step back and explain my original motivation for this: if somebody (say) accidently adds the line duff to the end of a stack.yaml file, the next version of Stack will report this (if colour is disabled):

Error: [S-6602]
       Stack could not load and parse C:\Users\mikep\Documents\Code\Haskell\boo\stack.yaml as a YAML configuraton file.

       While loading and parsing, Stack encountered the following exception:

       YAML parse exception at line 68, column 0,
       while scanning a simple key:
       could not find expected ':'

       For help about the content of Stack's YAML configuration files, see (for the most recent release of Stack)
       http://docs.haskellstack.org/en/stable/yaml_configuration/.

The format of the 'following exception ...' above is that chosen by the yaml package. It has 'pretty' output for its displayException of its ParseException values.

(As an aside Error [S-6602] has been 'written up' at https://errors.haskell.org/messages/S-6602/index.html, which is why it has my attention.)

Currently, if somebody accidently adds the line duff to the end of a package.yaml file, the next version of Stack's output is this:

C:\Users\mikep\Documents\Code\Haskell\boo\package.yaml:60: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:
C:\Users\mikep\Documents\Code\Haskell\boo\package.yaml

The exception encountered was:

ExitFailure 1

The first line of the output is Hpack's output. The rest of the lines are Pantry 'handing' the Hpack exception as Pantry Error [S-305]. Stack does not currently 'handle' [S-305], but it could.

My end objective was for a user of Stack to have a similar experience when something was wrong with package.yaml as when something was wrong with stack.yaml. My reasoning was that if Hpack provided 'informative' exceptions, then Pantry would handle them (as Error [S-305]) and Stack, if it wished, could handle the Pantry exception. Hpack chooses to be terse (terser than the yaml package) (which is fine), Pantry might choose to be less terse, and Stack (which aims to help newcomers to Haskell) might chose to provide more help than Pantry - for example, it might direct the user to the guide to package.yaml at the Hpack repository.

[sol]

OK, I have the feeling we are finally getting to the bottom of this. From what I understand you want to use displayException on YamlException so that the output is consistent with what stack produces for syntax errors in stack.yaml. Is that the whole story?

The reason why hpack uses file:line:column: problem is that it's a common, machine readable format that is understood by third-party tools (e.g. vim understand that format natively and can use it to help you set the cursor to where parsing failed).

Pantry might choose to be less terse, and Stack (which aims to help newcomers to Haskell) might chose to provide more help than Pantry - for example, it might direct the user to the guide

Are there any notable users of pantry other than stack?

Instead of looking at what somebody might need, let's approach this lean. Let's identify what you want to change in stack right now. Then let's:

1. Implement those changes in the fastest way possible
2. Only as a second step, once we have pinned down what we actually need, think about future proofing, backwards compat, general bike shedding.

@mpilgrem technically, it's possible to customize the error formatting by using Hpack.setDecode. It's not perfect, as it does not apply recursively to defaults, but I think this is something we can look into later. Can you open a pantry/stack PR using #535 and the following patch to pantry as a starting point?

diff --git a/src/Pantry.hs b/src/Pantry.hs
index 9218969..432a624 100644
--- a/src/Pantry.hs
+++ b/src/Pantry.hs
@@ -2,6 +2,7 @@
 {-# LANGUAGE TemplateHaskell #-}
 {-# LANGUAGE OverloadedStrings #-}
 {-# LANGUAGE ScopedTypeVariables #-}
+{-# LANGUAGE LambdaCase #-}
 -- | Content addressable Haskell package management, providing for
 -- secure, reproducible acquisition of Haskell package contents and
 -- metadata.
@@ -227,6 +228,16 @@ import Pantry.HTTP
 import Data.Char (isHexDigit)
 import Data.Time (getCurrentTime, diffUTCTime)
 
+import Data.Yaml.Include (decodeFileWithWarnings)
+import Hpack.Yaml (formatWarning)
+import Hpack.Error (renderHpackError)
+
+decodeYaml :: FilePath -> IO (Either String ([String], Value))
+decodeYaml file = do
+  bimap displayException (first formatWarnings) <$> decodeFileWithWarnings file
+  where
+    formatWarnings = map (formatWarning file)
+
 -- | Create a new 'PantryConfig' with the given settings.
 --
 -- For something easier to use in simple cases, see 'runPantryApp'.
@@ -741,15 +752,16 @@ hpack progName pkgDir = do
 
         he <- view $ pantryConfigL.to pcHpackExecutable
         case he of
-            HpackBundled -> do
-                r <- catchAny
+            HpackBundled ->
                        ( liftIO
-                           $ Hpack.hpackResult
+                           $ Hpack.hpackResultWithError
                            $ mHpackProgName
+                           $ Hpack.setDecode decodeYaml
                            $ Hpack.setTarget
                                (toFilePath hpackFile) Hpack.defaultOptions
-                       )
-                       ( throwIO . HpackLibraryException hpackFile )
+                       ) >>= \ case
+              Left err -> throwIO (HpackLibraryException hpackFile $ renderHpackError (fromMaybe "hpack" progName) err)
+              Right r -> do
                 forM_ (Hpack.resultWarnings r) (logWarn . fromString)
                 let cabalFile = fromString . Hpack.resultCabalFile $ r
                 case Hpack.resultStatus r of
diff --git a/src/Pantry/Types.hs b/src/Pantry/Types.hs
index 45fd914..9a904d2 100644
--- a/src/Pantry/Types.hs
+++ b/src/Pantry/Types.hs
@@ -970,7 +970,7 @@ data PantryException
   | MigrationFailure !Text !(Path Abs File) !SomeException
   | InvalidTreeFromCasa !BlobKey !ByteString
   | ParseSnapNameException !Text
-  | HpackLibraryException !(Path Abs File) !SomeException
+  | HpackLibraryException !(Path Abs File) !String
   | HpackExeException !FilePath !(Path Abs Dir) !SomeException
 
   deriving Typeable
@@ -1279,13 +1279,13 @@ instance Display PantryException where
     "Error: [S-994]\n"
      <> "Invalid snapshot name: "
      <> display t
-  display (HpackLibraryException file e) =
+  display (HpackLibraryException file err) =
     "Error: [S-305]\n"
     <> "Failed to generate a Cabal file using the Hpack library on file:\n"
     <> fromString (toFilePath file)
     <> "\n\n"
-    <> "The exception encountered was:\n\n"
-    <> fromString (show e)
+    <> "The error encountered was:\n\n"
+    <> fromString err
   display (HpackExeException fp dir e) =
     "Error: [S-720]\n"
     <> "Failed to generate a Cabal file using the Hpack executable:\n"
diff --git a/stack.yaml b/stack.yaml
index 8094e22..0be8567 100644
--- a/stack.yaml
+++ b/stack.yaml
@@ -2,7 +2,8 @@
 resolver: lts-18.28
 
 extra-deps:
-- hpack-0.35.0
+- git: https://github.com/sol/hpack
+  commit: 1ecc4158bdfc03609c88d0b1908fa9f41fdfbf5c
 
 ghc-options:
    "$locals": -fhide-source-paths

If this doesn't work out and you still need access to the ParseException for some reason, then please add it again to Hpack.Error.ParseYamlError (removing the Eq instance and ignoring any broken tests for now).

[sol]

@mpilgrem at the risk of stating the obvious (as you are on Window, I don't know how solid your UNIX foundations are), you can apply this patch by copy & pasting it into a file hpack.diff and then running patch -p1 < hpack.diff (inside your pantry directory and from a WSL shell, or maybe git bash or something).

[mpilgrem]

@sol, thanks. Due to other commitments, I can't turn to this immediately but I will look at it soon. Thanks also for the advice about the patch tool. I had not come across it before, but MSYS2 provides a version for Windows.

[mpilgrem]

@sol, after a couple of false starts on my part:

• the Pantry draft pull request is: Fix Hpack #530 Make use of prettier Hpack error messages commercialhaskell/pantry#74
• the Stack draft pull request is: Fix Hpack #530 commercialhaskell/stack#5966

The output from Pantry (and, consequently, Stack) is as I had originally hoped:

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

The error encountered was:

YAML parse exception at line 61, column 0,
while scanning a simple key:
could not find expected ':'

To answer your questions:

• 'Is that the whole story?` That was the only 'empirical' motivation; anything else was 'theoretical'.
• Notable users of pantry other than Stack? The only other users of pantry of which I am aware are mega-sdist (from reverse dependencies on Hackage) and curator (from past discussions about breaking changes to the Pantry API on Slack).

May I ask a question? Apologies if this is 'bike shedding'. My understanding of the use of 'error' v 'exception' terminology is informed principally by this Haskell Wiki page by Henning Thielemann: https://wiki.haskell.org/Error_vs._Exception, which ends with an acknowledgement that different people may use the terms differently, or not distinguish between them. I noted that you preferred (in the Pantry message) 'error' over 'exception' (which I had picked for consistency with other messages). My question is: what is your reasoning for the choice of terminology?

[sol]

@mpilgrem from my side #535 is good to go. Please take a final look and update your pantry / stack PRs.

Regarding your question (and exceptions in general) there are several aspects, or let's say chapters, to this. Instead of writing a whole book, i'll write one chapter at a time (as time permits). That means each of my subsequent comments will deal with one specific aspect. If you want more details to a specific aspect then please quote-reply the comment in question. I am not sure into how much details I want to go, so ultimately we may still only end up with one or two chapters.

[mpilgrem]

For some reason, the CI for Pantry is failing on macOS (only), and it looks to be Hpack-related (although not necessarily Hpack-caused). While building the Hpack dependency, the CI reports:

hpack                > [30 of 30] Compiling Hpack
hpack                > ld: warning: -undefined dynamic_lookup may not work with chained fixups
hpack                > Preprocessing executable 'hpack' for hpack-0.35.0..
hpack                > Building executable 'hpack' for hpack-0.35.0..
hpack                > 
hpack                > /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/stack-e91529d18c5b56fc/hpack-0.35.0/<built-in>:16:10: error:
hpack                >      warning: non-portable path to file '".stack-work/dist/x86_64-osx/Cabal-3.2.1.0/build/Hpack/autogen/cabal_macros.h"'; specified path differs in case from file name on disk [-Wnonportable-include-path]
hpack                > #include ".stack-work/dist/x86_64-osx/Cabal-3.2.1.0/build/hpack/autogen/cabal_macros.h"
hpack                >          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hpack                >          ".stack-work/dist/x86_64-osx/Cabal-3.2.1.0/build/Hpack/autogen/cabal_macros.h"
hpack                > 1 warning generated.
hpack                > [1 of 2] Compiling Main
hpack                > [2 of 2] Compiling Paths_hpack
hpack                > 
hpack                > /private/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/stack-e91529d18c5b56fc/hpack-0.35.0/<built-in>:16:10: error:
hpack                >      warning: non-portable path to file '".stack-work/dist/x86_64-osx/Cabal-3.2.1.0/build/Hpack/autogen/cabal_macros.h"'; specified path differs in case from file name on disk [-Wnonportable-include-path]
hpack                > #include ".stack-work/dist/x86_64-osx/Cabal-3.2.1.0/build/hpack/autogen/cabal_macros.h"
hpack                >          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hpack                >          ".stack-work/dist/x86_64-osx/Cabal-3.2.1.0/build/Hpack/autogen/cabal_macros.h"
hpack                > 1 warning generated.
hpack                > Linking .stack-work/dist/x86_64-osx/Cabal-3.2.1.0/build/hpack/hpack ...

and, later, the build of Pantry.TypesSpec fails with:

[11 of 13] Compiling Pantry.TypesSpec
<command line>: dlopen(/Users/runner/work/pantry/pantry/.stack-work/dist/x86_64-osx/Cabal-3.2.1.0/build/libHSpantry-0.8.0-FHYkC4EdfwaBpLVdC5liCG-ghc8.10.7.dylib, 0x0005): symbol not found in flat namespace (_hpackzm0zi35zi0zmDuafflpufj8Acl0bWndEqa_Hpack_zdwhpackResultWithError_info)

--  While building package pantry-0.8.0 (scroll up to its section to see the error) using:
      /Users/runner/.stack/setup-exe-cache/x86_64-osx/Cabal-simple_mPHDZzAJ_3.2.1.0_ghc-8.10.7 --verbose=1 --builddir=.stack-work/dist/x86_64-osx/Cabal-3.2.1.0 build lib:pantry test:spec --ghc-options ""
    Process exited with code: ExitFailure 1
Error: Process completed with exit code 1.

[sol]

@mpilgrem I think most likely this is related to caching. I'll reply to your PR.

[sol]

May I ask a question? Apologies if this is 'bike shedding'. My understanding of the use of 'error' v 'exception' terminology is informed principally by this Haskell Wiki page by Henning Thielemann: https://wiki.haskell.org/Error_vs._Exception, which ends with an acknowledgement that different people may use the terms differently, or not distinguish between them. I noted that you preferred (in the Pantry message) 'error' over 'exception' (which I had picked for consistency with other messages). My question is: what is your reasoning for the choice of terminology?

I'm not inclined to make a verdict on whether this wiki article from 2009 is authoritative, or not.

But I think when we talk about error messages that are presented to a user of a programming tool (say a compiler, or a build tool like stack) then that's also not necessary.

If I, as a user of a compiler, feed a program that contains a syntax error into that compiler then I think it's natural that the compiler tells me "you have a syntax error here" as opposed to "syntax exception".

In the same way, I think it's natural that a build tool (say stack) that is confronted with a syntax error in a YAML file calls that thing "syntax error", not "syntax exception".

I'm talking here about error messages that you show to users. How you name things internally and how you structure your program (read: build tool or compiler) is a different story.

Now, coincidentally, this also fits somewhat into the narrative of that wiki article. A syntax error is an error from which a compiler cannot recover (at least not in the most common sense). Still, in the hypothetical case we had a compiler that uses an error correcting parser and hence could recover from syntax errors, then we would probably still call it "syntax error", not "syntax exception".

As for stack / pantry, my patch was really just meant as a starting point. You should do what you think makes sense and what is consistent with the rest of the code base.

[mpilgrem]

@sol, looks good to me. Stack will now report (via Pantry) things like:

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

The error encountered was:

In respect of an Hpack defaults file:
D:\Users\mike\Code\Haskell\boo\defaults.yaml:

YAML parse exception at line 3, column 0,
while scanning a simple key:
could not find expected ':'