Skip to content
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

Inner tool attributes emit spurious unused attribute warning #81661

Closed
Nemo157 opened this issue Feb 2, 2021 · 8 comments · Fixed by #82399
Closed

Inner tool attributes emit spurious unused attribute warning #81661

Nemo157 opened this issue Feb 2, 2021 · 8 comments · Fixed by #82399
Assignees
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Nemo157
Copy link
Member

Nemo157 commented Feb 2, 2021

I tried this code:

// main.rs
mod bar;

fn main() {
    bar::foo();
}
// bar.rs
#![rustfmt::skip]

pub fn foo() { println!("unformatted") }

I expected to see this happen: No warnings and bar.rs will not get formatted by cargo fmt.

Instead, this happened: cargo build emits a warning, bar.rs is correctly not formatted by cargo fmt.

warning: unused attribute
 --> src/bar.rs:1:1
  |
1 | #![rustfmt::skip]
  | ^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_attributes)]` on by default

While trying to minimize I changed mod bar to be an inline module, but that hits the unstable custom_inner_attributes feature (#54726). So maybe rustfmt shouldn't be obeying this attribute? Either way there's an inconsistency here with whether rustc or rustfmt believe the attribute is active or not.

Meta

rustc --version --verbose:

rustc 1.50.0-nightly (0f6f2d681 2020-12-06)
binary: rustc
commit-hash: 0f6f2d681b39c5f95459cd09cb936b6ceb27cd82
commit-date: 2020-12-06
host: x86_64-unknown-linux-gnu
release: 1.50.0-nightly
@Nemo157 Nemo157 added the C-bug Category: This is a bug. label Feb 2, 2021
@ehuss
Copy link
Contributor

ehuss commented Feb 3, 2021

I believe that in other positions, tool attributes like rustfmt are only marked as "used" as a side-effect of the macro expansion machinery. From what I can tell, it looks like inner attributes on file-based modules simply aren't treated as candidates for macro expansion (presumably since that is not supported, so there is no reason to do that).

I imagine @petrochenkov might have a suggestion how how that should be handled. I'm uncertain if it makes sense to somehow just mark all tool attributes as used from within the resolver, or if the unused attributes lint should ignore tool attributes (I'm guessing the latter is harder because only the resolver knows the set of tool names AFAIK).

More details:
Attribute expansion normally hits this code block for things like #[rustfmt::skip] mod inline {}, but since file-based modules are handled later in that function here, the module attributes are not treated as macro invocation candidates.

@petrochenkov
Copy link
Contributor

Inner attributes on file-based modules should certainly participate in expansion.

Could someone check whether this is a regression from #69838 or a pre-existing issue?

@Nemo157
Copy link
Member Author

Nemo157 commented Feb 3, 2021

Before 1.44 this hits error[E0658]: non-builtin inner attributes are unstable, adding #![feature(custom_inner_attributes)] on rustc 1.43.0-nightly (823ff8cf1 2020-03-07) gives no warning.

@petrochenkov petrochenkov added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Feb 3, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Feb 3, 2021
@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 3, 2021

The regression seems pretty bad because it creates a hole in the #![feature(custom_inner_attributes)] feature gate, so some of the unstable inner attributes are now accepted.

@Nemo157
Copy link
Member Author

Nemo157 commented Feb 3, 2021

Checking some more dates; rustc 1.44.0-nightly (f4c675c47 2020-03-19) both added the warning and removed the error, so it seems likely that #69838 accidentally stabilized inner attributes (unless there was another PR touching that code in that exact nightly).

@ehuss
Copy link
Contributor

ehuss commented Feb 3, 2021

Ah. So just to be clear, here's the behavior that I see:

Inner attribute on file-based module

Proc-macro attribute:
1.43: Requires feature(custom_inner_attributes, proc_macro_hygiene), works as expected
1.44: Does not require a feature, the proc-macro is ignored, and issues an unused attribute warning.
1.51: Same as 1.44

rustfmt::skip:
1.43: Requires feature(custom_inner_attributes), no warnings with the feature
1.44: Does not require a feature, but issues an unused attribute warning
1.51: Same as 1.44

Inner attributes on inline modules

Proc-macro attribute:
1.43: Requires feature(custom_inner_attributes), works as expected
1.44: Same as 1.43
1.51: Same as 1.43

rustfmt::skip:
1.43: Requires feature(custom_inner_attributes), no warnings with the feature
1.44: Same as 1.43
1.51: Same as 1.43


It is not immediately clear to me how #69838 changed that (obviously it is touching the relevant code, but I'm uncertain how it worked prior to that, and I don't have time right now to investigate more).

@camelid camelid added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-attributes Area: Attributes (`#[…]`, `#![…]`) labels Feb 5, 2021
@apiraino apiraino added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 5, 2021
@apiraino
Copy link
Contributor

apiraino commented Feb 5, 2021

Assigning P-high as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

@petrochenkov petrochenkov self-assigned this Feb 7, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 19, 2021
ast: Keep expansion status for out-of-line module items

I.e. whether a module `mod foo;` is already loaded from a file or not.
This is a pre-requisite to correctly treating inner attributes on such modules (rust-lang#81661).

With this change AST structures for `mod` items diverge even more for AST structure for the crate root, which previously used `ast::Mod`.
Therefore this PR removes `ast::Mod` from `ast::Crate` in the first commit, these two things are sufficiently different from each other, at least at syntactic level.
Customization points for visiting a "`mod` item or crate root" were also removed from AST visitors (`fn visit_mod`).
`ast::Mod` itself was refactored away in the second commit in favor of `ItemKind::Mod(Unsafe, ModKind)`.
@petrochenkov
Copy link
Contributor

Fixed in #82399.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 24, 2021
expand: Resolve and expand inner attributes on out-of-line modules

Fixes rust-lang#81661
r? `@Aaron1011`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 24, 2021
expand: Resolve and expand inner attributes on out-of-line modules

Fixes rust-lang#81661
r? ``@Aaron1011``
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Feb 25, 2021
ast: Keep expansion status for out-of-line module items

I.e. whether a module `mod foo;` is already loaded from a file or not.
This is a pre-requisite to correctly treating inner attributes on such modules (rust-lang/rust#81661).

With this change AST structures for `mod` items diverge even more for AST structure for the crate root, which previously used `ast::Mod`.
Therefore this PR removes `ast::Mod` from `ast::Crate` in the first commit, these two things are sufficiently different from each other, at least at syntactic level.
Customization points for visiting a "`mod` item or crate root" were also removed from AST visitors (`fn visit_mod`).
`ast::Mod` itself was refactored away in the second commit in favor of `ItemKind::Mod(Unsafe, ModKind)`.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 14, 2021
expand: Resolve and expand inner attributes on out-of-line modules

Fixes rust-lang#81661
r? `@Aaron1011`
@bors bors closed this as completed in bb4cdf8 Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) C-bug Category: This is a bug. P-high High priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants