Skip to content

Move lint validation to attribute parsing - #164918

Open
JonathanBrouwer wants to merge 2 commits into
rust-lang:mainfrom
JonathanBrouwer:lint-id
Open

JonathanBrouwer wants to merge 2 commits into
rust-lang:mainfrom
JonathanBrouwer:lint-id

Conversation

@JonathanBrouwer

@JonathanBrouwer JonathanBrouwer commented Oct 25, 2026 •

Copy link
Copy Markdown
Member

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

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 25, 2026
/// - `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,

@JonathanBrouwer JonathanBrouwer Oct 25, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

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

@JonathanBrouwer JonathanBrouwer Oct 25, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

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

@JonathanBrouwer JonathanBrouwer Oct 25, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was wrong before! Oops!

View changes since the review

),
};

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

@JonathanBrouwer JonathanBrouwer Oct 25, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 25, 2026
@JonathanBrouwer
JonathanBrouwer marked this pull request as ready for review October 25, 2026 16:17
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Oct 25, 2026
Move lint validation to attribute parsing
@rustbot

rustbot commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator

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

@rustbot rustbot 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 Oct 25, 2026
@rust-bors

rust-bors Bot commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

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

@rust-timer

This comment has been minimized.

@rust-timer

Copy link
Copy Markdown
Collaborator

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 25, 2026
@rust-bors

This comment has been minimized.

@rustbot

This comment has been minimized.

@rust-bors

This comment has been minimized.

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

rustbot commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

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 mejrs left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@rustbot rustbot 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 Oct 26, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-clippy Relevant to the Clippy team. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants