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

FP for allow_attributes_without_reason with ? operator #10377

Closed
base0x10 opened this issue Feb 19, 2023 · 6 comments · Fixed by #10869
Closed

FP for allow_attributes_without_reason with ? operator #10377

base0x10 opened this issue Feb 19, 2023 · 6 comments · Fixed by #10869
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@base0x10
Copy link

Summary

Use of ? on results and options triggers allow_attributes_without_reason.

The specific type of the options and results in the reproduction aren't important. I first observed the FP using ? on fmt::Results.

I suspect, similar to #9630 we need to filter out findings that originate in builtins or std. That said, I don't see a specific macro or allow that is triggering this.

Lint Name

allow_attributes_without_reason

Reproducer

I tried this code:
(each line that triggers the lint will also individually trigger the lint)

#![warn(clippy::allow_attributes_without_reason)]
#![feature(lint_reasons)]

pub fn trigger_fp_option() -> Option<()>{
    Some(())?;      // Triggers lint
    None?;          // Triggers lint
    Some(())
}

pub fn trigger_fp_result() -> Result<(), &'static str> {
    Ok(())?;        // Triggers lint
    Err("asdf")?;   // Triggers lint
    Ok(())
}

I saw this happen:

warning: `allow` attribute without specifying a reason
 --> src/lib.rs:5:5
  |
5 |     Some(())?;      // Triggers lint
  |     ^^^^^^^^^
  |
  = help: try adding a reason at the end with `, reason = ".."`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#allow_attributes_without_reason
note: the lint level is defined here
 --> src/lib.rs:1:9
  |
1 | #![warn(clippy::allow_attributes_without_reason)]
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: `allow` attribute without specifying a reason
 --> src/lib.rs:6:5
  |
6 |     None?;          // Triggers lint
  |     ^^^^^
  |
  = help: try adding a reason at the end with `, reason = ".."`
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#allow_attributes_without_reason

warning: `allow` attribute without specifying a reason
  --> src/lib.rs:11:5
   |
11 |     Ok(())?;        // Triggers lint
   |     ^^^^^^^
   |
   = help: try adding a reason at the end with `, reason = ".."`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#allow_attributes_without_reason

warning: `allow` attribute without specifying a reason
  --> src/lib.rs:12:5
   |
12 |     Err("asdf")?;   // Triggers lint
   |     ^^^^^^^^^^^^
   |
   = help: try adding a reason at the end with `, reason = ".."`
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#allow_attributes_without_reason

I expected to see this happen:
No lints trigger

Version

rustc 1.69.0-nightly (4507fdaaa 2023-02-18)
binary: rustc
commit-hash: 4507fdaaa27ea2fb59a41df2ce7d1f290da53dae
commit-date: 2023-02-18
host: x86_64-unknown-linux-gnu
release: 1.69.0-nightly
LLVM version: 15.0.7

Additional Labels

No response

@base0x10 base0x10 added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Feb 19, 2023
@marmitar
Copy link

marmitar commented Mar 9, 2023

I'm hitting the same issue with simple functions like

pub fn check_warning(text: Option<&str>) -> Option<i32> {
    text?.parse().ok()
}

Versions: cargo 1.70.0-nightly (7d3033d2e 2023-03-08) and clippy 0.1.69 (900c354 2023-03-08).

@Spartan2909
Copy link

Spartan2909 commented May 20, 2023

I'm getting the same thing with the simplest example I could think of:

fn test() -> Result<(), ()> {
    Err(())?
}

Versions: cargo 1.71.0-nightly (09276c703 2023-05-16) and clippy 0.1.71 (521f4dae 2023-05-19)

@Centri3
Copy link
Member

Centri3 commented Jun 3, 2023

It seems the HIR output of any ? usage always contains #[allow(unreachable_code)], so I think this is fixed by #10869 (even though that was for procedural macros, it should still apply)

@Centri3
Copy link
Member

Centri3 commented Jun 5, 2023

Can anybody confirm this was fixed by #10869? I added a test for this and it seems fixed but want to make sure

@DoctorDalek1963
Copy link

This doesn't seem to be fixed. cargo clippy --version gives clippy 0.1.72 (2d0aa57 2023-06-18) and cargo --version gives cargo 1.72.0-nightly (0c14026aa 2023-06-14)

But the lint is still triggering on all ? operators and every function marked with tracing's #[instrument] macro, so it's still triggering on proc macros as well. Is this somehow not on nightly yet, or is this just not fixed?

@Centri3
Copy link
Member

Centri3 commented Jun 20, 2023

I don't believe the sync has happened yet, it's blocked by #112708

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 I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
5 participants