Move lint validation to attribute parsing by JonathanBrouwer · Pull Request #164918 · rust-lang/rust

JonathanBrouwer · 2026-10-25T16:08:48Z

This is an alternative approach to #164607

This can be reviewed commit by commit, each commit passes tests.
My goal is for a follow-up to remove lint_name_normalized completely and replace it by a LintId

r? @mejrs

JonathanBrouwer · 2026-10-25T16:10:38Z

-    /// - `custom_tool::blah::bleh` -> `blah::bleh`
-    pub lint_name: Symbol,
+    /// For example, `single_use_lifetime` is changed to `single_use_lifetimes`.
+    pub lint_name_normalized: Symbol,


My goal for a follow-up PR is to change this field from a Symbol that needs to be looked up in LintStore to a LintId. I think this should be possible, but encoding and decoding will be a challenge. I think this PR is already a good step forwards regardless.

View changes since the review

JonathanBrouwer · 2026-10-25T16:11:27Z

 pub use attributes::util::{is_builtin_attr, parse_version};
 pub use context::ShouldEmit;
-pub use diagnostics::ParsedDescription;
+pub use diagnostics::{ParsedDescription, RenamedLintSuggestion, UnknownLintSuggestion};


I'm not sure if people find it cursed to export diagnostic structs such that they can be re-used. I like it

View changes since the review

I don't, it can hide unused diagnostics 😅

JonathanBrouwer · 2026-10-25T16:11:46Z

            CRATE_NODE_ID,
            Some(self.features),
            ShouldEmit::ErrorsAndLints { recovery: Recovery::Allowed },
+            None,


This was wrong before! Oops!

View changes since the review

Can you add a test that would've caught this?

Wait, is this actually a bug? I m pretty sure we didnt access this before.

Yeah I don't think this is observable

JonathanBrouwer · 2026-10-25T16:12:11Z

                                    ),
                                };

+                                let lint_name_normalized = if let Some((_lint_ids, new_lint_name)) =


Note that we already have the lint_ids here! We just need to figure out how to encode & decode them in a follow-up PR!

View changes since the review

JonathanBrouwer · 2026-10-25T16:17:06Z

@bors try @rust-timer queue

Move lint validation to attribute parsing

rustbot · 2026-10-25T16:17:15Z

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

Some changes occurred in compiler/rustc_attr_ir

cc @jdonszelmann

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

rust-bors · 2026-10-25T17:16:11Z

☀️ Try build successful (CI)
Build commit: 00e2c28 (00e2c28e2c046bbcd149ca7f1bf88c8cfc9e8875)
Base parent: dc44e7f (dc44e7f2c436b6041742e138b9c2176f093196d5)

rust-timer · 2026-10-25T18:05:43Z

Finished benchmarking commit (00e2c28): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@bors rollup=iffy rustc-perf
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.3%	[-0.3%, -0.1%]	9
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.3%, -0.1%]	9

Max RSS (memory usage)

