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

[PhoebeSzmucer]

Motivation

Today, getting a file path and line number out of a parse error requires parsing the human-readable error string that anyhow produces. This is fragile: it breaks silently when error message formatting changes, and it's simply the wrong abstraction.

I want to use wit-parser in an LSP for WIT/WAC, but this change would be good for any tool that embeds wit-parser and wants to do something useful with errors.

I Know This Is Large

I'm aware this PR is bigger than ideal. The core reason it's large is that error type changes propagate through the entire call stack - there's no natural seam to split at. If this is something that we'd like to merge in some form, I can try splitting up this PR into smaller chunks - I wanted to open this draft first to start a conversation.

Global vs local spans

Currently I'm returning global Span objects as part of WitError, because existing code uses global spans almost exclusively. This makes things annoying for the consumer - they have to use the SourceMap object to map to file-local offsets. I'm considering changing the exposed spans to be resolved local spans.

[alexcrichton]

Thanks for this! I mentioned this on Zulip but wanted to echo here too -- I agree with the direction of this change and I think it would be good to have concrete error types for wit-parser. Implementation-wise, some thoughts from me are:

• If you're willing I think it'd be good to split this up and have pieces land incrementally. For example the changes to LexError and avoiding using anyhow in lex.rs is relatively separable. The end-goal could be to remove the anyhow dependency from this crate, but that wouldn't have to necessarily happen in one go (e.g. sections of APIs could be updated at-a-time).
• One benefit of anyhow::Error is its performance -- it's a pointer-large an non-nullable meaning that it can often fit into various niches (e.g. anyhow::Result<()> is a pointer large). While wit-parser isn't the most performance-sensitive of things I'd still be slightly worried about inflating the error sizes drastically (e.g. to dozens-of-pointers) which is unfortunately easy once there's lots of context added. This is solvable but would remove the pub enum WitError for example and would switch that to pub struct WitError with some sort of accessor like pub enum WitErrorKind(or something like that). We don't have any benchmarks forwit-parser` right now, though, so measuring the impact here is unfortunately not easy.
• Out of curiosity, do you need fine-grained rationale/context for all errors? Or are you mostly interested in just a Span for all errors? (or some sort of error-location) If it's just the latter I think we might be able to move in the direction of a concrete error without needing too much context information, but if the former that's a different possible design here.
• For Span-vs-rendered-string, personally I'd prefer to stick to using Span since that's what the rest of the API has, but you mention that it's unwieldy to turn that into a rendered location string. Can you elaborate more on that and how you feel that having a rendered string in the error is more appropriate?

Overall I'd say let's figure out the final design of the error types in play here, and once that's sorted out figure out how best to land this (e.g. all-at-once vs incrementally) -- does that sound ok?

[PhoebeSzmucer]

Thank you for a very detailed response @alexcrichton !

If you're willing I think it'd be good to split this up and have pieces land incrementally. For example the changes to LexError and avoiding using anyhow in lex.rs is relatively separable. The end-goal could be to remove the anyhow dependency from this crate, but that wouldn't have to necessarily happen in one go (e.g. sections of APIs could be updated at-a-time).

Totally agreed. I'll try to merge some non-controversial pieces first.

One benefit of anyhow::Error is its performance -- it's a pointer-large an non-nullable meaning that it can often fit into various niches (e.g. anyhow::Result<()> is a pointer large). While wit-parser isn't the most performance-sensitive of things I'd still be slightly worried about inflating the error sizes drastically (e.g. to dozens-of-pointers) which is unfortunately easy once there's lots of context added. This is solvable but would remove the pub enum WitError for example and would switch that to pub struct WitError with some sort of accessor like pub enum WitErrorKind(or something like that). We don't have any benchmarks forwit-parser` right now, though, so measuring the impact here is unfortunately not easy.

To clarify - we'd just make WitError hold a box to WitErrorKind enum, which would then contain richer metadata (if needed)? ie just store error metadata on a heap instead. Or did you have something else in mind?

Out of curiosity, do you need fine-grained rationale/context for all errors? Or are you mostly interested in just a Span for all errors? (or some sort of error-location) If it's just the latter I think we might be able to move in the direction of a concrete error without needing too much context information, but if the former that's a different possible design here.

For a very basic LSP we don't need it, but I think it would be a good idea to be more fine-grained than just span+message. It's the difference between the consumers (the LSP, or even the CLI) knowing just what to highlight and what to say, vs these tools actually understanding what the error is.

