[EXPERIMENT] Forbid derive attributes on invalid targets#78338
[EXPERIMENT] Forbid derive attributes on invalid targets#78338Aaron1011 wants to merge 2 commits intorust-lang:masterfrom
Conversation
When the 'early' and 'late' visitors visit an attribute target, they
activate any lint attributes (e.g. `#[allow]`) that apply to it.
This can affect warnings emitted on sibiling attributes. For example,
the following code does not produce an `unused_attributes` for
`#[inline]`, since the sibiling `#[allow(unused_attributes)]` suppressed
the warning.
```rust
trait Foo {
#[allow(unused_attributes)] #[inline] fn first();
#[inline] #[allow(unused_attributes)] fn second();
}
```
However, we do not do this for statements - instead, the lint attributes
only become active when we visit the struct nested inside `StmtKind`
(e.g. `Item`).
Currently, this is difficult to observe due to another issue - the
`HasAttrs` impl for `StmtKind` ignores attributes for `StmtKind::Item`.
As a result, the `unused_doc_comments` lint will never see attributes on
item statements.
This commit makes two interrelated fixes to the handling of inert
(non-proc-macro) attributes on statements:
* The `HasAttr` impl for `StmtKind` now returns attributes for
`StmtKind::Item`, treating it just like every other `StmtKind`
variant. The only place relying on the old behavior was macro
which has been updated to explicitly ignore attributes on item
statements. This allows the `unused_doc_comments` lint to fire for
item statements.
* The `early` and `late` lint visitors now activate lint attributes when
invoking the callback for `Stmt`. This ensures that a lint
attribute (e.g. `#[allow(unused_doc_comments)]`) can be applied to
sibiling attributes on an item statement.
For now, the `unused_doc_comments` lint is explicitly disabled on item
statements, which preserves the current behavior. The exact locatiosn
where this lint should fire are being discussed in PR rust-lang#78306
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @ghost |
|
@bors try |
|
⌛ Trying commit 4429d50 with merge ff044a3d65d25a17c3e14c1530004e1540179ba7... |
Aaron1011
changed the title
|
☀️ Try build successful - checks-actions |
|
@craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
There's a very small number of actual regressions. I'll open PRs to fix those crates |
|
Before landing this change we should at least change the lint to deny by default for a few versions. The change a priori seems significant enough (to me) to be gated on an edition boundary, but the actual impact seems low. (Let's take a look at the ICEs too.) |
I'm fine with doing this, but we'll need to introduce a new lint, since it's currently going through |
crlf0710
added
S-experimental
|
Closing in favor of #79078, which does the same thing. |
|
I don't think this needs a further deprecation period, this was a warning for very long time and the affected projects are not maintained. |
7 participants
cc #78331
Based on #78326
Opening for a Crater run