Results (primary -0.5%, secondary -0.7%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.5%, 0.9%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.4%	[-3.1%, -0.5%]	6
Improvements ✅ (secondary)	-0.7%	[-0.7%, -0.7%]	1
All ❌✅ (primary)	-0.5%	[-3.1%, 0.9%]	11

Cycles

Results (primary 0.4%, secondary 0.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.9%	[0.5%, 2.1%]	9
Regressions ❌ (secondary)	1.3%	[0.5%, 3.4%]	5
Improvements ✅ (primary)	-1.0%	[-1.7%, -0.7%]	3
Improvements ✅ (secondary)	-1.3%	[-2.1%, -0.5%]	2
All ❌✅ (primary)	0.4%	[-1.7%, 2.1%]	12

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 487.351s -> 486.11s (-0.25%)
Artifact size: 405.87 MiB -> 406.01 MiB (0.03%)

We will need the `lint_tools` in the next commits

This is a lot of code being moved around, but not much of the logic changes. I changed some stuff though, so I recommend reviewing the logic side by side.

rustbot · 2026-10-26T14:33:51Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

mejrs

I don't think this change is correct, it means lowering ast to hir depends on what lints have been registered. Does this mean cargo clippy and cargo check could produce different artefacts? What happens when rust-analyzer decides to implement its own lints? (it's possible, it's a pregistered tool).

Can you talk to people more knowledgeable about this and get them to sign off on this? The people that are working on sharing cargo build caches probably have opinions about this.

@rustbot author

View changes since this review

JonathanBrouwer · 2026-10-27T13:22:22Z

Discussion started in #t-compiler > What session state is hashed for incremental? @ 💬

JonathanBrouwer · 2026-10-27T15:13:54Z

Thanks for bringing that up, while I had considered quickly whether accessing the lint store during ast lowering is fine, I did not have a correct understanding of how this part of incremental worked. I assumed the lint store was tracked, but it's not and that's also fine. When tools will start sharing artifacts, we should track the lint store.

Bjorn confirmed this behavior in the linked thread.
@rustbot ready

mejrs

Can you summarize the zulip discussion in a comment somewhere?

View changes since this review

mejrs · 2026-10-27T16:18:51Z

    pub tool: Option<Ident>,
-    /// The lint's name, minus the tool name if any.
+    /// The full normalized lint name of an existing lint, including the tool name.
+    /// This may not be what we user wrote down, in case of renamed lints the new lint name is here.


Until #164607 this can also recover unknown lint names to a clippy lint even when it is not registered as renamed.

mejrs · 2026-10-27T17:09:21Z

 pub use attributes::util::{is_builtin_attr, parse_version};
 pub use context::ShouldEmit;
-pub use diagnostics::ParsedDescription;
+pub use diagnostics::{ParsedDescription, RenamedLintSuggestion, UnknownLintSuggestion};


I don't, it can hide unused diagnostics 😅

mejrs · 2026-10-27T17:22:46Z

@@ -45,13 +45,12 @@
 pub struct LintCheck {
    /// The lint's tool name, if present.
    pub tool: Option<Ident>,


I don't think this span is used

mejrs · 2026-10-27T17:22:57Z

    /// Like mentioned above, multiple lint attributes and multiple lints
    /// inside one attribute are all smooshed and flattened together, so
    /// multiple `LintCheck`s can have the same `attr_span`.
    pub attr_span: Span,


same for this span

mejrs · 2026-10-27T17:24:08Z

                                };

+                                let lint_name_normalized = if let Some((_lint_ids, new_lint_name)) =
+                                    Self::check_lint_name(lint_name, tool, p.span(), cx)


Suggested change

Self::check_lint_name(lint_name, tool, p.span(), cx)

LintParser::check_lint_name(lint_name, tool, p.span(), cx)

Avoid using Self in non generic contexts

mejrs · 2026-10-27T17:36:46Z

        }
    }

    fn check_lint_check(&self, hir_id: HirId, lints: &[LintCheck]) {


I wonder if this check can be in attribute parsing as well (didn't touch it during the initial port)

mejrs · 2026-10-27T17:37:13Z

+        &self,
+        lint_name: &str,
+        tool_name: Option<Symbol>,
+        registered_tools: &RegisteredTools,


Suggested change

registered_tools: &RegisteredTools,

lint_tools: &RegisteredTools,

mejrs · 2026-10-27T17:45:54Z

+                                    new_lint_name
+                                } else {
+                                    // Discard lint attributes with unknown lints in them
+                                    continue;


Please cover this in a test, I think it would be best to have a hir pretty test showing that unknown lints and lints whose tool is inactive are dropped.

mejrs · 2026-10-27T18:01:19Z

        }))
    }
+
+    fn check_lint_name(


Suggested change

fn check_lint_name(

fn check_attribute_lint_name(

this looks a bit recursive, would be nice to have different names (or have check_lint_name inlined here).

mejrs · 2026-10-27T18:09:51Z

+        // We only want to emit these lints once, on the HIR.
+        // On the AST `target_span` will be `DUMMY_SP`, which we use to detect which instance we are.
+        // #164898 will fix this, after that PR this check can be removed
+        let should_lint = cx.target_span != DUMMY_SP;


I don't think this is a good idea:

I would not accept this as a standalone PR (instead demanding a more rigorous fix)

Try to keep behavior changes to a minimum during refactors

mejrs · 2026-10-27T18:25:58Z

I also don't understand why this ought to block #164607

I still think we should store a LintName rather than a Symbol. (whether or not they are normalized), at least until you figure out your Lintid thing

mejrs · 2026-10-27T22:34:36Z

This PR is a breaking change, it now correctly rejects unknown tool lints

Consider the following code:

macro_rules! wat {
    () => {
        #[allow(unused_attributes)]
        {
            #[allow(unregistered_tool::lint)]
            println!("boo");
        }
    };
}

fn main() {
    wat!();
}

Behavior on current Rust (all channels)

compiles, no diagnostic issued

With this pr:

error[E0710]: unknown tool name `unregistered_tool` found in scoped lint: `unregistered_tool::lint`
  --> $DIR/inner.rs:5:21
   |
LL | macro_rules! wat {
...
LL |             #[allow(unregistered_tool::lint)]
   |                     ^^^^^^^^^^^^^^^^^
...
LL |     wat!();
   |     ------ in this macro invocation
   |
   = help: add `#![register_tool(unregistered_tool)]` to the crate root

error: aborting due to 1 previous error

JonathanBrouwer · 2026-10-28T11:43:54Z

Right, that's a fun one I didn't think about!
I think we can go for merging #164607 first given that this PR needs to go through lang

rust-bors · 2026-10-28T18:37:06Z

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

JonathanBrouwer commented Oct 25, 2026

View reviewed changes

rustbot assigned mejrs Oct 25, 2026