Rust: Removing FatalErrors from StringReader

[matklad]

StringReader uses a bunch of Fatal (panicking) errors:

https://github.com/rust-lang/rust/blob/1dc748fb3d2c54f536e6abd74f1ad34b3624f640/src/librustc_parse/lexer/mod.rs#L30

(search for .raise)

All of these unwinding errors should be replaced with "normal" error reporting. But we probably need to make sure that we won't cause cascading errors.

[matklad]

@estebank is there some existing best practices for doing FatalErrors without unwinding?

[LeSeulArtichaut]

I’d like to try to work on this

[LeSeulArtichaut]

I tried to remove a few calls in #76133. As expected this comes with a few additional errors, so I just blessed the UI tests if someone wants to take a look.

[matklad]

Yeah, I think we should avoid cascading diagnostics here. If there's an unpaired ", we shouldn't really report any diagnostics in this file (it might or might not b fine to emit diagnostics in other files).

My gut feeling is that Handler::emit_error should set a flag if a Fatal diagnostic is ever emitted, and drop all subsequent diagnostics on the floor. I don't think we should stop all processing after a fatal error though -- for an IDE use case, you want to have a syntax tree & type-infernece informed compltion even with broken string literals, you just don't want to spam the user with extra errors.

[LeSeulArtichaut]

My gut feeling is that Handler::emit_error should set a flag if a Fatal diagnostic is ever emitted, and drop all subsequent diagnostics on the floor.

I tried that as an experiment then ran the UI tests:

test result: FAILED. 6337 passed; 4367 failed; 76 ignored; 0 measured; 0 filtered out

I haven't looked in detail in the failures, but 4367 seems a lot to me.

EDIT: pushed blessed tests here

[LeSeulArtichaut]

My gut feeling is that Handler::emit_error should set a flag if a Fatal diagnostic is ever emitted, and drop all subsequent diagnostics on the floor.

@matklad here would be the results of such a change. It changes 10 UI files (sounds more reasonable than 4367 🙄). I've skimmed through the diff and find it is an improvement overall, I'd like to know what you think. Also maybe cc @estebank

[Mark-Simulacrum]

Hm, can you clarify what caused the 4367 failures? Was there a bug in the implementation?

[LeSeulArtichaut]

Yes, it was a silly mistake on my part. I simply forgot to handle the failure-note properly, which meant that when rustc was aborting, it didn't emit the "For more information about this error" messages. Hopefully it was easy to realize just by looking at the diff

[estebank]

is there some existing best practices for doing FatalErrors without unwinding?

Not that I am aware of, the mitigation has been to agressivly remove the FatalError usages as much as possible, but the for the lexer these were deemed "_acceptable uses_".

My gut feeling is that Handler::emit_error should set a flag if a Fatal diagnostic is ever emitted, and drop all subsequent diagnostics on the floor.

here would be the results of such a change. It changes 10 UI files (sounds more reasonable than 4367 🙄). I've skimmed through the diff and find it is an improvement overall, I'd like to know what you think.

That approach sounds reasonable at face value, and the ui file changes are few enough that we could potentially move forward, but 1) I don't agree that the diff is necessarily an improvement and 2) there still seems to be a small bug still where the "Some errors have detailed explanations" message is being hidden.

[LeSeulArtichaut]

2) there still seems to be a small bug still where the "Some errors have detailed explanations" message is being hidden.

There are still a few bugs, I also noticed that ICEs don’t emit all the notes that say "please file an issue" etc... I haven’t fixed them yet because it was only an experiment, but if you want me to dig into it, I’m up, though I’ll obviously need guidance.