-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Improve error handling in libsyntax #29285
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
1dd87dc
Don't use panicking helpers in Parser.
eefriedman c141f47
Delete unnecessary ParserAttr trait.
eefriedman de95857
Don't panic for fatal errors in attribute parsing.
eefriedman 329e487
Start pushing panics outward in lexer.
eefriedman e502492
Make fatal errors more consistent.
eefriedman 56ba8fe
Update libsyntax tests.
eefriedman e7d3ae6
Make quote plugin use parsing functions which explicitly panic.
eefriedman File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -206,13 +206,9 @@ impl Handler { | |
can_emit_warnings: can_emit_warnings | ||
} | ||
} | ||
pub fn fatal(&self, msg: &str) -> ! { | ||
pub fn fatal(&self, msg: &str) -> FatalError { | ||
self.emit.borrow_mut().emit(None, msg, None, Fatal); | ||
|
||
// Suppress the fatal error message from the panic below as we've | ||
// already terminated in our own "legitimate" fashion. | ||
io::set_panic(Box::new(io::sink())); | ||
panic!(FatalError); | ||
FatalError | ||
} | ||
pub fn err(&self, msg: &str) { | ||
self.emit.borrow_mut().emit(None, msg, None, Error); | ||
|
@@ -230,14 +226,15 @@ impl Handler { | |
pub fn abort_if_errors(&self) { | ||
let s; | ||
match self.err_count.get() { | ||
0 => return, | ||
1 => s = "aborting due to previous error".to_string(), | ||
_ => { | ||
s = format!("aborting due to {} previous errors", | ||
self.err_count.get()); | ||
} | ||
0 => return, | ||
1 => s = "aborting due to previous error".to_string(), | ||
_ => { | ||
s = format!("aborting due to {} previous errors", | ||
self.err_count.get()); | ||
} | ||
} | ||
self.fatal(&s[..]); | ||
|
||
panic!(self.fatal(&s[..])); | ||
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. could be &s instead of &s[..] probably |
||
} | ||
pub fn warn(&self, msg: &str) { | ||
self.emit.borrow_mut().emit(None, msg, None, Warning); | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
Do we need to panic in these places? We're already declaring a fatal exception, what happens if we just carry on? Don't we exit any way?
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.
Oh, you're changing fatal errors to not panic. That seems maybe not a good idea? Could you use non-fatal errors in libsyntax instead? Or maybe it is a good idea since it makes the panicking more explicit.
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, on master, we're in the slightly odd situation where
fatal()
panics, butspan_fatal()
doesn't panic; it probably doesn't make sense for it to stay that way.In terms of how this should work long-term... libsyntax has basically settled on a "don't panic on fatal errors" approach, so it doesn't really make sense for libsyntax to provide a panicking
fatal()
method. On the other hand, libsyntax also provides the canonical implementation of fatal errors... so maybe fatal errors should panic, but libsyntax just shouldn't use thefatal()
functions.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.
OK, this sounds reasonable, thanks for the explanantion