Revamp errors

[asmello]

Introducing thiserror for producing granular error types everywhere. This is primarily intended to support consuming Buffrs as a library, but also lays the foundations for easily producing better error messages (think miette and the likes).

There's A LOT of code touched by this PR, but the changes should be pretty straightforward. I tried not to make structural changes but some refactoring was unavoidable.

[asmello]

Thanks for the initial feedback @mara-schulke and @j-baker! This is what I'm going to do next:

• Add a buffrs::Error struct that will be the single error type exported from this crate.
• Make all other errors pub(crate).
• Define a buffrs::ErrorType enum with common error categories.

The root Error type will have a kind() method that returns an enumeration that can be used to distinguish categories of errors that are shared globally across this crate. It will also always be Display as a short one liner, but to expose more context we can provide a source() method that will return an impl Error implemented by the original granular error. Since this generic only exposes the thin Error API it doesn't leak any implementation details, while still making it possible for the granular errors to provide rich contextualised messages.

Open questions:

• What categories of errors should we include in the kind() enumeration?
• How granular should the internal errors be? Which ones can we merge?
• Is IoError useful?
• Should we keep #![warn(missing_docs)] on or is this demanding too many useless comments?
  • Actually this is probably a moot point since I don't think it'll complain once we make the errors crate visible.

EDIT: change of plans, now exporting several error types, see comments below for reasoning.

[mara-schulke]

Your action plan sounds good @asmello 😊

[asmello]

I've been doing some further reading on error handling best practices. I think we're on the right track with the dual internally-granular and externally-opaque principles.

That said there are some additional things I've been thinking about:

• Composition: source errors should not be directly included when the wrapper error is displayed. We should delegate to libraries like eyre and miette that can render error chains nicely.
• I do think having function-specific error types is a good thing, because while there's some overlap between errors produced by functions in each module, we want their returned enums to be as specific as possible. Any variants that can never be returned from a function shouldn't be in the enum! I don't want to handle TOML parsing failure when calling a function that only checks the existence of a file. Also it hurts context capture, as we lose information about where the error was produced, and modularisation, as it entangles functions to co-depend on a shared type. This is covered extensively in this writeup that I really like.
• I think we should make all error messages lowercase, to be in line with core library recommendations.

So in addition to the previous plan, I will also work on:

• Change the error messages to be lowercase one liners that don't include source messages.
• Include source errors in some places where I've not included it out of fear of creating a hard dependency on 3rd-party libraries. Since these granular errors won't be public, this isn't a concern anymore and we get the benefit of even more specific error messages via chaining.

[asmello]

