-
Notifications
You must be signed in to change notification settings - Fork 322
[wit-parser] Migrate to structured errors in the AST/package parser #2465
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f3c9fe2
0a58d5d
5164020
1d20ddc
e2c6c84
0c713ea
4cdf617
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| use alloc::boxed::Box; | ||
| use alloc::string::{String, ToString}; | ||
| use core::fmt; | ||
|
|
||
| use crate::{SourceMap, Span, ast::lex}; | ||
|
|
||
| #[non_exhaustive] | ||
| #[derive(Debug, PartialEq, Eq)] | ||
| pub enum PackageParseErrorKind { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| /// Lexer error (invalid character, unterminated comment, etc.) | ||
| Lex(lex::Error), | ||
| /// Syntactic or semantic error within a single package (duplicate name, | ||
| /// invalid attribute, etc.) | ||
| Syntax { span: Span, message: String }, | ||
| /// A type/interface/world references a name that does not exist within | ||
| /// the same package. | ||
| ItemNotFound { | ||
| span: Span, | ||
| name: String, | ||
| kind: String, | ||
| hint: Option<String>, | ||
| }, | ||
| /// A type/interface/world depends on itself. | ||
| TypeCycle { | ||
| span: Span, | ||
| name: String, | ||
| kind: String, | ||
| }, | ||
| } | ||
|
|
||
| impl PackageParseErrorKind { | ||
| pub fn span(&self) -> Span { | ||
| match self { | ||
| PackageParseErrorKind::Lex(e) => Span::new(e.position(), e.position() + 1), | ||
| PackageParseErrorKind::Syntax { span, .. } | ||
| | PackageParseErrorKind::ItemNotFound { span, .. } | ||
| | PackageParseErrorKind::TypeCycle { span, .. } => *span, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for PackageParseErrorKind { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| match self { | ||
| PackageParseErrorKind::Lex(e) => fmt::Display::fmt(e, f), | ||
| PackageParseErrorKind::Syntax { message, .. } => message.fmt(f), | ||
| PackageParseErrorKind::ItemNotFound { | ||
| kind, name, hint, .. | ||
| } => { | ||
| write!(f, "{kind} `{name}` does not exist")?; | ||
| if let Some(hint) = hint { | ||
| write!(f, "\n{hint}")?; | ||
| } | ||
| Ok(()) | ||
| } | ||
| PackageParseErrorKind::TypeCycle { kind, name, .. } => { | ||
| write!(f, "{kind} `{name}` depends on itself") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, PartialEq, Eq)] | ||
| pub struct PackageParseErrors(Box<PackageParseErrorKind>); | ||
PhoebeSzmucer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| impl PackageParseErrors { | ||
| pub fn kind(&self) -> &PackageParseErrorKind { | ||
| &self.0 | ||
| } | ||
|
|
||
| /// Format this error with source context (file:line:col + snippet) | ||
| pub fn highlight(&self, source_map: &SourceMap) -> String { | ||
| let e = self.kind(); | ||
| source_map | ||
| .highlight_span(e.span(), e) | ||
| .unwrap_or_else(|| e.to_string()) | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for PackageParseErrors { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| fmt::Display::fmt(self.kind(), f) | ||
| } | ||
| } | ||
|
|
||
| impl core::error::Error for PackageParseErrors {} | ||
|
|
||
| impl From<PackageParseErrorKind> for PackageParseErrors { | ||
| fn from(kind: PackageParseErrorKind) -> Self { | ||
| PackageParseErrors(Box::new(kind)) | ||
| } | ||
| } | ||
|
|
||
| impl From<lex::Error> for PackageParseErrors { | ||
| fn from(e: lex::Error) -> Self { | ||
| PackageParseErrorKind::Lex(e).into() | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.FileParseErrors=>FileErrorstype ParseResult<T, E = ParseErrors> = Result<T, E>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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect. I'll do that.