Re-port lint attributes to attribute parsers rust-lang/rust#155691
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
Just a sanity check
@bors try @rust-timer queue
View on GitHub
Awaiting bors try build completion.
@rustbot label: +S-waiting-on-perf
View on GitHub
⌛ Trying commit a7db19b with merge a8bd7ef…
To cancel the try build, run the command @bors try cancel.
Workflow: https://github.com/rust-lang/rust/actions/runs/24839909692
@rust-timer build a8bd7ef
View on GitHub
Queued a8bd7ef with parent 827651f, future comparison URL.
There is currently 1 preceding artifact in the queue.
It will probably take at least ~1.1 hours until the benchmark run finishes.
tests/pretty/delegation-inherit-attributes.pp
| 3 | #![attr = Feature([fn_delegation#0])] |
|
| 4 | extern crate std; |
|
| 5 | #[attr = PreludeImport] |
|
| 6 | use std::prelude::rust_2021::*; |
Why doesn't HIR pretty printing respect the comment ordering anymore?
compiler/rustc_span/src/caching_source_map_view.rs · outdated
| 142 | 142 | span_data.lo - lo_line_bounds.start, |
| 143 | 143 | hi_line_number, |
| 144 | span_data.hi - hi_line_bounds.start, |
|
| 144 | span_data.hi, |
What is this change for?
compiler/rustc_hir/src/attrs/data_structures.rs · outdated
| 929 | original_name: Option<Symbol>, |
|
| 930 | /// Index of this lint, used to keep track of lint groups |
|
| 931 | lint_index: usize, |
|
| 932 | 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.
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!
compiler/rustc_hir/src/attrs/data_structures.rs · outdated
| 923 | /// The fully resolved name of the lint |
|
| 924 | /// for renamed lints, this gets updated to match the new name |
|
| 925 | lint_name: Symbol, |
|
| 926 | /// The raw identifier for resolving this lint |
Nit: "raw identifier" has a specific and different meaning.
compiler/rustc_hir/src/attrs/data_structures.rs · resolved
| 1002 | if let LintAttrTool::Present { tool_name, .. } = self.kind { Some(tool_name) } else { None } |
|
| 1003 | } |
|
| 1004 | |
|
| 1005 | pub fn tool_is_named(&self, other: Symbol) -> bool { |
Looks like the method is never used.
Could you check if the other methods are actually used as well?
compiler/rustc_hir/src/attrs/data_structures.rs · outdated
| 1017 | #[derive(Debug, Clone, PrintAttribute, Encodable, Decodable, HashStable_Generic)] |
|
| 1018 | enum LintAttrTool { |
|
| 1019 | Present { tool_name: Symbol, full_lint: Symbol }, |
|
| 1020 | NoTool, |
Why not Option?
compiler/rustc_hir/src/attrs/data_structures.rs · outdated
| 963 | None => LintAttrTool::NoTool, |
|
| 964 | }; |
|
| 965 | |
|
| 966 | Self { original_name, span, lint_index, lint_name, kind } |
Could you avoid using Self in non-generic contexts? At least in constructors like this.
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.
compiler/rustc_hir/src/attrs/data_structures.rs · outdated
| 941 | impl LintInstance { |
|
| 942 | pub fn new( |
|
| 943 | original_name: Symbol, |
|
| 944 | long_lint_name: String, |
| long_lint_name: String, | |
| long_lint_name: Symbol, |
compiler/rustc_hir/src/attrs/data_structures.rs · outdated
| 957 | }; |
|
| 958 | let kind = match tool_name { |
|
| 959 | Some(tool_name) => { |
|
| 960 | let full_lint = Symbol::intern(&format!("{tool_name}::{lint_name}",)); |
What's the difference between long_lint_name and full_lint?
no difference, good catch!
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.
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%)
Will do some more review later, some initial comments
tests/ui/parser/issues/issue-104620.rs · outdated
| 2 | 2 | |
| 3 | #![rustc_dummy=5z] //~ ERROR invalid suffix `z` for number literal |
|
| 3 | #![rustc_dummy=5z] |
|
| 4 | //~^ ERROR invalid suffix `z` for number literal |
Why was this test changed?
I think I made this change during a time when I was getting some duplicate errors, and I just forgot to remove this formatting change back to it's original. now it's reverted
tests/ui/lint/rfc-2383-lint-reason/multiple_expect_attrs.rs · outdated
| 2 | 2 | |
| 3 | 3 | #![warn(unused)] |
| 4 | 4 | |
| 5 | #[warn(unused_variables)] |
Why was this test changed?
change has been reverted, I forget why it was changed
src/tools/clippy/clippy_lints/src/returns/needless_return.rs · resolved
| 186 | && let Some(lst) = metas |
|
| 187 | && let [MetaItemInner::MetaItem(meta_item), ..] = lst.as_slice() |
|
| 188 | && let [tool, lint_name] = meta_item.path.segments.as_slice() |
|
| 189 | && tool.ident.name == sym::clippy |
The tool.ident.name == sym::clippy is missing from the right-hand side
added this back :)
Reminder, once the PR becomes ready for a review, use @rustbot ready.
compiler/rustc_passes/src/check_attr.rs
| 1499 | fn check_unused_attribute(&self, hir_id: HirId, attr: &Attribute) { |
|
| 1500 | // Warn on useless empty attributes. |
|
| 1501 | // FIXME(jdonszelmann): this lint should be moved to attribute parsing, see `AcceptContext::warn_empty_attribute` |
|
| 1502 | let note = if attr.has_any_name(&[sym::feature]) |
Why is this unused check for feature added here?
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?
compiler/rustc_passes/src/check_attr.rs · resolved
| 1525 | } |
|
| 1526 | Some(ast::AttrStyle::Inner) | None => self.tcx.emit_node_span_lint( |
|
| 1442 | fn check_lint_attr(&self, hir_id: HirId, sub_attrs: &[LintAttribute]) { |
|
| 1443 | for LintAttribute { attr_span, lint_instances, attr_style, .. } in sub_attrs { |
Could you leave a FIXME in place to move this logic to attr_parsing in a future PR?
doing this in attr_parsing would require access to hir_id and TyCtxt in the attr parser, I don't think this is possible since lint attrs need to be really early parsed.
compiler/rustc_mir_build/src/builder/scope.rs · outdated
| 1299 | 1299 | } |
| 1300 | 1300 | |
| 1301 | if self.tcx.hir_attrs(id).iter().any(|attr| Level::from_attr(attr).is_some()) { |
|
| 1301 | if self |
Can you use find_attr! here?
compiler/rustc_lint_defs/src/lib.rs · outdated
| 143 | 141 | fn hash_stable(&self, hcx: &mut Hcx, hasher: &mut StableHasher) { |
| 144 | 142 | match self { |
| 145 | LintExpectationId::Stable { hir_id, attr_index, lint_index: Some(lint_index) } => { |
|
| 143 | LintExpectationId::Stable { hir_id, attr_index, lint_index, .. } => { |
I think the , .. here and in the function below don't do anything right?
compiler/rustc_attr_parsing/src/attributes/lint.rs · resolved
| 303 | unreachable!("tools required while parsing attributes"); |
|
| 304 | }; |
|
| 305 | if tools.is_empty() { |
|
| 306 | unreachable!("tools should never be empty") |
Wondering why tools should never be empty?
because the compiler always registers some tools, very early on, way before
this check is there, so if someone in the future doesn't read the comment on parse_limited for example, and wants to parse lint attrs using it (this function doesn't take in tools), the compiler will ICE and they will realise that they should probably use parse_limited_all_filtered instead
compiler/rustc_attr_parsing/src/interface.rs
| 33 | 34 | /// context, through which all attributes can be lowered. |
| 34 | 35 | pub struct AttributeParser<'sess, S: Stage = Late> { |
| 35 | pub(crate) tools: Vec<Symbol>, |
|
| 36 | 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?
I'd rather move all the other refactorings too #155691 (comment), so the core change becomes as minimal as possible.
compiler/rustc_hir_typeck/src/expr.rs
| 76 | } |
|
| 77 | } |
|
| 78 | false |
|
| 72 | 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?
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.
compiler/rustc_lint/src/context.rs
| 851 | } |
|
| 852 | } |
|
| 853 | false |
|
| 826 | self.tcx.hir_attrs(id).iter().any(hir::Attribute::has_span_without_desugaring_kind) |
Why this change?
see my response to the comment above
View on GitHub
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.
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.
compiler/rustc_attr_parsing/src/interface.rs
| 231 | 258 | &mut emit_lint, |
| 232 | 259 | ); |
| 233 | 260 | } |
| 261 | 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.
compiler/rustc_attr_parsing/src/interface.rs
| 140 | 146 | |
| 147 | /// This method provides the same functionality as [`parse_limited_all`](Self::parse_limited_all) except filtered, |
|
| 148 | /// making sure that only allow-listed symbols are parsed |
|
| 149 | 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.
compiler/rustc_attr_parsing/src/parser.rs
| 150 | | [sym::deny] |
|
| 151 | | [sym::expect] |
|
| 152 | | [sym::forbid] |
|
| 153 | | [sym::warn] |
What is the special relation between lint attributes and incorrect delimiters?
Why shouldn't the delimiters be reported for lint attributes specifically?
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
compiler/rustc_attr_parsing/src/attributes/lint.rs
| 14 | |
|
| 15 | pub(crate) trait Lint { |
|
| 16 | const KIND: LintAttributeKind; |
|
| 17 | 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).
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.
compiler/rustc_attr_parsing/src/attributes/lint.rs
| 123 | |
|
| 124 | fn finalize(mut self, _cx: &FinalizeContext<'_, '_, S>) -> Option<AttributeKind> { |
|
| 125 | if !self.lint_attrs.is_empty() { |
|
| 126 | // Sort to ensure correct order operations later |
Span ordering shouldn't generally be relied upon for anything besides displaying diagnostics.
Just tested and this sort actually isn't necessary, which is nice :)
compiler/rustc_lint/src/early.rs
| 391 | &[] |
|
| 323 | 392 | } |
| 324 | 393 | fn check<'ecx, 'tcx, T: EarlyLintPass>(self, cx: &mut EarlyContextAndPass<'ecx, 'tcx, T>) { |
| 325 | walk_list!(cx, visit_attribute, self.1); |
I'm not sure what this change achieves.
Pre-expansion lints never want to check node attributes?
compiler/rustc_lint_defs/src/lib.rs
| 131 | 129 | } |
| 132 | 130 | |
| 133 | pub fn set_lint_index(&mut self, new_lint_index: Option<u16>) { |
|
| 131 | pub fn set_lint_index(&mut self, new_lint_index: u16) { |
This method is never used now.
get_lint_index isn't used either.
☔ The latest upstream changes (presumably #155662) made this pull request unmergeable. Please resolve the merge conflicts.
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?
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
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?
View all comments
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
r? @JonathanBrouwer
r? @jdonszelmann
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
ShouldEmitor changing theNothingvariant to have an option of whether to emit bugs or not