Make lint diagnostics responsible for providing their primary span #125208

fmease · 2024-05-17T11:11:14Z

Summary and Rustc-Dev-Facing Changes

Instead of passing the primary span of lint diagnostics separately — namely to the functions tcx.emit_span_lint and tcx.emit_node_span_lint —, make lint diagnostics responsible for “storing” (providing) their span. I've removed the two aforementioned methods. You are now expected to use tcx.emit_lint and tcx.emit_node_lint respectively and to provide the primary span when deriving LintDiagnostic (via #[primary_span]) / implementing LintDiagnostic manually (via LintDiagnostic::span).

Motivation

Making the general format of #[derive(LintDiagnostic)] identical to #[derive(Diagnostic)]. I bet this has lead to slight confusion or annoyances before. Moreover, it enables deriving both traits at once (see #125169 for the motivation).

Approach

As outlined in #125169 (comment), the naïve approach of doing the same as Diagnostic doesn't work for LintDiagnostic, i.e., setting the primary span with Diag::span inside decorate_lint (that's into_diag for Diagnostic) because lint_level (lint_level_impl) needs access to the primary spans before decorating the Diag to be able to filter out some lints (namely if they come from an external macro) etc.

Therefore, I had to introduce a new method to LintDiagnostic: fn span(&self) -> Option<MultiSpan> (similar to the existing method LintDiagnostic::msg).

Commits

The 1st commit addresses #125169 (comment). It contains the necessary changes to the linting APIs. It doesn't build on its own due to the removed APIs. Split out for easier reviewing.
The 2nd commit updates all existing lint diagnostics to use the new APIs. Most of them are mindless, monotonous, obvious changes. Some changes are slightly more involved out of necessity. I've verified (partly programmatically, partly manually) that all lint diags that used to have a primary span still have a primary span.
The 3rd commit updates the fulldeps UI tests that exercise the (lint) diagnostic API
The 4th commit fixes #125169. I've only deduplicated the lint diagnostic structs mentioned in the issue and haven't gone looking for other structs that may benefit from deriving both Diagnostic and LintDiagnostic because I didn't have the time to do so yet.

Meta

Fixes #125169.

I'll submit a rustc-dev-guide PR later.
I hope this doesn't need an MCP, it's pretty straight forward.

cc @davidtwco
r? @nnethercote or anyone on compiler who has the energy to review 82 files

fmease · 2024-05-17T11:12:39Z

@bors p=1 prone to bitrot

rustbot · 2024-05-17T11:20:49Z

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

Some changes occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred in match checking

cc @Nadrieril

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

fmease · 2024-05-17T14:43:41Z

compiler/rustc_errors/src/diagnostic.rs

@@ -198,10 +198,11 @@ pub trait SubdiagMessageOp<G: EmissionGuarantee> =
 /// `#[derive(LintDiagnostic)]` -- see [rustc_macros::LintDiagnostic].
 #[rustc_diagnostic_item = "LintDiagnostic"]
 pub trait LintDiagnostic<'a, G: EmissionGuarantee> {
-    /// Decorate and emit a lint.


decorate_lint is only responsible for decorating a lint diagnostic. It's not responsible for actually emitting the diagnostic!

fmease · 2024-05-17T14:43:53Z

compiler/rustc_errors/src/diagnostic.rs

    fn decorate_lint<'b>(self, diag: &'b mut Diag<'a, G>);

    fn msg(&self) -> DiagMessage;
+    fn span(&self) -> Option<MultiSpan>;


Open to rename this to primary_span.

fmease · 2024-05-17T14:46:34Z

compiler/rustc_macros/src/diagnostics/diagnostic.rs

@@ -152,6 +156,41 @@ impl<'a> LintDiagnosticDerive<'a> {
            }
        });

