fix: remove source error interpolation from #[error] format strings#3172
Open
mitchmindtree wants to merge 3 commits into
Open
fix: remove source error interpolation from #[error] format strings#3172mitchmindtree wants to merge 3 commits into
#[error] format strings#3172mitchmindtree wants to merge 3 commits into
Conversation
mitchmindtree
changed the title
#[error] format strings#[error] format strings
Remove interpolation of `#[from]`/`#[source]` fields from `#[error("...")]`
attributes. Since `#[from]` implies `#[source]`, the inner error message
was duplicated in both `Display` output and the `source()` chain. Each
layer now only describes its own context while inner errors remain
accessible via `source()`.
Also fix `IndexedInstructionError` to use `#[source]` instead of
interpolating the error field in the format string.
Update the test harness to format errors via `anyhow::Error`'s `{:#}`
(alternate Display), which walks the `source()` chain. This preserves
the full error context in test expectations.
Remove source error interpolation from #[error] format strings on `#[from]` variants across utilities, fields, and algorithms crates. When thiserror sees both #[from] and a format string that interpolates the inner error, it produces duplicated output (the message printed twice). Using #[error(transparent)] delegates Display and source() correctly.
…rrors
Group A — replace trivial manual `From` impls with `#[from]`:
- `SynthesisError::IoError`: add `#[from]`, delete manual `From<std::io::Error>` impl
- `AHPError::ConstraintSystemError`: add `#[from]`, delete manual `From<SynthesisError>` impl
- `GroupError::FieldError`: add `#[from]`, delete manual `From<FieldError>` impl
Group B — add `#[source]` to named-field variants in `CheckBlockError`:
- `InvalidPrefix`: add `#[source]` on `error` field, fix typo ("as" → "has"),
remove `{error:?}` interpolation
- `SpeculationFailed`: add `#[source]` on `inner`, remove `{inner}` interpolation
- `VerificationFailed`: add `#[source]` on `inner`, remove `{inner}` interpolation
Format strings no longer interpolate `#[source]` fields, avoiding the
duplication bug fixed in the prior commit. The inner errors remain
accessible via `Error::source()`, enabling proper error chain traversal.
mitchmindtree
force-pushed
the
mitchmindtree/fix-error-fmt
branch
from
4c7902a to
f3b0097
Compare
mitchmindtree
changed the title
#[error] format strings#[error] format strings
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
@kaimast mentioned he's looking into improving
Displayconsistency for errors across snarkvm, and it made me realise the error formatting of some of the errors I introduced (in #3081, #3082) is incorrect.Several of the
thiserrorerror types I added interpolate#[from]fields in their#[error("...")]format strings (e.g.#[error("VM execution failed: {0}")]where{0}is#[from] SomeError). Because#[from]implies#[source], the inner error's message appears in bothDisplayoutput and thesource()chain - causing duplication when consumers walk the chain. ThestdErrordocs explicitly advise against this:Further Context
This is an issue because when an error is surfaced by an app downstream, they often need to have control over how to format the error. E.g. in some cases they may only want to surface the top-level error, in others they may want to present the full causal chain.
anyhowoffers the ability to easily format either style:{}(Display) - prints only the outermost error message{:#}(alternateDisplay) - prints the full chain, e.g. "Failed to read instrs from ./path: No such file or directory (os error 2)"{:?}(Debug) - prints the full chain plus backtracewhich is another part of what makes it better-suited for app-level error handling.
The Fix
This PR removes the interpolation from the format string so each layer only describes its own context. The inner error remains accessible via
source().