Re-port lint attributes to attribute parsers by Bryntet · Pull Request #155691 · rust-lang/rust

Bryntet · 2026-04-23T13:24:30Z

I managed to fix the related issues related to the lint attrs PR (one of which, with the help of @JonathanBrouwer ❤️ )

this is another attempt at #152369 and reverts #155056

follow up to this PR would be a cleaner way to do what I do in the commit c9e832c

probably by adding a new variant to ShouldEmit or changing the Nothing variant to have an option of whether to emit bugs or not

rustbot · 2026-04-23T13:24:34Z

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

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

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

JonathanBrouwer · 2026-04-23T14:06:36Z

Just a sanity check
@bors try @rust-timer queue

Re-add lint attributes

rust-bors · 2026-04-23T16:17:37Z

☀️ Try build successful (CI)
Build commit: a8bd7ef (a8bd7ef876125368f7f163a9b97cd468051ca908, parent: 827651f2200cefab42dac4c2ae7f80a7149340de)

JonathanBrouwer · 2026-04-23T16:25:18Z

@rust-timer build a8bd7ef

petrochenkov · 2026-04-23T15:39:21Z

+#![attr = Feature([fn_delegation#0])]
+extern crate std;
+#[attr = PreludeImport]
+use std::prelude::rust_2021::*;


Why doesn't HIR pretty printing respect the comment ordering anymore?

petrochenkov · 2026-04-23T16:07:29Z

+    original_name: Option<Symbol>,
+    /// Index of this lint, used to keep track of lint groups
+    lint_index: usize,
+    kind: LintAttrTool,


Any specific reason to make the fields here private and provide trivial accessor methods instead?
Most of the other data in this file is public.

View changes since the review

I mainly just don't want other places constructing LintInstance without using LintInstance::new

I'm fine with making these public though if you think this would be preferable!

petrochenkov · 2026-04-23T16:13:34Z

+            None => LintAttrTool::NoTool,
+        };
+
+        Self { original_name, span, lint_index, lint_name, kind }


Could you avoid using Self in non-generic contexts? At least in constructors like this.

View changes since the review

would you prefer this in the function signature as well?

Up to you, I think having LintInstance { and LintInstance::new constructors searchable is particularly important, but I personally usually avoid Self in other cases too.

petrochenkov · 2026-04-23T16:31:34Z

I'd like to review this PR before it is merged.
I started today and will continue tomorrow.

The PR is huge and contains both various refactorings (registered_tools, early.rs, string -> symbol, etc) and the core change (migrating lint attributes), and perhaps some post-migration cleanups too.
It would be good to split it into at least 2 PRs, one for refactorings (with explanation/motivation for each change) and then another for the core change.

Also cjgilllot left some comments in #152369, didn't check if they were addressed yet.

rust-timer · 2026-04-23T17:16:43Z

Finished benchmarking commit (a8bd7ef): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

@bors rollup=never
@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.3%	[0.2%, 0.6%]	8
Improvements ✅ (primary)	-0.3%	[-0.6%, -0.2%]	16
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.2%]	26
All ❌✅ (primary)	-0.3%	[-0.6%, -0.2%]	16

Max RSS (memory usage)

Results (primary -0.0%, secondary 1.0%)

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

	mean	range	count
Regressions ❌ (primary)	3.2%	[3.2%, 3.2%]	1
Regressions ❌ (secondary)	4.6%	[2.0%, 6.5%]	7
Improvements ✅ (primary)	-0.8%	[-1.3%, -0.6%]	4
Improvements ✅ (secondary)	-2.5%	[-6.9%, -1.1%]	7
All ❌✅ (primary)	-0.0%	[-1.3%, 3.2%]	5

Cycles

Results (primary 2.9%, secondary 4.3%)

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

	mean	range	count