Worked on changes today (haven't pushed yet), and as I went through the code again I started to think that granular public errors are not the root of the problem, and not intrinsically a bad thing. The issue James flagged with leaking internal details is extremely valid, but it doesn't mean we should drop error enumeration altogether. I believe it is a fallacy that we should use std::io::Error as a role model (more on that later).

Here are the actual things we wish to avoid:

• Leaking types from 3rd-party libraries via our public API
• Exposing details that are subject to change in the future
• Creating friction to adding new error conditions (or metadata) — in other words, prevent the library from evolving

To implement a good error system that can evolve fearlessly, it should be sufficient to:

• Hide 3rd-party types as private fields and provide accessor methods with stable signature as an abstraction layer
  • All the exposed types should be defined locally in the crate or be part of std
• Merge similar or related error conditions together, and capture implementation-defined errors under an abstract Other variant or equivalent
• Mark enums as #[non_exhaustive] so that variants can continue to be added (same for the fields)

Taking this example from James' comment earlier:

So e.g. for reading credentials...

• Locate
• Io
• Toml

what is an API caller of this thing supposed to be doing if a 'locate' error comes back?

A Locate error here (actually Locate::MissingHome) would indicate that the environment is misconfigured. Since this is an environmental problem, there usually isn't much the caller function can do to handle the error itself, but suppose the caller was a tool that manages virtual environments and initialises a sandboxed Buffrs project, then perhaps this would be a recoverable error (not that wild of an example! this is something that could potentially happen in a tests module). At the very least, by capturing this failure mode with a unique variant, we allow the caller to safely remap the error to a local type, change the display message etc. — this is vital for healthy composition across crates.

Consider if we instead just mapped this failure mode to an Io variant, we'd have to resort to string matching to distinguish between environment problems and filesystem errors, which have very different solutions.

It's easy for a lib to drop type information, impossible for consumers to safely recover it.

Another thing, we don't want to directly expose the toml::de::Error error inside the Toml variant publicly, but that doesn't mean a parsing-related variant isn't important to have. It just needs to abstract away the specifics of how we parse the payload.

BTW I like this writeup I found today, it covers a good chunk of good practices.

Why I don't think std::io::Error is a good role model

The IO module faces the relatively peculiar problem of having to report heavily platform dependent errors. Not only there are distinct sets of errors that can happen for the same operation as implemented in different runtimes, the details available about those errors aren't particularly consistent and are subject to change outside of Rust's version control ecosystem. Thus to be portable, the exposed Error type must only capture the common denominator and delegate platform-dependent specialisation to business logic.

This sort of situation can happen in crates that must interface with some types of external low-level libraries, and that are foundational for a wide array of applications. But Buffrs has the privilege of only interacting with other Rust libraries and its direct and transitive error conditions are all well defined. So there's no reason for its errors types to be agnostic to the underlying context.

Moreover, there are downsides to the single-public-error approach worth noting:

• To recover the fully contextualised errors one must downcast, which is an unsafe operation
• The caller can only rely on documentation (and source-code inspection) to determine which conditions can lead to error results
  • This is often inadequate! Consider how reqwest::RequestBuilder::build() can fail, but docs don't mention how. The provided Error variant also doesn't give us a clue, as the type is used for anything from TLS misconfiguration, connection errors or invalid HTTP header value... We have no way of knowing and can only hope to get an informative display message!
  • std::io::Error::kind() mitigates this problem a bit, but it's still a problem that we have no reliable way of knowing which variants can return from a given call...

I think that as long as we account for the pitfalls mentioned earlier in this comment, defining a few high-signal public variants is a sensible design for Buffrs. In particular, I think it's ok for each function in the command module to define its own error type, as they have well defined behaviour with well defined failure modes. Outside of that, a few common errors defined globally should probably cover the rest - HttpError, ParseError, IoError etc. each one abstracting away the specifics of 3rd-party libraries and how they are used, of course. Here's one example:

[derive(thiserror::Error, Debug)]
#[error("{path}: at line {line} column {column}: {message}")]
pub struct ParseError {
    path: PathBuf,
    line: usize,
    column: usize,
    message: String,
}

impl ParseError {
    pub fn path(&self) -> &Path {
        &self.path
    }

    pub fn line(&self) -> usize {
        self.line
    }

    pub fn column(&self) -> usize {
        self.column
    }

    pub fn message(&self) -> &str {
        &self.message
    }
}

There might be more context to be captured, but this is the basic idea of something that doesn't expose any unnecessary internal details, can compose nicely and should integrate well with miette and other reporting crates.

Another tentative example I've been fiddling with:

/// Error produced when initializing a Buffrs project
#[derive(Error, Display, Debug)]
#[non_exhaustive]
pub enum InitError {
    /// a manifest file was found, project is already initialized
    AlreadyInitialized,
    /// invalid package name
    InvalidName(#[from] PackageNameValidationError),
    /// IO error
    Io(#[from] std::io::Error)
}

Unless we make a breaking change to the init command (for example, by changing its default behaviour to override the manifest file if it exists), this interface is stable. Even then, there are cases where we may simply deprecate variants by making them unused until the next major version. Even though this error type is tied to a single function, it shouldn't create any maintenance friction.

[asmello]

This PR is ready for another round of feedback.

[mara-schulke]

Can we upercase the doc comments, and lowercase them manually in the wrapping error?

[asmello]

The doc comments are now unified with the error messages, so unfortunately this stylistic conflict is something we have to live with. The alternative is to have duplicative comments and messages like before.

[mara-schulke]

this one can and should go

[asmello]

Unfortunately not, because if we add a top-level doc-comment it will override the per-variant display implementation. It's another downside of using displaydoc...

[mara-schulke]

is this a meaningful doc comment?

[asmello]

Not at all. It's just there to tell displaydoc how to render the Other variant (ie, transparently). I could use #[displaydoc("...")] to make it possible to additionally include a non-display doc-comment, but I'm not sure what I'd put there.

[asmello]

@jonhoo this is a complex and subtle enough design decision that we'd deeply value your input as well!

[mara-schulke]

Lets not close this already, i believe further discussion is needed

[asmello]

Superseded by #103