Better exception throwing machinery
- Dominant language
- Haskell
- Stars
- 1.7k
- Forks
- 750
- Avg merge
- 4d 3h
- Merged PRs (30d)
- 28
Description
# Context
When a `CabalException` or [`CabalInstallException`](https://github.com/haskell/cabal/blob/1082c0bb4e498fc9ea3b7b8cbfa78a99f2edb3b9/cabal-install/src/Distribution/Client/Errors.hs#L38) is thrown via [`dieWithException`](https://github.com/haskell/cabal/blob/1082c0bb4e498fc9ea3b7b8cbfa78a99f2edb3b9/Cabal/src/Distribution/Simple/Utils.hs#L425-L447), we get a few benefits:
1. An error message that can (theoretically) be looked up in a manual is printed.
2. If some special options are run, output markers that enable only "stable" output to show up for `cabal-testsuite`.
```
$ cabal "-vnormal+markoutput" build
-----BEGIN CABAL OUTPUT-----
Error: [Cabal-7134]
-----END CABAL OUTPUT-----
-----BEGIN CABAL OUTPUT-----
No targets given and there is no package in the current directory. Use the target 'all' for all packages in the project or specify packages or components by name or location. See 'cabal build --help' for more details on target options.
-----END CABAL OUTPUT-----
```
As a result `CabalInstallException` is very important — it’s used by lots of modules — and as a result it’s very hard to include structured data in it, because importing the types you need to declare a variant with structured data nearly always results in import cycles.
So if you’re using a rich exception type like [`BadPackageLocations`](https://github.com/haskell/cabal/blob/1082c0bb4e498fc9ea3b7b8cbfa78a99f2edb3b9/cabal-install/src/Distribution/Client/ProjectConfig.hs#L1053-L1056), you don’t get error codes and you don’t get output markers, so the integration tests are very painful, like this:
https://github.com/haskell/cabal/blob/1082c0bb4e498fc9ea3b7b8cbfa78a99f2edb3b9/cabal-testsuite/PackageTests/ProjectImport/DedupUsingConfigFromComplex/cabal.test.hs
# Considerations for exception machinery
## Centralization of error codes
We'd like all of the error codes to be defined and described in one place. (Or as few places as possible — currently they're split between `CabalException` and `CabalInstallException`.)
Rust solves this by writing a (detailed) description for each error code in one directory:
https://github.com/rust-lang/rust/blob/6c8347b9588a302afebb81a1ae6daa64ec37abd0/compiler/rustc_error_codes/src/error_codes/E0023.md
See also: ["Errors and Lints" in the Rust Compiler Development Guide](https://rustc-dev-guide.rust-lang.org/diagnostics.html)
## Structured errors
We'd like to be able to throw and catch errors that contain richly-structured and typed data. `CabalException` and `CabalInstallException` largely just contain opaque `String` message fragments.
**Richly-structured error types are critical** — when we don't have these, the Cabal UX suffers (errors don't include error codes or other niceties) and the experience of Cabal maintainers suffers (integration tests are very difficult to write and difficult to maintain).
See the `BadPackageLocations` example above to see how this plays out in practice. [Here's a commit](https://github.com/haskell/cabal/commit/d2dd158ba833f5c3f633d7c79572dbcfbede1447) showing how the experience improves if structured errors are able to hook into the `VerboseException` machinery. Additionally, it becomes possible for `cabal-testsuite` to automatically update the `cabal.out` file to reflect changes in the implementation, rather than relying on a programmer reading a (very noisy and long) readout of the test's output, locating the differences in the regular expression, and updating them to match. (The API used in that commit is not viable because it breaks the ability to _catch_ these exceptions, but I believe the proposal outlined below does not suffer from this deficiency.)
# Proposal
First, we add a class `IsCabalException` representing an exception like `CabalException` or `CabalInstallException` which can be pretty-printed and has an error code:
```haskell
class
( Show e
, Typeable e
, Exception e
, Pretty e
) => IsCabalException e where
-- | Get this error's unique error code.
getErrorCode :: e -> Int
```
Then, we add a type-erased `SomeCabalException` type (comparable to [`SomeException`](https://hackage.haskell.org/package/base-4.21.0.0/docs/Control-Exception.html#t:SomeException)) which replaces `VerboseException`:
```haskell
data SomeCabalException where
SomeCabalException :: (IsCabalException e) => CallStack -> Verbosity -> e -> SomeCabalException
instance Exception SomeCabalException where
-- ...
instance Exception CabalException where
toException e = toException (SomeCabalException ... e)
fromException someExn@(SomeException inner) = cast inner <|> do
SomeCabalException inner' <- fromException someExn
cast inner'
```
@parsonsmatt sketched out with the design for this system (inspired by [`annotated-exception`](https://github.com/parsonsmatt/annotated-exception#readme)) after [my first attempt](https://github.com/9999years/cabal/commit/5f4750a15abd6d47bf5a9414df35af8f76c7f7fe) lost the ability to _catch_ structured exceptions.
Using `annotated-exception` at work, I can confirm it works pretty nicely, but a notable pitfall is it becomes very easy to lose the annotations (in this case, the verbosity and callstack information) — because you can catch exceptions as the inner type, it's easy to write a `catch` clause that erases some of the exception's data.
Contributor guide
Research direction
Start by reading dieWithException in Cabal/src/Distribution/Simple/Utils.hs, the CabalInstallException definition in cabal-install/src/Distribution/Client/Errors.hs, and the BadPackageLocations case in cabal-install/src/Distribution/Client/ProjectConfig.hs. Compare the linked ProjectImport integration test and the proposed SomeCabalException design. Done means a reviewed exception mechanism that centralizes error codes, preserves structured catching, and provides verbosity and output markers.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- haskell
- Domain
- build-system
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100