Regressions ❌ (primary)	2.9%	[2.0%, 4.0%]	7
Regressions ❌ (secondary)	4.3%	[2.4%, 6.1%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.9%	[2.0%, 4.0%]	7

Binary size

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

Bootstrap: 490.734s -> 492.119s (0.28%)
Artifact size: 394.28 MiB -> 394.31 MiB (0.01%)

JonathanBrouwer

Will do some more review later, some initial comments

View changes since this review

rustbot · 2026-04-24T09:29:07Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

JonathanBrouwer · 2026-04-24T10:02:28Z

+    fn check_unused_attribute(&self, hir_id: HirId, attr: &Attribute) {
+        // Warn on useless empty attributes.
+        // FIXME(jdonszelmann): this lint should be moved to attribute parsing, see `AcceptContext::warn_empty_attribute`
+        let note = if attr.has_any_name(&[sym::feature])


Why is this unused check for feature added here?

View changes since the review

diff looks weird, but it's because I refactored the check_lint_attr logic out of the already existing function check_unused_attribute

I can't find this check anywhere in the old code tho, could you point me to where this check was moved from?

JonathanBrouwer · 2026-04-24T10:12:55Z

 /// context, through which all attributes can be lowered.
 pub struct AttributeParser<'sess, S: Stage = Late> {
-    pub(crate) tools: Vec<Symbol>,
+    pub(crate) tools: Option<&'sess FxIndexSet<Ident>>,


Could you make this change to tools a separate PR that is merged before this one, to reduce the size of this one a bit?

View changes since the review

I'd rather move all the other refactorings too #155691 (comment), so the core change becomes as minimal as possible.

JonathanBrouwer · 2026-04-24T10:23:25Z

-                }
-            }
-            false
+            self.tcx.hir_attrs(id).iter().any(Attribute::has_span_without_desugaring_kind)


This looks like it might partially fix #143094 (for the attrs that has_span_without_desugaring_kind supports)

Does it?

Why does this change need to be in this PR?

View changes since the review

I created the helper function has_span_without_desugaring_kind because if you don't do this check in a specific way, you'll get ICE, and the same check was done in multiple places, and it would become a big mess if we don't have a helper function

I think that issue could possibly be fixed by expanding on this helper function, it does not fix that specific issue right now, no.

JonathanBrouwer · 2026-04-24T10:23:52Z

-                }
-            }
-            false
+            self.tcx.hir_attrs(id).iter().any(hir::Attribute::has_span_without_desugaring_kind)


Why this change?

View changes since the review

see my response to the comment above

rustbot · 2026-04-24T10:57:13Z

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.

petrochenkov · 2026-04-24T12:12:30Z

                &mut emit_lint,
            );
        }
+        let attr_id = sess.psess.attr_id_generator.mk_attr_id();


This is not good, I'd rather make the attr_id in the context optional.
When processing lint attributes it is always set, so it can be unwrapped there.

View changes since the review

petrochenkov · 2026-04-24T12:15:04Z


+    /// This method provides the same functionality as [`parse_limited_all`](Self::parse_limited_all) except filtered,
+    /// making sure that only allow-listed symbols are parsed
+    pub fn parse_limited_all_filtered<'a>(


Calling this function is not much simpler than calling the underlying parse_limited_all with an iterator.
If levels.rs calls it multiple times, then it could have a more specialized private helper, but otherwise it would probably be better to reduce the API surface.

View changes since the review

petrochenkov · 2026-04-24T12:22:10Z

+                                | [sym::deny]
+                                | [sym::expect]
+                                | [sym::forbid]
+                                | [sym::warn]


What is the special relation between lint attributes and incorrect delimiters?
Why shouldn't the delimiters be reported for lint attributes specifically?

View changes since the review

it's specifically only in the case of when ShouldEmit::Nothing

when we compile a file, the the pre ast expansion linting pass doesn't care about cfg attrs

the only attributes we ever parse during pre ast expansion linting, are the lint attributes

because of the fact that in pre expansion, the lint attrs have the ShouldEmit::Nothing, so that we don't get duplicate error emissions, but since we don't know whether the attribute will actually be checked by later steps, (attribute could be removed by cfg), and the behaviour of ShouldEmit::Nothing.emit_err() is to delay_bug, which leads to a delayed bug which never errors and therefore we get ICE, see issue #154800

I want to make another PR later, making this cleaner, where we add maybe another variant to ShouldEmit that doesn't delay bugs or emit errors

petrochenkov · 2026-04-24T12:26:23Z

