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

allow_attributes_without_reason does nothing in Rust 1.81 #13348

Closed
danwilliams opened this issue Sep 5, 2024 · 4 comments
Closed

allow_attributes_without_reason does nothing in Rust 1.81 #13348

danwilliams opened this issue Sep 5, 2024 · 4 comments
Labels
C-bug Category: Clippy is not doing the correct thing

Comments

@danwilliams
Copy link

danwilliams commented Sep 5, 2024

Summary

I have been using clippy::allow_attributes_without_reason for some time, with the lint_reasons feature enabled under nightly. Recently, the expect issue was merged, and I was excited to see that this would all be released as stable in Rust 1.81!

However, in trying this in Rust 1.81 (released today), I am unable to get it to work. This seems strange as there should be no feature gate now (obviously features are on nightly not stable), and nightly confirms the feature is stable. So why does it not trigger under the conditions it did before?

Adding the actual reasons is accepted, so it's just the lack of any error or warning from Clippy when they are absent which is puzzling! Perhaps I am doing something wrong?

Note, I have tried the lint via Cargo.toml, source code, and -D on the command line - nothing causes it to trigger. Example below is source code as that's simplest to illustrate.

Reproducer

I tried this code:

#![forbid(clippy::allow_attributes_without_reason)]
#![deny(clippy::print_stdout)]
#![warn(clippy::allow_attributes)]

#[allow(clippy::print_stdout)]
fn main() {
    println!("Hello, world!");
}

I expected to see this happen: Clippy to tell me off for allowing without specifying a reason.

Instead, this happened: Clippy was happy, no errors. Note that I was also anticipating something about the use of allow instead of expect, but that didn't trigger either... but I've not previously used that feature.

Version

rustc 1.81.0 (eeb90cda1 2024-09-04)
binary: rustc
commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c
commit-date: 2024-09-04
host: aarch64-apple-darwin
release: 1.81.0
LLVM version: 18.1.7

Additional Labels

No response

@danwilliams danwilliams added the C-bug Category: Clippy is not doing the correct thing label Sep 5, 2024
@danwilliams
Copy link
Author

After some more experimentation, I have tracked the problem down - I had this in my Cargo.toml:

rust-version = "1.80.0"

This made it fail silently, where by "fail" I mean "do nothing". It's a bit of an odd situation, because although the lint_reasons feature is newly-stable, the two associated lints have been around for quite a while. So it seems the MSRV prevented the new feature from being applied, but there was no message.

I wonder if that's the correct behaviour, given that addingreason = "explanation" to the attribute doesn't trigger an error despite this not working in Rust 1.80? In other words:

In Rust 1.80, this fails:

#![forbid(clippy::allow_attributes_without_reason)]
#![deny(clippy::print_stdout)]
#![warn(clippy::allow_attributes)]

#[allow(clippy::print_stdout, reason = "testing")]
fn main() {
    println!("Hello, world!");
}

Output:

error[E0658]: lint reasons are experimental
 --> src/main.rs:5:31
  |
5 | #[allow(clippy::print_stdout, reason = "testing")]
  |                               ^^^^^^^^^^^^^^^^^^
  |

In Rust 1.81, it does not fail:

  • If the MSRV is not set, or is set to 1.81, it works as expected and gives warnings/errors from Clippy.
  • If the MSRV is set to 1.80, it does nothing - all passes, no warnings of any kind.

To me that seems like unexpected behaviour - I would like to see it fail if reasons are present and the MSRV is below 1.81. That would have been a clue.

Hopefully my detailing the nature of the problem here will help anyone else who finds themselves in the same situation 🙂

I'll leave this issue open for now, to let you guys decide if this behaviour counts as a bug or not.

danwilliams added a commit to dotfive/standards-rs that referenced this issue Sep 5, 2024
  - Bumped the MSRV to 1.81. This properly enables the lint_reasons
    feature, which was masked:

    rust-lang/rust-clippy#13348

  - Adjusted to pass linting against Rust 1.81.
@joshtriplett
Copy link
Member

It seems like it would help to configure one of two potential behaviors:

  1. Suppress lints that can only be fixed by upgrading to a new MSRV.
  2. Emit such lints, with a note that fixing them will raise MSRV to 1.xy.

The former is useful for a project that wants to keep its MSRV at a given level. The latter is useful for a project that wants to document its MSRV but update it whenever needed.

@Jarcho Jarcho added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 11, 2024
@Jarcho
Copy link
Contributor

Jarcho commented Sep 11, 2024

Marking as nominated for @joshtriplett's suggestion. At RustConf right now and my laptop's keyboard is acting up so I can't write up a separate issue.

The current behavior is intentional. If fixing the lint would require raising the MSRV then we don't lint.

@Jarcho Jarcho added I-nominated Issue: Nominated to be discussed at the next Clippy meeting and removed beta-nominated Nominated for backporting to the compiler in the beta channel. I-nominated Issue: Nominated to be discussed at the next Clippy meeting labels Sep 17, 2024
@Jarcho
Copy link
Contributor

Jarcho commented Sep 19, 2024

Closing in favor of #13416.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing
Projects
None yet
Development

No branches or pull requests

3 participants