-
Notifications
You must be signed in to change notification settings - Fork 12.1k
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.
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
Make lint diagnostics responsible for providing their primary span #125208
base: master
Are you sure you want to change the base?
Conversation
@bors p=1 prone to bitrot |
44d8cb9
to
68763a5
Compare
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 |
68763a5
to
b5bee8b
Compare
This comment has been minimized.
This comment has been minimized.
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
decorate_lint
is only responsible for decorating a lint diagnostic. It's not responsible for actually emitting the diagnostic!
fn decorate_lint<'b>(self, diag: &'b mut Diag<'a, G>); | ||
|
||
fn msg(&self) -> DiagMessage; | ||
fn span(&self) -> Option<MultiSpan>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to rename this to primary_span
.
@@ -152,6 +156,41 @@ impl<'a> LintDiagnosticDerive<'a> { | |||
} | |||
}); | |||
|
|||
let span = Self::KIND.each_variant(&mut structure, |_, variant| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
b5bee8b
to
5d6cff6
Compare
This comment has been minimized.
This comment has been minimized.
5d6cff6
to
ce91022
Compare
This comment has been minimized.
This comment has been minimized.
I foresee lots of merge conflicts with #124417 :/ |
ce91022
to
d8ce69b
Compare
@Xiretza Happy to block this PR on yours and do the rebasing myself here. |
☔ The latest upstream changes (presumably #125077) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, r=me once you're ready to see this merged
I've got an old branch merging |
Summary and Rustc-Dev-Facing Changes
Instead of passing the primary span of lint diagnostics separately — namely to the functions
tcx.emit_span_lint
andtcx.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 usetcx.emit_lint
andtcx.emit_node_lint
respectively and to provide the primary span when derivingLintDiagnostic
(via#[primary_span]
) / implementingLintDiagnostic
manually (viaLintDiagnostic::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 forLintDiagnostic
, i.e., setting the primary span withDiag::span
insidedecorate_lint
(that'sinto_diag
forDiagnostic
) becauselint_level
(lint_level_impl
) needs access to the primary spans before decorating theDiag
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 methodLintDiagnostic::msg
).Commits
Diagnostic
andLintDiagnostic
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