Fix panic when compiling Rocket.
#121427
Fix panic when compiling Rocket.
#121427
Conversation
`Rustc::emit_diagnostic` reconstructs a diagnostic passed in from the macro machinery. Currently it uses the type `DiagnosticBuilder<'_, ErrorGuaranteed>`, which is incorrect, because the diagnostic might be a warning. And if it is a warning, because of the `ErrorGuaranteed` we end up calling into `emit_producing_error_guaranteed` and the assertion within that function (correctly) fails because the level is not an error level. The fix is simple: change the type to `DiagnosticBuilder<'_, ()>`. Using `()` works no matter what the diagnostic level is, and we don't need an `ErrorGuaranteed` here. The panic was reported in rust-lang#120576.
PR rust-lang#119097 made the decision to make all `IntoDiagnostic` impls generic, because this allowed a bunch of nice cleanups. But four hand-written impls were unintentionally overlooked. This commit makes them generic.
rustbot
added
S-waiting-on-review
|
I was able to reproduce the panic when building Rocket like so: Unfortunately, I wasn't able to test the fix. I tried using I also haven't written a test, because I don't understand the proc macro machinery enough to do so. So I have only tested this fix in my head :) But I have high confidence that it will work, because the panic message and the stack trace make it very clear what the problem is, conceptually. |
| @@ -510,7 +510,7 @@ impl server::FreeFunctions for Rustc<'_, '_> { | |||
|
|
|||
| fn emit_diagnostic(&mut self, diagnostic: Diagnostic<Self::Span>) { | |||
| let message = rustc_errors::DiagnosticMessage::from(diagnostic.message); | |||
| let mut diag: DiagnosticBuilder<'_, rustc_errors::ErrorGuaranteed> = | |||
| let mut diag: DiagnosticBuilder<'_, ()> = | |||
There was a problem hiding this comment.
I see why this fixed the issue, but why are the other changes necessary?
There was a problem hiding this comment.
You mean in the second commit? While I was looking at this I did an audit of all the DiagnosticBuilder::new calls to make sure there weren't any other problems. That's when I noticed the non-generic IntoDiagnostic impls, and since they are kind of related I decided to do them in the same PR (albeit in a different commit).
There was a problem hiding this comment.
Some of them are explicitly setting error codes. It doesn't really make sense to allow them to be anything but an error. I guess that's a limitation of the IntoDiagnostic API
I don't think they should be made generic if they are always meant to be errors
There was a problem hiding this comment.
#119097 had a lot of stuff about this, see this comment for a summary. In short, all derived IntoDiagnostic impls have to be generic. For consistency, we decided that all hand-written ones should be too (plus it allowed some important related simplifications). That way, for all diagnostics, its the emission site that dictates the level. But four of them were overlooked.
There was a problem hiding this comment.
Right, we should probably create a rustc lint for that then
There was a problem hiding this comment.
Maybe the IntoDiagnostic doc comment should explain that impls should always be generic, and why?
There was a problem hiding this comment.
I've added that to my todo list, I'll get to it next week.
|
@bora r+ |
bors
added
S-waiting-on-bors
Fix panic when compiling `Rocket`. This panic was reported [here](rust-lang#120576 (comment)). r? `@oli-obk`
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#120598 (No need to `validate_alias_bound_self_from_param_env` in `assemble_alias_bound_candidates`) - rust-lang#121386 (test that we do not support higher-ranked regions in opaque type inference) - rust-lang#121393 (match lowering: Introduce a `TestCase` enum to replace most matching on `PatKind`) - rust-lang#121401 (Fix typo in serialized.rs) - rust-lang#121427 (Fix panic when compiling `Rocket`.) - rust-lang#121439 (Fix typo in metadata.rs doc comment) - rust-lang#121441 (`DefId` to `LocalDefId`) - rust-lang#121452 (Add new maintainers to nto-qnx.md) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#121427 - nnethercote:fix-Rocket, r=oli-obk Fix panic when compiling `Rocket`. This panic was reported [here](rust-lang#120576 (comment)). r? ``@oli-obk``
|
@SergioBenitez: Hopefully this fixes the problem and Rocket's build with Nightly will start working again some time in the next 24-48 hours, once this PR gets integrated! Please let us know if it works or doesn't work. |
This panic was reported here.
r? @oli-obk