Structured resolver errors#2482
Structured resolver errors#2482PhoebeSzmucer wants to merge 19 commits intobytecodealliance:mainfrom
Conversation
This reverts commit 0692157.
|
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 |
|
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. |
| @@ -1,11 +1,15 @@ | |||
| //! Error types for WIT parsing. | |||
There was a problem hiding this comment.
Added some missing docs
| Resolve::default() | ||
| } | ||
|
|
||
| /// Parse WIT packages from the input `path`. |
There was a problem hiding this comment.
This doc was very out of date.
| return Err(arbitrary::Error::IncorrectFormat); | ||
| } | ||
| let err = e.to_string(); | ||
| if err.contains("shadows previously") || err.contains("mismatch in stability") { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Although wit-smith is just a fuzzer so idk if it's worth it.
| /// Merge `main` and `deps` into this [`Resolve`], topologically sorting | ||
| /// them internally. Returns the [`PackageId`] of `main` and a | ||
| /// [`PackageSources`] covering all groups. | ||
| fn sort_unresolved_packages( |
There was a problem hiding this comment.
Note - this function was restructured to do upfront merging so that if a DuplicatePackage or PackageCycle error happens, we already have valid spans in the error
There was a problem hiding this comment.
I added some tests for this
| /// The returned [`PackageId`] corresponds to `main`. | ||
| /// | ||
| /// On error, `self` may be partially modified and should not be reused. | ||
| pub fn push_groups( |
There was a problem hiding this comment.
This function is something I will want to use in the WIT LSP. I was going to add it in a separate PR, but having this function now makes it much easier to test the behavior of sort_unresolved_packages.
| ); | ||
| } | ||
|
|
||
| assert!( |
There was a problem hiding this comment.
Note we went from bail! to assert!, but this is a programmer error (pushing the same package twice), not a WIT source error. So I think panic is more appropriate here.
|
@alexcrichton this is ready for a review now. I left some inline comments on the PR |
2 participants
Part of #2460 (part 1 was here).
This PR replaces most of
anyhowusages in the resolver with structuredParserErrortype, which carries structured error information (with spans).