Structured resolver errors by PhoebeSzmucer · Pull Request #2482 · bytecodealliance/wasm-tools

PhoebeSzmucer · 2026-04-07T11:28:55Z

Part of #2460 (part 1 was here).

This PR replaces most of anyhow usages in the resolver with structured ParserError type, which carries structured error information (with spans).

This reverts commit 0692157.

alexcrichton · 2026-04-08T17:38:12Z

Happy to take a look when you feel this is ready, but I'm interpreting the draft state as "come back later" for now -- just wanted to note this

PhoebeSzmucer · 2026-04-08T17:43:18Z

Thanks @alexcrichton. No review needed just yet - I'm still cleaning up some final things and do one more self review, and I'll mark it as ready soon.

crates/wit-parser/src/ast/error.rs

crates/wit-parser/src/resolve/mod.rs

PhoebeSzmucer · 2026-04-09T17:08:09Z

crates/wit-smith/src/lib.rs

                    return Err(arbitrary::Error::IncorrectFormat);
                }
                let err = e.to_string();
                if err.contains("shadows previously") || err.contains("mismatch in stability") {


Both of these are ResolveErrorKind::Semantic. We could introduce a dedicated type for them to prevent string matching, either now or in a follow up PR.

Although wit-smith is just a fuzzer so idk if it's worth it.

Yeah this is a great use case for dedicated error kinds, but totally fine to do in a follow-up

I'll just do it now then, it's easy enough

crates/wit-parser/src/resolve/mod.rs

PhoebeSzmucer · 2026-04-10T10:07:55Z

@alexcrichton this is ready for a review now. I left some inline comments on the PR

alexcrichton · 2026-04-10T20:38:36Z

crates/wit-parser/src/resolve/fs.rs

-        let (pkg_id, inner) = self.sort_unresolved_packages(top_pkg, deps)?;
+        let sort_result = self.sort_unresolved_packages(top_pkg, deps);
+        let (pkg_id, inner) =
+            sort_result.map_err(|e| anyhow::anyhow!("{}", e.highlight(&self.source_map)))?;


I feel like I might have asked this already on the previous PR of yours, so I apologize if so, but here this feels kind of bad to stringify the error and forget about the original error. The main downside here is that the returned value of anyhow::Result doesn't have the ability to downcast the error to a concrete error type and inspect it.

I forget, though, was this something where the anyhow::anyhow!("{}", ...) is temporary? Or is this intended to be more permanent? For example should ResolveError have a "from I/O error" variant and this function returns a ResolveError?

Or, alternatively, one thing I thought of is that it's useful to have a decoupled error from the SourceMap where the returned error has all the info it needs to be rendered without pairing to something else, which this call to highlight is doing. I was thinking, though, that highlight would mutate the error-in-place (e.g. filling in some fields) as oppose to returning just a string

I feel like I might have asked this already on the previous PR of yours, so I apologize if so, but here this feels kind of bad to stringify the error and forget about the original error. The main downside here is that the returned value of anyhow::Result doesn't have the ability to downcast the error to a concrete error type and inspect it.

You're right, I was a bit sloppy here. I think I can fix this by using anyhow::Error::from instead of anyhow!, something like anyhow::Error::from(e).context(msg). I'll play around with it a bit.

I forget, though, was this something where the anyhow::anyhow!("{}", ...) is temporary? Or is this intended to be more permanent? For example should ResolveError have a "from I/O error" variant and this function returns a ResolveError?

In this place it's not temporary. I personally don't think we should introduce any I/O error variants to ResolveError - the resolver's main job is not to interact with the filesystem, but to deal with already loaded files. Adding I/O variants would mix the concerns too much. I/O matters only at the boundary of the resolver.

Or, alternatively, one thing I thought of is that it's useful to have a decoupled error from the SourceMap where the returned error has all the info it needs to be rendered without pairing to something else, which this call to highlight is doing. I was thinking, though, that highlight would mutate the error-in-place (e.g. filling in some fields) as oppose to returning just a string

I generally prefer the errors to stay very lightweight - ie only contain the span info and the necessary metadata. Adding rendered strings to errors means that we lose a clean separation between what went wrong (spans/metadata), and how to display it (source map rendering). And if we mutate the error, the error now becomes dependent on when and whether you call that mutation (which increases the chance of bugs).

I also plan to revisit the highlight naming and the overall error presentation in a follow-up - now that errors are structured, it should be much easier to reason about where and how rendering happens, and to improve individual error messages.

alexcrichton · 2026-04-10T20:41:30Z

crates/wit-smith/src/lib.rs

                    return Err(arbitrary::Error::IncorrectFormat);
                }
                let err = e.to_string();
                if err.contains("shadows previously") || err.contains("mismatch in stability") {


Yeah this is a great use case for dedicated error kinds, but totally fine to do in a follow-up

alexcrichton · 2026-04-10T20:43:58Z

crates/wit-parser/src/resolve/mod.rs

-        self.source_map.rewrite_error(|| ret)
+        ret


Question on the removal here, and I feel like again I may have already asked this and/or you already answered so sorry if so, but this seems like a bit of a regression insofar as callers need to rewrite the error as opposed to that happening automatically internally. That's sort of where above I was thinking that the errors would be rewritten in-place so these calls to rewrite_error would stick around.

There is a broader design question here - whether erros should rewrite themselves, vs rendering being explicit. I'm intentionally trying to keep the errors as pure data (spans + metadata), since I think that's the pattern that most compilers (including rustc) use.

For example rustc has an Emitter which owns the source map, renders the diagnostics, and the diagnostics themselves are just metadata (without source code).

I think this pattern is the best, because:

The errors stay light, and tools that don't care about rendered code don't have to pay the cost of it

We can have emitters for different targets

The data and presentation layer are clearly separated

Errors can stay immutable

In practice, rendering only needs to happen at a few boundary points where ResolveError crosses into user-facing output. The source map is always available there, so I don't think it will be burdensome enough to warrant prerendering the errors (which has its own downsides).

What do you think?

alexcrichton · 2026-04-10T20:46:46Z

crates/wit-parser/src/resolve/mod.rs

            let span = iface.span;
-            if !resolve
-                .include_stability(&iface.stability, &pkgid, span)
-                .with_context(|| {


The loss of the context here, and a few below, is a bit unfortunate because it means that the error messages have less contextual information than they did before. Would it be possible to retain a context-like method on ResolveError which acts similar-ish to anyhow::Context?

I think some of that contet was pretty redundant, and leaked details about the internal workings of the resolver. Example https://github.com/bytecodealliance/wasm-tools/pull/2482/changes#diff-e50c5a3992e0125ef03e2863997fb3e17e33acd1f626f5ed8f5e9a6039880322 - I think the new version is better, but let me know if you think otherwise.

In this case I might have dropped valuable context though - let me generate test cases/snapshots for each one of these cases, and see how they look now vs before. If the errors don't have enough info, I'm more than happy to add extra metadata to the error variants.

I can also add a context method, but I think it's generally better to have typed/structured metadata rather than accumulating strings.

I definitely don't want to introduce any regression to how errors are presented (if anything, I want to make them better). Let's make sure we're happy with snapshot changes before this PR merges.

PhoebeSzmucer added 13 commits April 1, 2026 14:45

[wit-parser] Migrate to structured errors in the resolver

b1bdd14

Rename structs

2aeff69

Remove unneeded methods

2bb93f0

Remove dangling doc

9177bad

Merge branch 'main' into structured-resolver-errors

8bce150

Doc improvements

f924443

Update snapshot

0641505

Don't reference a non-existent method in docs

2043c6c

Fix bad merge

89df4c4

sm_idx -> *source_maps_index

0692157

Revert "sm_idx -> *source_maps_index"

6496897

This reverts commit 0692157.

Remove push_groups, push_group returns a structured error

d90a9f2

Remove stale reference

151cda0

PhoebeSzmucer added 5 commits April 9, 2026 17:59

Use fewer abbreviations

9ab9544

Introduce ResolveError::new_semantic

69fe727

Bring back push_groups because it makes it easier to test things

c44b8ba

Rename for consistency

d489aea

Fix typo

231996e

PhoebeSzmucer commented Apr 10, 2026

View reviewed changes

PhoebeSzmucer marked this pull request as ready for review April 10, 2026 10:04

PhoebeSzmucer requested a review from a team as a code owner April 10, 2026 10:04

PhoebeSzmucer requested review from fitzgen and removed request for a team April 10, 2026 10:04

Merge branch 'main' into structured-resolver-errors

1a01c31

alexcrichton requested review from alexcrichton and removed request for fitzgen April 10, 2026 20:36

alexcrichton reviewed Apr 10, 2026

View reviewed changes

Conversation

PhoebeSzmucer commented Apr 7, 2026

Uh oh!

alexcrichton commented Apr 8, 2026

Uh oh!

PhoebeSzmucer commented Apr 8, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PhoebeSzmucer commented Apr 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants