-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix inconsistencies in handling of inert attributes on statements #78306
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
19b91fc
to
af2449a
Compare
This lint is firing on a large number of doc comments within the compiler itself. I see several different ways of proceeding:
cc @rust-lang/rustdoc Is there any reason to have doc comments (as a opposed to regular comments) on a function if it won't be rendered by rustdoc? |
@Aaron1011 not that I'm aware of. IMO they should be changed to regular comments (1), but I don't feel very strongly about that. |
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. The `unused_doc_comments` lint now fires on a number of items in the stnadard library. I've added `#[allow(unused_doc_comments)]` to these items.
af2449a
to
ede108c
Compare
Nominating for lang team, I suspect we'll want to just let this go through (it's not really breaking) but it's an interesting question of whether these documentation comments are truly unused. |
The 'inner items' message was introduced here: https://github.com/rust-lang/rust/pull/57882/files#diff-38d78212bad0cf87e09d707ee7331a84a049fac2d8b1c7b26584580be81d818aR860-R868 So, it looks like the intention of PR #57882 was to lint doc comments on statement items - this just never worked until now. |
ede108c
to
73264b6
Compare
73264b6
to
23f4e64
Compare
r=me on the implementation. |
I'm going to split out the purely internal changes into a separate PR, with |
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
@petrochenkov: I've moved the implementation to #78326 |
…rochenkov Split out statement attributes changes from rust-lang#78306 This is the same as PR rust-lang#78306, but `unused_doc_comments` is modified to explicitly ignore statement items (which preserves the current behavior). This shouldn't have any user-visible effects, so it can be landed without lang team discussion. --------- 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
Rollup of 8 pull requests Successful merges: - rust-lang#77984 (Compute proper module parent during resolution) - rust-lang#78085 (MIR validation should check `SwitchInt` values are valid for the type) - rust-lang#78208 (replace `#[allow_internal_unstable]` with `#[rustc_allow_const_fn_unstable]` for `const fn`s) - rust-lang#78209 (Update `compiler_builtins` to 0.1.36) - rust-lang#78276 (Bump backtrace-rs to enable Mach-O support on iOS.) - rust-lang#78320 (Link to cargo's `build-std` feature instead of `xargo` in custom target docs) - rust-lang#78322 (BTreeMap: stop mistaking node::MIN_LEN for a node level constraint) - rust-lang#78326 (Split out statement attributes changes from rust-lang#78306) Failed merges: r? `@ghost`
Split out statement attributes changes from #78306 This is the same as PR rust-lang/rust#78306, but `unused_doc_comments` is modified to explicitly ignore statement items (which preserves the current behavior). This shouldn't have any user-visible effects, so it can be landed without lang team discussion. --------- 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 #78306
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)]
suppressedthe warning.
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 forStmtKind
ignores attributes forStmtKind::Item
.As a result, the
unused_doc_comments
lint will never see attributes onitem statements.
This commit makes two interrelated fixes to the handling of inert
(non-proc-macro) attributes on statements:
HasAttr
impl forStmtKind
now returns attributes forStmtKind::Item
, treating it just like every otherStmtKind
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 foritem statements.
early
andlate
lint visitors now activate lint attributes wheninvoking the callback for
Stmt
. This ensures that a lintattribute (e.g.
#[allow(unused_doc_comments)]
) can be applied tosibiling attributes on an item statement.
The
unused_doc_comments
lint now fires on a number of items in thestnadard library. I've added
#[allow(unused_doc_comments)]
to theseitems.