[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).

PhoebeSzmucer · 2026-03-13T10:27:47Z

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

 }

-#[derive(Debug)]
-pub enum Error {


Thsi went away now in favor of just using PackageParseError

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

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

+
+#[non_exhaustive]
+#[derive(Debug, PartialEq, Eq)]
+pub enum PackageParseErrorKind {


Note - I named it "package parse", because in the future we might want to expose a dedicated error for the "file parse" stage (a single WIT package can consist of many files).

Parsing is also usually thought of as converting a single file into an AST, so I think mentioning that this is about parsing a whole package makes things a bit clearer.

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

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.

PhoebeSzmucer · 2026-03-13T10:35:58Z

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

@@ -1,4 +1,4 @@
-name `nonexistent` is not defined
+name `nonexistent` does not exist


Previously we were doing a mix of "does not exist" and "is not defined". I'm going to standarize on "does not exist".

Once the structured error migration is done we might want to audit all error messages and see if anything could be better (now that we can track rich metadata easily).

PhoebeSzmucer · 2026-03-13T10:38:15Z

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

+}
+
+#[derive(Debug, PartialEq, Eq)]
+pub struct PackageParseErrors(Box<PackageParseErrorKind>);


Happy to change this to a struct with #[repr(transparent)] if that's better (is there a practical difference between them?)

Nah #[repr(transparent)] is kind of a niche concern and isn't required, it's fine to omit.

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

alexcrichton · 2026-03-18T05:18:46Z

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

+}
+
+#[derive(Debug, PartialEq, Eq)]
+pub struct PackageParseErrors(Box<PackageParseErrorKind>);


Nah #[repr(transparent)] is kind of a niche concern and isn't required, it's fine to omit.

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.

alexcrichton · 2026-03-18T05:23:16Z

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

    kind: &str,
    deps: &IndexMap<&'a str, Vec<Id<'a>>>,
-) -> Result<Vec<&'a str>, Error> {
+) -> Result<Vec<&'a str>, PackageParseErrorKind> {


s/PackageParseErrorKind/PackageParseErrors/

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?

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.

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

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

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

		@@ -1,4 +1,4 @@
		name `nonexistent` is not defined
		name `nonexistent` does not exist

Conversation

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

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!

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