+
+pub(crate) trait Lint {
+    const KIND: LintAttributeKind;
+    const ATTR_SYMBOL: Symbol = Self::KIND.symbol();


Is ATTR_SYMBOl ever overridden to something other than Self::KIND.symbol()?
If not, then it shouldn't be a trait item.

I'm not actually sure why lint attribute kinds need to exist at type level (as opposed to value level LintAttributeKind enum).

View changes since the review

Is ATTR_SYMBOl ever overridden to something other than Self::KIND.symbol()?
If not, then it shouldn't be a trait item.

will change this

I'm not actually sure why lint attribute kinds need to exist at type level (as opposed to value level LintAttributeKind enum).

Because lints behave differently depending on the order lint attributes were placed on a target,

#[deny(unused)] #[allow(unused)] fn (my_var: bool) { }

gives different output compared to

#[allow(unused)] #[deny(unused)] fn (my_var: bool) { }

and unless we want to add an index number to each variant (only possible by being parsed by the same AttributeParser), and then also add support for AttributeParser::finalize to return multiple different AttributeKind as well as re-sorting the attributes into correct order

alternatively, you could keep the parsers separate, and sort them based on their span location, everytime you want to use them, but as you noted in a different comment, we shouldn't use spans for this, and it's innefficient.

petrochenkov · 2026-04-24T12:31:43Z

+
+    fn finalize(mut self, _cx: &FinalizeContext<'_, '_, S>) -> Option<AttributeKind> {
+        if !self.lint_attrs.is_empty() {
+            // Sort to ensure correct order operations later


Span ordering shouldn't generally be relied upon for anything besides displaying diagnostics.

View changes since the review

Just tested and this sort actually isn't necessary, which is nice :)

petrochenkov · 2026-04-24T12:42:48Z

+        &[]
    }
    fn check<'ecx, 'tcx, T: EarlyLintPass>(self, cx: &mut EarlyContextAndPass<'ecx, 'tcx, T>) {
-        walk_list!(cx, visit_attribute, self.1);


I'm not sure what this change achieves.
Pre-expansion lints never want to check node attributes?

View changes since the review

petrochenkov · 2026-04-24T12:47:31Z

    }

-    pub fn set_lint_index(&mut self, new_lint_index: Option<u16>) {
+    pub fn set_lint_index(&mut self, new_lint_index: u16) {


This method is never used now.
get_lint_index isn't used either.

View changes since the review

rust-bors · 2026-04-25T00:31:54Z

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

cjgillot

Part of the complexity in this PR comes from moving tool and lint name validation. Why do those need to be moved? Why not leave them in rustc_lint::level which needs to do this validation anyway?

View changes since this review

Bryntet · 2026-04-30T06:34:23Z

Part of the complexity in this PR comes from moving tool and lint name validation. Why do those need to be moved? Why not leave them in rustc_lint::level which needs to do this validation anyway?

Because clippy otherwise manually parses the level from the AST in multiple places

JonathanBrouwer · 2026-04-30T06:39:02Z

Because clippy otherwise manually parses the level from the AST in multiple places

Can we fix that in a separate PR, and just let clippy do that for now?

@petrochenkov

change field `tools` on `AttributeParser` to hold `&'tcx RegisteredTools` Makes tools actually stored, and not just tool names this was originally part of rust-lang#155691 but was split out to make that PR smaller. r? @petrochenkov cc @JonathanBrouwer

@petrochenkov

change field `tools` on `AttributeParser` to hold `&'tcx RegisteredTools` Makes tools actually stored, and not just tool names this was originally part of rust-lang#155691 but was split out to make that PR smaller. r? @petrochenkov cc @JonathanBrouwer

@petrochenkov

change field `tools` on `AttributeParser` to hold `&'tcx RegisteredTools` Makes tools actually stored, and not just tool names this was originally part of rust-lang#155691 but was split out to make that PR smaller. r? @petrochenkov cc @JonathanBrouwer

@petrochenkov

Rollup merge of #156091 - Bryntet:move-tools, r=JonathanBrouwer change field `tools` on `AttributeParser` to hold `&'tcx RegisteredTools` Makes tools actually stored, and not just tool names this was originally part of #155691 but was split out to make that PR smaller. r? @petrochenkov cc @JonathanBrouwer

rustbot assigned JonathanBrouwer Apr 23, 2026

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 23, 2026

rust-bors Bot pushed a commit that referenced this pull request Apr 23, 2026

Auto merge of #155691 - Bryntet:lint_attrs_v2, r=<try>

a8bd7ef

Re-add lint attributes

petrochenkov self-assigned this Apr 23, 2026

This comment has been minimized.

Sign in to view

petrochenkov reviewed Apr 23, 2026

View reviewed changes

JonathanBrouwer changed the title ~~Re-add lint attributes~~ Re-port lint attributes to attribute parsers Apr 23, 2026

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 23, 2026

Bryntet force-pushed the lint_attrs_v2 branch from a7db19b to 0b99f69 Compare April 23, 2026 17:18

JonathanBrouwer assigned jdonszelmann Apr 23, 2026

Bryntet force-pushed the lint_attrs_v2 branch from 0b99f69 to 8790380 Compare April 24, 2026 09:22

JonathanBrouwer requested changes Apr 24, 2026

View reviewed changes

Comment thread tests/ui/parser/issues/issue-104620.rs Outdated

Comment thread tests/ui/lint/rfc-2383-lint-reason/multiple_expect_attrs.rs

Comment thread src/tools/clippy/clippy_lints/src/returns/needless_return.rs

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 24, 2026

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 24, 2026

Bryntet force-pushed the lint_attrs_v2 branch from 8790380 to 65519a6 Compare April 24, 2026 09:34

Bryntet force-pushed the lint_attrs_v2 branch 2 times, most recently from 803cb3c to 1a81a4b Compare April 24, 2026 09:48

JonathanBrouwer requested changes Apr 24, 2026

View reviewed changes

Bryntet force-pushed the lint_attrs_v2 branch from 1a81a4b to 955c72d Compare April 24, 2026 10:45

This comment has been minimized.

Sign in to view

Bryntet force-pushed the lint_attrs_v2 branch from 955c72d to a9c8cf3 Compare April 24, 2026 10:47

Bryntet and others added 3 commits April 24, 2026 12:56

unrevert lint attrs

2c65d6d

dont delay bugs on lint attrs during pre-ast-expansion lint checking

5ca7af3

Fix incr issue

7dc9dda

Bryntet force-pushed the lint_attrs_v2 branch from a9c8cf3 to 7dc9dda Compare April 24, 2026 10:57

petrochenkov reviewed Apr 24, 2026

View reviewed changes

cjgillot reviewed Apr 25, 2026

View reviewed changes

Bryntet mentioned this pull request May 2, 2026

change field tools on AttributeParser to hold &'tcx RegisteredTools #156091

Merged

Uh oh!

Conversation

Bryntet commented Apr 23, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Apr 23, 2026

Uh oh!

JonathanBrouwer commented Apr 23, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors Bot commented Apr 23, 2026

Uh oh!

JonathanBrouwer commented Apr 23, 2026

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Uh oh!

petrochenkov Apr 23, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bryntet Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrochenkov Apr 23, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented Apr 23, 2026

Uh oh!

rust-timer commented Apr 23, 2026

Overall result: ❌✅ regressions and improvements - please read:

Uh oh!

JonathanBrouwer left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rustbot commented Apr 24, 2026

Uh oh!

JonathanBrouwer Apr 24, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JonathanBrouwer Apr 24, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonathanBrouwer Apr 24, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonathanBrouwer Apr 24, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bryntet commented Apr 23, 2026 •

edited by rustbot

Loading

petrochenkov Apr 23, 2026 •

edited by rustbot

Loading

Bryntet Apr 23, 2026 •

edited

Loading

petrochenkov Apr 23, 2026 •

edited by rustbot

Loading

JonathanBrouwer left a comment •

edited by rustbot

Loading

JonathanBrouwer Apr 24, 2026 •

edited by rustbot

Loading

JonathanBrouwer Apr 24, 2026 •

edited by rustbot

Loading

JonathanBrouwer Apr 24, 2026 •

edited by rustbot

Loading

JonathanBrouwer Apr 24, 2026 •

edited by rustbot

Loading

petrochenkov Apr 24, 2026 •

edited by rustbot

Loading

petrochenkov Apr 24, 2026 •

edited by rustbot

Loading

petrochenkov Apr 24, 2026 •

edited by rustbot

Loading

petrochenkov Apr 24, 2026 •

edited by rustbot

Loading

petrochenkov Apr 24, 2026 •

edited by rustbot

Loading

petrochenkov Apr 24, 2026 •

edited by rustbot

Loading

petrochenkov Apr 24, 2026 •

edited by rustbot

Loading

cjgillot left a comment •

edited by rustbot

Loading