- Notifications
You must be signed in to change notification settings - Fork1.4k
Pass more information in user defined parse error#1106
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
compiler/src/error.rs Outdated
| // Print line number: | ||
| write!(f," at line {:?}",self.location.row()) | ||
| match&self.error{ | ||
| CompileErrorType::Parse(..) =>Ok(()), |
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.
Hmm. This exception for the parse error is not ideal, but we can improve on this later.
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.
Please implement the todo at this line:
RustPython/compiler/src/error.rs
Line 17 inef1d543
| location:Default::default(),// TODO: extract location from parse error! |
Then we have proper locations on all compiler errors (where possible).
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.
Done
parser/src/error.rs Outdated
| UnrecognizedToken(TokSpan,Vec<String>), | ||
| /// Maps to `User` type from `lalrpop-util` | ||
| Other, | ||
| Other(LexicalError), |
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.
It might be good to rename this error variant intoLexical(LexicalError).
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.
Done
parser/src/error.rs Outdated
| InnerError::ExtraToken{ token} =>ParseError::ExtraToken(token), | ||
| // Inner field is a unit-like enum `LexicalError::StringError` with no useful info | ||
| InnerError::User{ ..} =>ParseError::Other, | ||
| InnerError::User{ error} =>ParseError::Other(error), |
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.
This is very good to propagate the lexer errors!
parser/src/lexer.rs Outdated
| } | ||
| LexicalErrorType::OtherError(ref msg) =>write!(f,"{}", msg), | ||
| }?; | ||
| write!( |
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.
I would prefer to add the line and column info later on, so in theDisplay method of theCompilerError struct.
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.
Done
windelbouwman commentedJul 6, 2019
This change looks very good now. The only thing left is to take a look at the wasm build. |
palaviv commentedJul 6, 2019
I am trying to fix the wasm but the error currently look like: SyntaxError:ErrorparsingPythoncode:Gotunexpectedtoken:DoubleStaratLocation {row:1,column:8 }atline0 Not sure why the |
| ); | ||
| } | ||
| } | ||
| let _ = |
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.
Well, adding thelocation attribute clearly was beneficial!
Before the message was:
And now it is: