Skip to content
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.

Sign up for GitHub

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

Fix panic when compiling Rocket. #121427

Merged
merged 2 commits into from
Feb 22, 2024
Merged

Fix panic when compiling Rocket. #121427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
326b44e
Fix panic when compiling `Rocket`.
nnethercote Feb 22, 2024
02423a5
Make some `IntoDiagnostic` impls generic.
nnethercote Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_expand/src/proc_macro_server.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<'_, ()> =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see why this fixed the issue, but why are the other changes necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@nnethercote nnethercote Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#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.

Copy link
Contributor

@oli-obk oli-obk Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, we should probably create a rustc lint for that then

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the IntoDiagnostic doc comment should explain that impls should always be generic, and why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added that to my todo list, I'll get to it next week.

DiagnosticBuilder::new(&self.sess().dcx, diagnostic.level.to_internal(), message);
diag.span(MultiSpan::from_spans(diagnostic.spans));
for child in diagnostic.children {
Expand Down
6 changes: 4 additions & 2 deletions compiler/rustc_mir_build/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,8 +461,10 @@ pub(crate) struct NonExhaustivePatternsTypeNotEmpty<'p, 'tcx, 'm> {
pub ty: Ty<'tcx>,
}

impl<'a> IntoDiagnostic<'a> for NonExhaustivePatternsTypeNotEmpty<'_, '_, '_> {
fn into_diagnostic(self, dcx: &'a DiagCtxt, level: Level) -> DiagnosticBuilder<'_> {
impl<'a, G: EmissionGuarantee> IntoDiagnostic<'a, G>
for NonExhaustivePatternsTypeNotEmpty<'_, '_, '_>
{
fn into_diagnostic(self, dcx: &'a DiagCtxt, level: Level) -> DiagnosticBuilder<'_, G> {
let mut diag = DiagnosticBuilder::new(
dcx,
level,
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_parse/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,9 +1073,9 @@ pub(crate) struct ExpectedIdentifier {
pub help_cannot_start_number: Option<HelpIdentifierStartsWithNumber>,
}

impl<'a> IntoDiagnostic<'a> for ExpectedIdentifier {
impl<'a, G: EmissionGuarantee> IntoDiagnostic<'a, G> for ExpectedIdentifier {
#[track_caller]
fn into_diagnostic(self, dcx: &'a DiagCtxt, level: Level) -> DiagnosticBuilder<'a> {
fn into_diagnostic(self, dcx: &'a DiagCtxt, level: Level) -> DiagnosticBuilder<'a, G> {
let token_descr = TokenDescription::from_token(&self.token);

let mut diag = DiagnosticBuilder::new(
Expand Down Expand Up @@ -1133,9 +1133,9 @@ pub(crate) struct ExpectedSemi {
pub sugg: ExpectedSemiSugg,
}

impl<'a> IntoDiagnostic<'a> for ExpectedSemi {
impl<'a, G: EmissionGuarantee> IntoDiagnostic<'a, G> for ExpectedSemi {
#[track_caller]
fn into_diagnostic(self, dcx: &'a DiagCtxt, level: Level) -> DiagnosticBuilder<'a> {
fn into_diagnostic(self, dcx: &'a DiagCtxt, level: Level) -> DiagnosticBuilder<'a, G> {
let token_descr = TokenDescription::from_token(&self.token);

let mut diag = DiagnosticBuilder::new(
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_session/src/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ use std::num::NonZero;
use rustc_ast::token;
use rustc_ast::util::literal::LitError;
use rustc_errors::{
codes::*, DiagCtxt, DiagnosticBuilder, DiagnosticMessage, ErrorGuaranteed, IntoDiagnostic,
Level, MultiSpan,
codes::*, DiagCtxt, DiagnosticBuilder, DiagnosticMessage, EmissionGuarantee, ErrorGuaranteed,
IntoDiagnostic, Level, MultiSpan,
};
use rustc_macros::Diagnostic;
use rustc_span::{Span, Symbol};
Expand All @@ -17,9 +17,9 @@ pub struct FeatureGateError {
pub explain: DiagnosticMessage,
}

impl<'a> IntoDiagnostic<'a> for FeatureGateError {
impl<'a, G: EmissionGuarantee> IntoDiagnostic<'a, G> for FeatureGateError {
#[track_caller]
fn into_diagnostic(self, dcx: &'a DiagCtxt, level: Level) -> DiagnosticBuilder<'a> {
fn into_diagnostic(self, dcx: &'a DiagCtxt, level: Level) -> DiagnosticBuilder<'a, G> {
DiagnosticBuilder::new(dcx, level, self.explain).with_span(self.span).with_code(E0658)
}
}
Expand Down
Loading