+        let span = Self::KIND.each_variant(&mut structure, |_, variant| {


Initially I tried to implement this in a DRY way by reusing existing functions from diagnostic_builder but in the end I opted for just doing it the straightforward way because it wasn't entirely obvious to me how I should refactor diagnostic_builder to suit my needs.

Xiretza · 2024-05-17T16:21:28Z

I foresee lots of merge conflicts with #124417 :/

fmease · 2024-05-17T16:48:32Z

@Xiretza Happy to block this PR on yours and do the rebasing myself here.

bors · 2024-05-18T22:40:20Z

☔ The latest upstream changes (presumably #125077) made this pull request unmergeable. Please resolve the merge conflicts.

nnethercote · 2024-05-20T07:00:32Z

@Xiretza Happy to block this PR on yours and do the rebasing myself here.

@fmease: this seems fine to me but I guess you're still waiting on this.

davidtwco

This looks good to me, r=me once you're ready to see this merged

davidtwco · 2024-05-20T14:06:12Z

I've got an old branch merging LintDiagnostic and Diagnostic that you may want to use as a base for follow-ups for this (https://rust-lang.zulipchat.com/#narrow/stream/147480-t-compiler.2Fwg-diagnostics/topic/fluent.20and.20future-incompat/near/425579646) but maybe it is rendered obsolete by this change too.

fmease added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 17, 2024

rustbot assigned nnethercote May 17, 2024

fmease force-pushed the lint-diags-store-their-span branch from 44d8cb9 to 68763a5 Compare May 17, 2024 11:20

fmease marked this pull request as ready for review May 17, 2024 11:20

fmease removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 17, 2024

fmease changed the title ~~Make lint diagnostics responsible for storing their span~~ Make lint diagnostics responsible for storing their primary span May 17, 2024

fmease force-pushed the lint-diags-store-their-span branch from 68763a5 to b5bee8b Compare May 17, 2024 11:26

This comment has been minimized.

Sign in to view

fmease added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2024

fmease changed the title ~~Make lint diagnostics responsible for storing their primary span~~ Make lint diagnostics responsible for providing their primary span May 17, 2024

fmease commented May 17, 2024

View reviewed changes

fmease added 2 commits May 17, 2024 16:49

Make lint diagnostics responsible for storing their span

115ead1

Update all lint diagnostics to store their span

957c07f

fmease force-pushed the lint-diags-store-their-span branch from b5bee8b to 5d6cff6 Compare May 17, 2024 14:50

fmease added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 17, 2024

This comment has been minimized.

Sign in to view

fmease force-pushed the lint-diags-store-their-span branch from 5d6cff6 to ce91022 Compare May 17, 2024 15:32

This comment has been minimized.

Sign in to view

fmease added 2 commits May 17, 2024 18:42

Update fulldeps UI tests to the new lint diagnostic API

4c02f16

Derive both Diagnostic and LintDiagnostic on some tys

d8ce69b

fmease force-pushed the lint-diags-store-their-span branch from ce91022 to d8ce69b Compare May 17, 2024 16:43

fmease added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label May 17, 2024

davidtwco approved these changes May 20, 2024

View reviewed changes

fmease removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make lint diagnostics responsible for providing their primary span #125208

Make lint diagnostics responsible for providing their primary span #125208

fmease commented May 17, 2024 •

edited

fmease commented May 17, 2024

rustbot commented May 17, 2024

This comment has been minimized.

fmease May 17, 2024

fmease May 17, 2024

fmease May 17, 2024 •

edited

This comment has been minimized.

This comment has been minimized.

Xiretza commented May 17, 2024

fmease commented May 17, 2024

bors commented May 18, 2024

nnethercote commented May 20, 2024

davidtwco left a comment

davidtwco commented May 20, 2024

Make lint diagnostics responsible for providing their primary span #125208

Are you sure you want to change the base?

Make lint diagnostics responsible for providing their primary span #125208

Conversation

fmease commented May 17, 2024 • edited

Summary and Rustc-Dev-Facing Changes

Motivation

Approach

Commits

Meta

fmease commented May 17, 2024

rustbot commented May 17, 2024

This comment has been minimized.

fmease May 17, 2024

Choose a reason for hiding this comment

fmease May 17, 2024

Choose a reason for hiding this comment

fmease May 17, 2024 • edited

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Xiretza commented May 17, 2024

fmease commented May 17, 2024

bors commented May 18, 2024

nnethercote commented May 20, 2024

davidtwco left a comment

Choose a reason for hiding this comment

davidtwco commented May 20, 2024

fmease commented May 17, 2024 •

edited

fmease May 17, 2024 •

edited