Improve error handling in libsyntax #29285
Conversation
|
r? @nrc (rust_highfive has picked a reviewer for you, use r? to override) |
eefriedman
force-pushed
the
libsyntax-panic
branch
2 times, most recently
from
e8fe2ac to
113ee6f
Compare
| handler.fatal(&format!("{}: {}", | ||
| &msg[..], | ||
| &err[..])); | ||
| panic!(handler.fatal(&format!("{}: {}", &msg[..], &err[..]))); |
There was a problem hiding this comment.
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.
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.
Well, on master, we're in the slightly odd situation where fatal() panics, but span_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 the fatal() functions.
There was a problem hiding this comment.
OK, this sounds reasonable, thanks for the explanantion
|
r+, but has some test fails which need fixing. |
eefriedman
force-pushed
the
libsyntax-panic
branch
from
113ee6f to
77f186a
Compare
|
@nrc Updated; 77f186a fixes the test failure. |
|
@eefriedman still some tests failing |
Rename parse_* to parse_*_panic, and add parse_attribute_panic.
eefriedman
force-pushed
the
libsyntax-panic
branch
from
77f186a to
e7d3ae6
Compare
|
Tests fixed in e7d3ae6. |
|
@nrc Ping. |
|
@bors: r+ (sorry @eefriedman I've been travelling) |
|
📌 Commit e7d3ae6 has been approved by |
A set of commits which pushes some panics out of core parser methods, and into users of those parser methods.
rust-lang/rust#29582 renamed `TtToken` to `Token`. rust-lang/rust#29285 renamed `parse_expr` to `parse_expr_panic`.
A set of commits which pushes some panics out of core parser methods, and into users of those parser methods.