Error refactoring and thiserror 2.0

[PhilippGackstatter]

Use thiserror 2.0 and refactor errors to be mostly in line with how our other errors are structured, e.g. see 0xPolygonMiden/miden-base#972.

So this changes errors to use lowercase messages without trailing punctuation and prefer source style over wrapping source errors.

Other notable changes include:

• SourceManagerError::Custom variant which, I think, makes sense to include as SourceManager is a trait implementable by other crates which cannot add variants to SourceManagerError. Let me know if this is not what we want.
• Similarly, EventError(String) was changed to EventError(Box<dyn Error>).
• Not using #[source] for errors that turn into Report, because Report does not implement core::error::Error and so source errors would be effectively swallowed that way.

There is a problem with building the assembly crate for wasm/no-std with this change. If I understand everything correctly, then this is because of the following. miden_miette uses miden_thiserror which exposes StdError. If we use neither a nightly compiler nor the std flag then StdError is not a reexport of the core::error::Error type but rather a copy of the Error trait, which means it is a separate type. miden_miette however with its Diagnostic trait expects StdError and that trait is no longer implemented by some errors, like SyntaxError. This previously worked because we used miden_thiserror to derive errors and so in that configuration, the derived error was StdError and that always matched what miden_miette was expecting.

It seems to me the proper fix would be to modify miden_miette to use thiserror 2.0. I've tested this change locally and both std and no-std works. This would make miden_thiserror obsolete, and we could consider yanking that crate.
My question is whether we want to go ahead with this change in miden_miette or not. Pinging @bitwalker, as I think you've made the no-std changes to miden-thiserror and miden-miette.

Or should we go one step further and ask if the miette author would be open to have the no_std changes upstreamed?

closes #1261

[bobbinth]

It seems to me the proper fix would be to modify miden_miette to use thiserror 2.0. I've tested this change locally and both std and no-std works. This would make miden_thiserror obsolete, and we could consider yanking that crate.
My question is whether we want to go ahead with this change in miden_miette or not. Pinging @bitwalker, as I think you've made the no-std changes to miden-thiserror and miden-miette.

I would modify miden_miette for now and then get rid of miden-thiserror. In parallel, we can also see if miette could switch to the latest thiserror - but I think that would be a separate task.

[PhilippGackstatter]

I would modify miden_miette for now and then get rid of miden-thiserror. In parallel, we can also see if miette could switch to the latest thiserror - but I think that would be a separate task.

Sounds good.
I tried to git push a branch to our miette fork but got this, maybe you can add me as a collaborator?

ERROR: Permission to 0xPolygonMiden/miette.git denied to PhilippGackstatter.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

I would do only the minimal necessary changes for now, because updating our miette fork from 7.1.0 to the latest miette at 7.4.0 comes with a bunch of conflicts. And if we can get miette to support no_std and use thiserror 2.0 then that will be obsolete.

[bobbinth]

I tried to git push a branch to our miette fork but got this, maybe you can add me as a collaborator?

You should be able to write to it now.

I would do only the minimal necessary changes for now, because updating our miette fork from 7.1.0 to the latest miette at 7.4.0 comes with a bunch of conflicts. And if we can get miette to support no_std and use thiserror 2.0 then that will be obsolete.

Yes, let's do the minimal changes for now.

[bobbinth]

Looks good! Thank you! Though not a very deep review from me.

[plafer]

Thank you for taking this one on! Left a few nits and questions

[plafer]

Shouldn't we use #[source] here? We should also add #[from] to remove the need for map_err() (see next comment)

[PhilippGackstatter]

Usually, yes, but LibraryError is sometimes also converted to Report (in assemble_library for example). This particular variant KernelConversion is only used in TryFrom which is user-called, so probably not converted to a Report, but that is just far too subtle and wouldn't be noticed if we change this in the future, so I chose the safe route by including it in the display impl so we don't swallow the source error.

I should've added a comment here, similar to the Forest variant in AssemblyError. Will do that, thanks for pointing it out.

[plafer]

Why are we removing the #[from]? Using map_err() everywhere can get pretty verbose, although it is a matter of taste

[PhilippGackstatter]

