Open AngusGMorrison opened 5 months ago
I agree with the general strategy and the recommendations. I would only add that application developers might want to use a crate like anyhow or color-eyre to display errors to users.
Could you add this as a short dev doc to the repo (e.g. docs/error_handling.md
)?
Note, DafSpkError doesn't actually implement Error.
Ooops. Thanks for spotting this. I wonder if there's a linter check that we could enable.
LGTM.
Lox currently takes several approaches to error handling, which is confusing for authors and users. This proposal aims to establish a consistent approach to error handling throughout the library.
Current situtation
Lox currently employs three different error handling strategies:
LoxTimeError
,LoxPyError
andLoxBodiesError
.LoxCubicSplineError
.UTCUndefinedError
.Deciding which to implement for a new error scenario leaves too much room for personal preference, and results in an inconsistent experience for library consumers.
List of Lox errors
Since it's useful to understand the state of play, and because I'll be referring back to it later, here are the definitions of all errors currently used in the Lox workspace:
DafSpkError
doesn't actually implementError
.The trouble with crate-level error enums
Our crate-level error enums have two, closely related issues.
Irrelevant variants
First, most of their variants are irrelevant to the operations that return them. For example,
Subsecond::new
returnsResult<Subsecond, LoxTimeError>
.LoxTimeError
is a five-variant enum, but the only variant relevant toSubsecond::new
isLoxTimeError::InvalidSubsecond
.To both library users and authors,
LoxTimeError
implies that an operation may return any of the possible variants, and forces the use of a match statement to distinguish between variants where there may only be one returned variant in practice. Both readability and usability are reduced.Breaking changes
Second, adding or removing variants from public enums is a breaking change. For example, by removing the
NonLeapYear
variant fromLoxTimeError
, we would risk breaking error handling code for unrelated errors, such asInvalidSubsecond
. This simple change would require a major version bump.What good enum errors look like
LoxCubicSplineError
is an example of an error enum done well. It has three variants, and all three of these variants may occur in a call toCubicSpline::new
. In other words, it is a tight upper and lower bound on the possible error scenarios of a particular operation. Its variants are unlikely to change, and it doesn't attempt to encapsulate the errors of unrelated operations.Proposed solution
How will Lox users handle errors?
Considering the list of Lox errors, they overwhelmingly relate to invalid inputs. This tells us that Lox errors are highly unlikely to be handled programmatically. If the input is wrong, it requires human attention to fix. Therefore, the universal error strategy will be to log the error and exit.
This tells us several things:
Box<dyn Error + Send + Sync>
, since the only useful information they contain is the message describing the ways in which the input was invalid.Why not
Box<dyn Error + Send + Sync>
?At runtime, Lox users are unlikely to need any further detail than a
dyn Error
is able to provide. However, well typed errors offer significant benefits to code authors and readers. Through the type system, they reveal the complete set of error scenarios for a given operation at a glance. For example, reading thatUtc::try_from::<Time<Tai>>
returns onlyUtcUndefinedError
is more powerfully descriptive than eitherBox<dyn Error + Sync + Send>
orLoxTimeError
.Struct and tuple error types additionally assist library authors in providing meaningful, standardized error messages. Constructing ad hoc errors from string literals is less likely to include the full wealth of data a consumer could use to debug their inputs, and is more prone to copy-pasting.
More granular errors are better for authors and readers, with no impact on consumers
For Lox, the ideal error is the narrowest type that fully represents all error scenarios that may occur during a function call.
thiserror
to define error types specific to the operation they're implementing, as this makes the intent and failure scenarios of their code clearer.thiserror
also makes composing errors simple with its#[source]
,#[from]
and#[transparent]
annotations.Box<dyn Error + Send + Sync>
without degradation of the debugging information available to them.Examples
Good
LoxCubicSplineError
is the narrowest type that represents everything that could go wrong when instantiating aCubicSpline
. It contains no unnecessary variants and gives the users the debugging information they need to resolve the issue.ArgumentSizeMismatchError
is a struct error representing the only thing that can go wrong for a given operation, and provides the user with the information they need to find the source of the problem.Bad
LoxTimeError
encapsulates error variants for many unrelated operations.UtcUndefinedError
does well to represent a single error scenario, but could be improved by including the invalid date in its error message to aid debugging.Drop the
Lox
prefixFinally, the
Lox
prefix for error types was useful when needed as a disambiguation from, e.g.std::io::Error
, but is unnecessary when errors are specific to the operations that return them.