Typed variants unlock richer things - DuplicatePackage naturally maps to multiple spans, and similarly PackageCycle can expose the cycle chain. In the future we can carry suggestions for fixes or other notes, and that would integrate really well with something like Ariadne (which would give us very informative rustc-like errors).

For Span-vs-rendered-string, personally I'd prefer to stick to using Span since that's what the rest of the API has, but you mention that it's unwieldy to turn that into a rendered location string. Can you elaborate more on that and how you feel that having a rendered string in the error is more appropriate?

I agree that the Span is the right internal primitive, but it raises the question for the public API - the consumer needs to know which source map a given Span is valid in.

• for SourceMap::parse() errors it's the map we just parsed
• for push_groups() errors it's resolve.source_map

This is manageable but leaks internal architecture to callers. In case of DuplicatePackage and PackageCycle I took a shortcut and just pre-rendered the messages because geting the right source map wasn't straightforward.

An alternative is for errors to carry already-resolved locations. Not pre-rendered strings, but structured (file_path, start_byte, end_byte) tuples resolved at the point where the source map is available (note - the byte offsets here would relative to the files themselves). The consumer then has everything they need without touching a source map, and can convert that easily to line/col.

I'm happy to keep exposing global spans in the API, but for that I might need to modify how package cycles are computed - from my understanding cycles are detected during toposort before source maps are merged, which is why I did eager resolution in my PR. I think if we merge the source maps before toposort that would work.

[alexcrichton]

How about a design like this:

#[derive(Debug)]
#[repr(transparent)]
pub struct WitError {
    inner: Box<WitErrorInner>,
}

struct WitErrorInner {
    kind: WitErrorKind,
    location: Option<WitLocation>,
}

#[non_exhaustive]
pub enum WitErrorKind {
    // ... full details of `WitError` as today, slightly modified maybe ...
}

pub struct WitLocation {
    file: String,
    line: u32,
    col: u32,
}

impl WitError {
    pub fn location(&self) -> Option<&WitLocation> {
        // ...
    }

    pub fn span(&self) -> Option<Span> {
        // ...
    }

    pub fn kind(&self) -> &WitErrorKind {
        // ...
    }

    /// Called by `rewrite_error` throughout the codebase
    pub(crate) fn update_location(&mut self, map: &SourceMap) {
        // ... update `self.location` ...
    }

    /// Convenience constructors for throughout this crate.
    pub(crate) fn duplicate_package(..) -> WitError {
        // ...
    }
}

impl fmt::Display for WitError {
    // ..
}

impl core::error::Error for WitError {
    // ..
}

Here WitError has the one-word-only representation can can optionally contain all sorts of rich information through Kind/Location/etc. That should handle the performance concerns of mine plus your desire to have as fine-grained information as possible thorugh the Kind accessor. Additionally by having both a Span an an optionally-rendered location I think that should also handle the question of "where is this Span relative to?"

One final thing I might bikeshed as well is that the current ad-hoc growth of the crate has a few error enums throughout (e.g. toposort, package name stuff, lex errors, etc), and I think it would be best to unify everything into a single large WitError. I don't think there's much value in having lots of little errors all over the place and I personally feel like it's best for error-per-library style design. The tradeoff is a little different when using anyhow::Error since the errors today are about being able to programmatically test for specific errors, but that can be done with WitError through the kind accessor as well.

How's that sound?

[PhoebeSzmucer]

I just pushed another version that we can consider going with - it compiles (modulo snapshots). I pushed the code just now.

Essentially:

Parse/AST phase (src/ast/error.rs):

pub struct ParseErrors(Box<[ParseErrorKind]>); // Maintains the possibility of us returning more errors in the future

#[non_exhaustive]
pub enum ParseErrorKind {
  Lex(lex::Error),
  Syntax { span: Span, message: String },
  ItemNotFound { span: Span, name: String, kind: String, hint: Option<String> },
  TypeCycle { span: Span, name: String, kind: String },
}

Resolve phase (resolve/error.rs)

pub struct ResolveErrors(Box<[ResolveErrorKind]>); // Maintains the possibility of us returning more errors in the future

#[non_exhaustive]
pub enum ResolveErrorKind {
  PackageNotFound { span: Span, requested: PackageName, known: Vec<PackageName> },
  InvalidTransitiveDependency { span: Span, name: String },
  DuplicatePackage { name: PackageName, span1: Span, span2: Span },
  PackageCycle { package: PackageName, span: Span },
  Semantic { span: Span, message: String }, // Sort of a "catch all" - we can explode this into more variants later
}

All of the variants contain one or more global spans. ParseErrors will use the source map that the file was parsed with, and ResolveErrorKind will use the source map that we're accumulating into (I have this working locally).

