Repository navigation
Move lint validation to attribute parsing - #164918
JonathanBrouwer wants to merge 2 commits into
Conversation
| /// - `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, |
There was a problem hiding this comment.
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.
| pub use attributes::util::{is_builtin_attr, parse_version}; | ||
| pub use context::ShouldEmit; | ||
| pub use diagnostics::ParsedDescription; | ||
| pub use diagnostics::{ParsedDescription, RenamedLintSuggestion, UnknownLintSuggestion}; |
There was a problem hiding this comment.
I'm not sure if people find it cursed to export diagnostic structs such that they can be re-used. I like it
| CRATE_NODE_ID, | ||
| Some(self.features), | ||
| ShouldEmit::ErrorsAndLints { recovery: Recovery::Allowed }, | ||
| None, |
There was a problem hiding this comment.
This was wrong before! Oops!
| ), | ||
| }; | ||
|
|
||
| let lint_name_normalized = if let Some((_lint_ids, new_lint_name)) = |
There was a problem hiding this comment.
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!
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Move lint validation to attribute parsing
|
Some changes occurred in compiler/rustc_attr_parsing Some changes occurred in compiler/rustc_attr_ir
cc @rust-lang/clippy Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
1a9e5b3 to
cb8338e
Compare
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (00e2c28): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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 Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
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.
CyclesResults (primary 0.4%, secondary 0.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 487.351s -> 486.11s (-0.25%) |
This comment has been minimized.
This comment has been minimized.
cb8338e to
5a6079c
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
5a6079c to
62ff266
Compare
|
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. |
There was a problem hiding this comment.
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
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_normalizedcompletely and replace it by aLintIdr? @mejrs