Error design guidelines

[PhilippGackstatter]

Unless I missed it, our error handling story hasn't been fleshed out yet and it seems we were waiting on thiserror to become no-std compatible, which it now is. This means we can rework our errors now but before we do that, I think we could throw some considerations into the mix.

The following suggestions are roughly ordered from presumably least controversial to somewhat controversial. Happy to receive any input!

Use core::error::Error

With core::error::Error stabilized since Rust 1.81 we can now rewrite the feature-gated impls:

// Previously:
#[cfg(feature = "std")]
impl std::error::Error for MyError {}
// Now:
impl core::error::Error for MyError {}

This is only relevant when not using thiserror.

Use thiserror

Additionally, thiserror 2.0.0 has been released and supports no_std environment. In a discussion in miden-vm there seems to have been consensus on using thiserror.

Lowercase, no-punctuation error message

For best interoperability we should follow this advice from the Rust API Guidelines:

The error message given by the Display representation of an error type should be lowercase without trailing punctuation, and typically concise.
https://rust-lang.github.io/api-guidelines/interoperability.html#error-types-are-meaningful-and-well-behaved-c-good-err

This is good and necessary because we can't anticipate if error messages will be embedded in others (typically separated by :) and capitalizing or adding punctuation makes the error message look odd.

• bad: Update failed.: Value was wrong.: Amount exceeded.
• good: update failed: value was wrong: amount exceeded

Make error enums non_exhaustive

The rational is that users can still match on specific errors, but since errors are rarely handled exhaustively making an error enum exhaustive is frequently unnecessary. On the win-side it does make the error future-proof as we can add new variants in a non-breaking way (e.g. particularly important post-1.0, but even now between 0.X versions).

Don't implement Clone and PartialEq

We should avoid implementing Clone, PartialEq and Eq for our errors (as we currently do in miden-base) as the core::error::Error trait does not require Clone or PartialEq and thus errors do not generally implement those. Implementing those traits means we require it on all errors we wrap too and this makes wrapping errors that do not implement these traits impossible. Examples where we have this problem are here:
https://github.com/0xPolygonMiden/miden-base/blob/a62865204823e078037fbb31c0f78ef402e3d654/objects/src/errors.rs#L24-L26

It also means we cannot add a OpaqueVariant(Box<dyn core::error::Error + Send + Sync>) variant to an enum because that boxed error does not implement Clone and PartialEq.

Locally I removed the mentioned derive macros from our errors everywhere to see where it would break and it seems we were only using it (at least in miden-base) to compare errors in tests. Here we can replace assert_eq!(error, MyError::Err) with assert!(matches!(error, MyError::Err)) (or assert_matches!). I did not find a compilation error for the missing Clone.

Display XOR source

When implementing errors one has to make a choice between two possible styles. There is guidance in Rust which says that:

Return source errors via Error::source unless the source error's message is included in your own error message in Display.
rust-lang/project-error-handling#27 (comment)

This can be summarized as "Display XOR source". So, using thiserror exactly one of the following:

pub enum AccountError {
    #[error("asset vault failed to update")]
    AssetVaultUpdateError(#[source] AssetVaultError),
}

or

pub enum AccountError {
    #[error("asset vault failed to update: {0}")]
    AssetVaultUpdateError(AssetVaultError),
}

Despite that repo being archived, the advice is still valid. Error reporters like anyhow or eyre (perhaps also miette?) iterate the source errors and produce nice reports. If one includes the error in the message and returns it from source then it will cause duplicate error messages. The guidance just says that we should not do both {0} and #[source] which is easy enough and avoids the duplication, but deciding which one to actually use is not straightforward.

The source style gives error reporters the chance to format errors in different ways (single-line vs. multi-line, color vs. no-color, etc.), e.g. using anyhow with the source-style example above:

Error: asset vault failed to update

Caused by:
    0: adding fungible asset would exceed max amount
    1: fungible asset amount 500 is too big

On the other hand, with the include-in-Display style we get a slightly less nice report, although the difference here is just in readability:

Error: asset vault failed to update: adding fungible asset would exceed max amount: fungible asset amount 500 is too big

The potentially biggest issue with source style is that that users who are not aware of reporters and simply print an AccountError would just see asset vault failed to update because that's all the Display impl of that type does and expects a reporter to do the rest. There hasn't been a fix on the std/core level for this yet (see also the discussion in the linked thread above). So we have to decide what route we want to take, the nice one but with the arguably worse UX (source-style) or the less nice one but with hard-to-get-wrong UX (include-in-Display).

When using include-in-Display style and we wrap an error type that does follow the source-style approach, we need to build a report ourselves in our Display impl, otherwise we end up swallowing some errors.

Another thing to note is that, to the best of my knowledge, developers typically unwrap on errors in prototype or test code (where errors are particularly prevalent), and if an error occurs they see the result of Debug and not Display. They have to go out of their way (unfortunately) to see the much better error message. Hence using the source-style would only be an issue if developers were displaying errors themselves without using something like anyhow. Since most devs do use a convenient crate like that I think source-style wouldn't be as big of a problem for the developer experience.

My personal opinion is that while there is no guarantee that error reporters are being used they often are when actually printing error messages instead of unwrapping them (which uses the Debug impl). It seems to me that the Rust ecosystem prefers the source-style for its higher flexibility. Both styles have their caveats (source style needing reporters and include-in-Display style needing reporters when wrapping source-style errors), so I would lightly suggest to just go with source style.

Use Result in tests

The above discussion spawned another thought for me: Since we always unwrap in test code and thus see the Debug impl of errors, couldn't we make life for ourselves better by writing our tests with a Result<(), anyhow::Error> type and use ? instead of unwrap in test code?Together with RUST_BACKTRACE=1 we would get our own nice error messages if tests fail (plus validating that they are in fact nice or prompting an improvement) instead of the debug representation. With the backtrace (which anyhow adds with that env var) we could still easily figure out which line in the test code failed, so we wouldn't loose that information compared to the panic caused by unwrap.
This doesn't suggest to rewrite all our tests using this style immediately. We we could just start experimenting with this.

Summary

To summarize the suggestions in this issue:

• Use impl core::error::Error for MyError {} unless using thiserror.
• Use thiserror whenever possible to derive the Error trait impl.
• Use lowercase, no-punctuation error messages.
• Mark error enums as non_exhaustive.
• Avoid implementing additional traits (like Clone and PartialEq) for Errors that source errors do not implement so we can properly wrap them.
• Decide on Display XOR source with a light suggestion towards source style.
• Consider experimenting with Result<(), anyhow::Error> in tests and whether it helps with debugging tests.

[Mirko-von-Leipzig]

I largely agree with what you've said. I'll also note that derive_more at some point advertised that they fully covered thiserror and included nostd support. Just mentioning in case derive_more is already widely in-use within miden - not suggesting we use it instead; thiserror is more widespread in general.

Make error enums non_exhaustive

I don't quite agree with this. From a library perspective I'm sure its nice not having to worry about semver breakages, but I almost pedantically never wild-card error variants in matches. I want to be forced to think about new error variants. If I'm matching I clearly had some subset that I cared about - who's to say this new variant doesn't fall into that category?

One can be carefully aware of this and ensure one checks all references to the error type. This sort of works as the PR author; but does mean reviewers cannot trivially check for correctness without being aware of all instances themselves.

Don't implement Clone and PartialEq

This does make dx worse. I'm usually annoyed when errors don't implement this but I understand the rationale. assert!(matches!(A, B)) also doesn't display the debug info on failure so we'd ideally use assert_matches crate until that lands in std one day.

Display XOR source

I prefer the richness of source style. Its a pity it isn't automatic/default derived (iiuc).

Use Result in tests

I think this is a good idea. It also enhances loop based tests by making it trivial to embed the iteration info with context:

for i in x..y {
    foo(inputs[i]).with_context(|| format!("Iteration {i}"))?;
}

---

I'll also add my 2c that we overuse error variants. At least from the node perspective we could replace 99% instances with anyhow and end in the same place - a top level application panic. Though I don't really think this should change; I'm just grumpy :D

[PhilippGackstatter]

Thanks for sharing your thoughts!

derive_more

I haven't seen derive_more in our crates yet, so I think thiserror it is.

never wild-card error variants in matches

I'm assuming that we may be thinking of different errors here:
The AccountError was top of mind for me:

https://github.com/0xPolygonMiden/miden-base/blob/a62865204823e078037fbb31c0f78ef402e3d654/objects/src/errors.rs#L24-L71

I don't think anyone really exhaustively matches that one. If the error is smaller with fewer variants, then I agree that we could make it exhaustive. So my original statement was perhaps too generic and it depends on the concrete case. But this may all be due to a deeper error design issue related to:

we overuse error variants.

Perhaps, yes: Should the AccountError even be that large in the first place?

One issue with errors in Rust and probably in general is that when composing functions from other modules with different error type one tends to just add a variant for that other error to one's own error type without thinking too much about it. Sometimes we have to bubble such errors up but then we should make a proper error variant (meaning describe the exact error case) in our own and map the error. Often though the composing module will have some invariants that will make certain errors from the third party module impossible and those should generally be expect'ed on. That would reduce the number of variants by a bit.

There is also always the question about recoverable and unrecoverable errors. The latter could be panics or collected in a single variant in the error type to not pollute the error type too much. But it's generally hard to categorize errors because libraries can be used from so many contexts. So often one has to conservatively give the choice to the caller and that means adding a dedicated variant to the error and that leads to bigger error enums.

And all that makes me think that enums won't be matched on exhaustively because they will probably end up being of decent size.

assert!(matches!(A, B)) also doesn't display the debug info on failure so we'd ideally use assert_matches crate

Fully agree!

[bobbinth]

• Use impl core::error::Error for MyError {} unless using thiserror.
• Use thiserror whenever possible to derive the Error trait impl.
• Use lowercase, no-punctuation error messages.

Agreed!

• Mark error enums as non_exhaustive.

I don't have a strong preference here. Maybe for now we keep things as they are (i.e., keep enums exhaustive) and create a separate issue to make error variants non exhaustive in the future.

• Avoid implementing additional traits (like Clone and PartialEq) for Errors that source errors do not implement so we can properly wrap them.

Agreed!

• Decide on Display XOR source with a light suggestion towards source style.

I prefer the source approach.

• Consider experimenting with Result<(), anyhow::Error> in tests and whether it helps with debugging tests.

I like this - but I'd probably do this as follow-up work.