This design is a bit different from your latest proposed one. There are a couple of decisions that we should pick:

• Global Span vs pre-resolved WitLocation - I kept raw Spans rather than pre-resolving to line/col. Reasons:
  • This is much simpler to ship because the entire crate operates on those global spans
  • Some errors have multiple spans (cycle/duplicate package)
  • In practice we can make it pretty obvious which source map should be used for span resolution
• Parse/resolve split vs unified error - I previously had just WitError but given there are two separate stages here, I decided to split into two. Happy to revert that to a unified error though.
• Box<[XXX]> vs Box<XXX> - I used a slice to leave the door open for error recovery returning multiple errors without a signature change, but I realize this is very forward looking (as currently we always return a single error). It would result in a nicer LSP experience and I think we could relatively easily start returning more errors, but I'm more than happy to just return Box<XXX> for now.

Thanks for iterating with this on me - once we agree on these questions I'll update accordingly

[PhoebeSzmucer]

One final thing I might bikeshed as well is that the current ad-hoc growth of the crate has a few error enums throughout (e.g. toposort, package name stuff, lex errors, etc), and I think it would be best to unify everything into a single large WitError. I don't think there's much value in having lots of little errors all over the place and I personally feel like it's best for error-per-library style design. The tradeoff is a little different when using anyhow::Error since the errors today are about being able to programmatically test for specific errors, but that can be done with WitError through the kind accessor as well.

And yeah definitely. I agree and I'm happy to do that.

[alexcrichton]

Personally I think I'd still lean towards one error type for the crate rather than multiple error types, but do you think for the LSP purposes it'd be useful to have multiple kinds of errors?

For many-errors-vs-one, I like the idea of being able to report many errors when parsing WIT as opposed to stopping on the first one, but I suspect that fully supporting this is going to require some deeper refactoring. For example idiomatically throughout the code errors are propagated with ? but that doesn't quite work with multiple errors. Additionally one thing I'd be worried about is cascading too many errors -- for example if an import was typo'd then all references to that import would also generate an error saying "name not found" which could have a giant wall of errors with the only relevant one at the top.

I suppose another way to put this is that I don't at least personally have all that much experience in a multi-error-returning system. I'd hope to reach some sort of middle-ground where it reports reasonable errors, tries to not have a billion cascading errors, but doesn't baloon infrastructure to precisely track cascading errors just yet. My rough assumption is that doing all this will be more involved than adding a slice to the error types themselves, so if you're ok with it I'd prefer to separate that out to a future change perhaps?

[PhoebeSzmucer]

but I suspect that fully supporting this is going to require some deeper refactoring

Agreed.

for example if an import was typo'd then all references to that import would also generate an error saying "name not found" which could have a giant wall of errors with the only relevant one at the top.

Agreed for a CLI, but for the LSP we would probably want that - otherwise it's pretty random which file gets the resolution diagnostic. But I guess that shows that the compiler and the LSP are pretty different, and we will need to put some more thought into how wit-parser should be optimally structured to support both.

I will go with a single error for now, and maybe in the future we can revisit if we decide returning a list is better. Are we ok occasionally making breaking compile-time changes in this package?

Personally I think I'd still lean towards one error type for the crate rather than multiple error types, but do you think for the LSP purposes it'd be useful to have multiple kinds of errors?

Yes, I think the split is useful for LSP purposes specifically because parse and resolve can then be separate salsa queries in the LSP. Per-file parsing can cached independently from workspace-level resolution. Having distinct types makes it natural to handle diagnostics at the right granularity - parse errors belong to a single file, resolve errors belong to the workspace. With a unified type you lose that distinction at the type level, which might force us to do unreachable! in some places.

That said, I'm happy to go with a single type if you feel strongly - it's easy to split later if we decide to (again, if we can afford to make more breaking changes).

[alexcrichton]

I will go with a single error for now, and maybe in the future we can revisit if we decide returning a list is better. Are we ok occasionally making breaking compile-time changes in this package?

Ok sounds good to me, and yeah breaking API changes here are fine. We're still in a mode of "each release is a new major version" so it's fine to have futher breaking changes later (it's already a breaking change to switch away from anyhow)

Yes, I think the split is useful for LSP purposes specifically because parse and resolve can then be separate salsa queries in the LSP.

What you say is convincing to me, so sounds good!

---

For landing this, given that there's going to be two high-level error types -- parsing & resolving -- how about landing those separately? For example the first error to land would be the parsing result and then afterwards could be the resolve result. The error types would look similar in their implementation, and I think that's fine.