This is a matter of taste, yes, but I think there is a tendency to produce worse errors when using From. For example, you may have a LibraryError wrapped in an AssemblyError, which probably looks like AssemblyError::Library(#[from] LibraryError). The problem with this is that it doesn't add any context and is too generic. What is better, I think, is to have a dedicated error variant for the each error case where a library error must be wrapped into an assembly error. Imaginary examples:

enum AssemblyError {
   #[error("failed to assemble kernel library")]
    KernelLibraryAssemblyFailed(#[source] LibraryError),
   #[error("failed to load library")]
    FailedToLoadLibrary(#[source] LibraryError),
}

So it's possible to have LibraryError in AssemblyError multiple times and adding #[from] in this case produces conflicting implementations.
The lack of From is good, because it forces the developer to use map_err and think about what error case to map the error to, rather than bubbling it up blindly. This tends to lead to better errors.

Of course, it's not always wrong to use #[from], in simple cases it's fine, especially when you can't add meaningful context. I personally like to avoid it generally because it simply forces me to think about the error context more carefully and that's a good thing.

Ultimately you have worked more in this repo than I did, so feel free to make a call on whether to use it or not 🙂, but that is the rationale.

[plafer]

Yeah no this makes sense - the theory being that e.g. writing .map_err(FailedToLoadLibrary) would make you realize that this is not quite the variant you want, and make you create the new KernelLibraryAssemblyFailed instead. I'm good to try this moving forward.

[bitwalker]

@PhilippGackstatter The Report type specifically already captures context and backtraces (and includes even richer diagnostic information like associated source code spans). With thiserror + anyhow you can also achieve the same results.

I find that it is rarely the case that manually constructing errors versus using #[from] produces any better error information in practice. If people aren't adding context either way, you end up with the same outcome.

In general, at least with the assembler, the use of Report is encouraged/recommended to ensure that it is easier to do this, and to avoid a bunch of useless error variants being defined in favor of ad-hoc messages and site-specific context, but there is still a place for the former, when you simply need to know the type of error (and optionally, the backtrace).

Anyway, wanted to add my two cents to this.

[plafer]

Hmmm, so is miette's Report incompatible with Error::source? Or is this a by-product of the miden-miette situation described in the issue somehow?

Also MastForestError currently doesn't use any #[source] - but if we add one, then it won't get displayed by Report, right? Isn't this a larger issue?

[PhilippGackstatter]

Yeah, Report doesn't implement core::error::Error (zkat/miette#366), so anything that is not part of the Display impl of an error would be silently swallowed.

This makes me think that we might not want to return Report from library functions like assemble_*, but the underlying AssemblyError instead. On the other hand, this will usually mean that no Report is constructed for such errors and so the error is worse than it could be.
This is why for now, I'm including the printed report in the display impl of an error that wraps a Report, e.g.: https://github.com/0xPolygonMiden/miden-base/blob/67272131f02ed73b5b919223610e97afa7bdc8db/objects/src/errors.rs#L29
(The other reason being that Report doesn't implement core::error::Error).

I'm not sure if I'm missing something here, since the interop between core::error::Error and Report feels unsatisfying.

[plafer]

@bitwalker IIRC you'd like us to return Report much more, right? From previous conversations, I think you even suggested we convert AssemblyError to just use Reports everywhere?

I haven't played with miette enough to have a good intuition here, but typically when all your error are user-facing/to be printed only, you'd want to use anyhow/eyre/miette (instead of an enum that you end up printing out anyways). We could do that, but then I'm still not sure how we would "bridge properly" between e.g. errors coming from miden-processor which would all use Error::source.

TLDR this PR is already a big upgrade from the current state of things, but I wonder what "optimal strategy" we should be striving for here.

[bobbinth]

Yeah - we want to remove AssemblyError. There is an #1431 specifically for this. I think we can address this right after this PR.

[bitwalker]

The idea of Report (or alternatively, something like anyhow::Error if there isn't any associated source code, which is where Report really shines) is to allow ad-hoc diagnostics to be constructed/created, while handling all of the annoying/boilerplatey work of conversion/rendering/etc. The use of ad-hoc diagnostics ends up producing much more useful errors in practice IMO.

The downside of using Report (or anyhow::Error) is that you can't easily obtain the concrete underlying error type (if there is one), in order to say, assert that it is of a specific type. I don't find this particularly important IMO, and there are better ways to test errors (e.g. how we test assembler errors), and downcasting can be used in many situations if really needed.

You can wrap Report in a type that allows #[from] or #[source] conversion, IIRC that is what the RelatedError type exists for, but it is probably better to not have a variant that is just a transparent wrapper around Report.

[plafer]

LGTM!