[wit-parser] Migrate to structured errors in the AST/package parser by PhoebeSzmucer · Pull Request #2465 · bytecodealliance/wasm-tools

PhoebeSzmucer · 2026-03-13T10:13:02Z

This PR replaces anyhow error handling in the AST/parser laye rwith structured PackageParseErrors and PackageParseErrorKind types. Errors now carry span information directly, enabling better error diagnostics and better integration with downstream tools that use wit-parser programatically.

Part of a larger change: #2460 (see that link for API discussion).

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

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

PhoebeSzmucer · 2026-03-13T10:33:31Z

crates/wit-parser/src/ast.rs

+    ///
+    /// On failure returns `Err((self, e))` so the caller can use the source
+    /// map for error formatting if needed.
+    pub fn parse(self) -> Result<UnresolvedPackageGroup, (Self, PackageParseErrors)> {


Note that we're now returning the source map as well. Otherwise the consumers would need to clone the source map if they want to use it for resolving spans (since parse consumes self).

Do you think it'd be reasonable to change this signature to &self to avoid the need for that? That's a bit more idiomatic and I'm not seeing off the top of my head why self is used here vs &self

The reason it takes self is that UnresolvedPackageGroup takes the ownership of source_map: SourceMap, so we'd either have to clone or introduce a lifetime.

SourceMap contains the contents of multiple files so it might not be very cheap to clone.

I will investigate if UnresolvedPackageGroup can just contain a reference to a source map.

Ok so I think cloning won't be cheap, and adding a lifetime will be pretty invasive because it will propagate to a lot of places. I don't have a better idea than returning (Self, ParseError), but let me know if you do.

PhoebeSzmucer · 2026-03-13T10:33:55Z

crates/wit-parser/src/ast.rs

+    /// Runs `f` and, on error, attempts to add source highlighting to resolver
+    /// error types that still use `anyhow`. Only needed until the resolver is
+    /// migrated to structured errors.
+    pub(crate) fn rewrite_error<F, T>(&self, f: F) -> anyhow::Result<T>


This will go away once the resolver is also migrated to structured errors.

crates/wit-parser/tests/ui/parse-fail/bad-function.wit.result

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

alexcrichton · 2026-03-16T22:55:05Z

I apologize I've been extra busy the past few days and I'm about to get even busier traveling for a conference + then a vacation. I wanted to at least comment and say that I haven't forgotten about this and I'll try to get to it when I can. Other maintainers can certainly take a look too, but @PhoebeSzmucer wanted to at least let you know I may take a bit.

PhoebeSzmucer · 2026-03-17T09:28:37Z

@alexcrichton that's completely fine - I'm actually also on vacation now, plus this PR and the LSP is just a side project for me, so it can definitely wait.

alexcrichton

Thanks again for your work on this! Overall looks good, but some comments on idioms/style below

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

alexcrichton · 2026-03-18T05:22:26Z

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

+                                return Err(PackageParseErrors::from(
+                                    PackageParseErrorKind::Syntax {
+                                        span: f.name.span,
+                                        message: "duplicate constructors".to_owned(),
+                                    },
+                                ));


Bikeshedding this slightly, one option here might be:

return Err(PackageParseErrors::new_syntax(f.name.span, "duplicate constructors"))

where the constructor new_syntax takes message: impl Into<String> or somthing like that.

Would you want me to do this for all variants? I'm worried about things like:

ItemNotFound { span: Span, name: String, kind: String, hint: Option<String>, },

where the order of strings matters (ie PackageParseErrors::item_not_found(span, "A", "B", None) is different to PackageParseErrors::item_not_found(span, "B", "A", None)) and it's easy to accidentally mix it up.

And we'll also have some variants that contain multiple spans (e.g. duplicate package).

I introduced just the new_syntax method, since that's the most common one.

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

alexcrichton · 2026-03-18T05:24:45Z

crates/wit-parser/src/ast.rs

+    ///
+    /// On failure returns `Err((self, e))` so the caller can use the source
+    /// map for error formatting if needed.
+    pub fn parse(self) -> Result<UnresolvedPackageGroup, (Self, PackageParseErrors)> {


Do you think it'd be reasonable to change this signature to &self to avoid the need for that? That's a bit more idiomatic and I'm not seeing off the top of my head why self is used here vs &self

alexcrichton · 2026-03-18T05:25:45Z

crates/wit-parser/src/lib.rs

        let mut map = SourceMap::default();
        map.push_str(path, contents);
        map.parse()
+            .map_err(|(map, e)| anyhow::anyhow!("{}", e.highlight(&map)))


We'll generally want to avoid stringifying errors like this since it loses type information -- but isn't the highlighting handled by by parse itself?

but isn't the highlighting handled by by parse itself?

There are two parses here - SourceMap.parse and UnresolvedPackageGroup.parse. The former is what we call here, and it doesn't do highlights anymore (it returns structured errors which can then be highlighted when needed).

We'll generally want to avoid stringifying errors like this since it loses type information

Correct. I just didn't touch the UnresolvedPackageGroup struct for now. This struct just contains methods that are a wrapper for things like "read file, add string to SourceMap, call SourceMap.parse", ie:

UnresolvedPackageGroup::parse - convert path to string, then parse the contents

UnresolvedPackageGroup::parse_str - this is a thin wrapper around SourceMap::parse so it can start returning PackageParseErrors

UnresolvedPackageGroup::parse_path - calls UnresolvedPackageGroup::parse_dir or UnresolvedPackageGroup::parse_file depending on whether the path is a dir or a file

UnresolvedPackageGroup::parse_file - reads the file contents, and then parses them

UnresolvedPackageGroup::parse_dir - adds all files in the directory to the source map, then parses it.

If we were to move all of these to return ParseErrors, then ParseErrors` would have to gain variants for bad IO/path operations, and I'm not sure if we want to go there? It might be good to keep parsing totally separate from things like this.

I actually considered deprecating/removing most of these methods, because the consumers can just replicate the functionality themselves. What do you think?

I ended up removing parse_str, parse_file, and parse_path since consumers can easily replicate that logic themselves using SourceMap directly. I kept parse and parse_dir since they have external callers and the directory walking logic is non-trivial to replicate.

The remaining stringification in those two methods is a consequence of mixing IO and parse errors.

alexcrichton · 2026-03-18T05:28:42Z

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

Mind adding some brief comments to public types/enums/structs in this file as well as the module itself?

To bikeshed a bit, I find the naming here a bit too wordy. WDYT about something like this:

PackageParseErrors => ParseErrors.

(future) FileParseErrors => FileErrors

(new) type ParseResult<T, E = ParseErrors> = Result<T, E>

(new) type LexResult<T, E = lex::Error> = Result<T, E>

That to me keeps the same flexibility for package/file distinctions insofar as there's different classes of errors, but I think it's useful for error ergonomics to not really get in the way when reading/writing code. To that extent I'm worried how verbose the error handling here is, so I'm hoping some renamings/idioms can help reduce the noise without adding too much complexity.

Perfect. I'll do that.

I renamed PackageParseErrors => ParseError (singular, since we decided not to return a list of errors), and also introduced a type alias for the result.

I didn't touch the lexer, since it's unrelated to this PR. I can do that in a FLUP.

PhoebeSzmucer added 5 commits March 13, 2026 10:09

[wit-parser] Migrate to structured errors in the AST/package parser

f3c9fe2

Update snapshots

0a58d5d

Derive Eq and PartialEq

5164020

Remove toposort::Error

1d20ddc

Remove useless comments

e2c6c84

PhoebeSzmucer commented Mar 13, 2026

View reviewed changes

crates/wit-parser/src/ast/toposort.rs Show resolved Hide resolved

PhoebeSzmucer commented Mar 13, 2026

View reviewed changes

crates/wit-parser/src/ast/error.rs Outdated Show resolved Hide resolved

PhoebeSzmucer commented Mar 13, 2026

View reviewed changes

crates/wit-parser/tests/ui/parse-fail/bad-function.wit.result Show resolved Hide resolved

PhoebeSzmucer marked this pull request as ready for review March 13, 2026 10:36

PhoebeSzmucer requested a review from a team as a code owner March 13, 2026 10:36

PhoebeSzmucer requested review from alexcrichton and removed request for a team March 13, 2026 10:36

PhoebeSzmucer commented Mar 13, 2026

View reviewed changes

crates/wit-parser/src/ast/error.rs Outdated Show resolved Hide resolved

PhoebeSzmucer mentioned this pull request Mar 13, 2026

[WIP/do not merge] Replace anyhow with typed errors in wit-parser #2460

Draft

PhoebeSzmucer added 2 commits March 13, 2026 16:17

Make highlight_span pub(crate) again

0c713ea

Expose ast/parsing errors

4cdf617

alexcrichton reviewed Mar 18, 2026

View reviewed changes

PhoebeSzmucer added 8 commits March 23, 2026 13:00

Introduce new_syntax helper

61619f6

PackageParseErrorKind -> PackageParseErrors

430f706

Introduce ParseResult

c685bad

Rename PackageParse -> Parse

3ab3261

Comments

7211ee3

Use singular ParseError

09c1a90

Remove some methods

ec5d885

Merge branch 'main' into structured-parsing-errors

f00e850

Conversation

PhoebeSzmucer commented Mar 13, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alexcrichton commented Mar 16, 2026

Uh oh!

PhoebeSzmucer commented Mar 17, 2026

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

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!

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