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

question_mark suggestion changes code semantics #9518

Open
divagant-martian opened this issue Sep 22, 2022 · 2 comments
Open

question_mark suggestion changes code semantics #9518

divagant-martian opened this issue Sep 22, 2022 · 2 comments
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

@divagant-martian
Copy link

divagant-martian commented Sep 22, 2022

Summary

If let expression that produces a result is suggested to change to ?, changing the semantics of the code

Lint Name

question_mark

Reproducer

I tried this code:

fn do_stuff() -> Result<(), ()> {
    let result = if let Err(e) = op1() { Err(e) } else { Ok(()) };

    do_something_with_result(&result);

    result
}

fn op1() -> Result<(), ()> {
    Err(())
}

fn do_something_with_result(arg: &Result<(), ()>) {
    println!("doing something with the result {arg:?}");
}

fn main() {
    do_stuff().unwrap();
}

}

I saw this happen:

warning: this block may be rewritten with the `?` operator
 --> src/main.rs:2:18
  |
2 |     let result = if let Err(e) = op1() { Err(e) } else { Ok(()) };
  |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `op1()?`
  |
  = note: `#[warn(clippy::question_mark)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#question_mark

I expected to see this happen:
There should be no warning. Changing the code for what clippy suggests would prevent calling do_something_with_result when the result is an error

Version

rustc 1.64.0 (a55dd71d5 2022-09-19)
binary: rustc
commit-hash: a55dd71d5fb0ec5a6a3a9e8c27b2127ba491ce52
commit-date: 2022-09-19
host: x86_64-unknown-linux-gnu
release: 1.64.0
LLVM version: 14.0.6

Additional Labels

No response

@divagant-martian divagant-martian 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 Sep 22, 2022
@divagant-martian divagant-martian changed the title question_mark suggestion changed code semantics question_mark suggestion changes code semantics Sep 22, 2022
bors bot pushed a commit to sigp/lighthouse that referenced this issue Sep 23, 2022
## Issue Addressed
fixes lints from the last rust release

## Proposed Changes
Fix the lints, most of the lints by `clippy::question-mark` are false positives in the form of rust-lang/rust-clippy#9518 so it's allowed for now

## Additional Info
@kraktus
Copy link
Contributor

kraktus commented Oct 2, 2022

@rustbot claim

@kraktus kraktus removed their assignment Oct 16, 2022
@ebobrow
Copy link
Contributor

ebobrow commented Oct 25, 2022

If I'm not mistaken, it looks like there are two issues here. Clippy's suggested code wouldn't even compile because the question mark returns the inner value of the Result while the original code returns a new Result (I think related to #8281 (comment)). Do you have a reproduction where Clippy's suggestion is valid code, but we still get this semantics error? Or is this issue fixed by fixing the other false positive?

bors added a commit that referenced this issue Oct 27, 2022
`question_mark` don't lint on `if let Err` with `else`

cc #9518

AFAICT the only time this would be a valid suggestion is the rather esoteric

```rust
let _ = if let Err(e) = x {
    return Err(e);
} else {
    // no side effects
    x.unwrap()
}
```

which doesn't seem worth checking to me. Please correct me if I'm missing something.

changelog: [`question_mark`] don't lint on `if let Err` with `else`
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this issue Jan 6, 2024
## Issue Addressed
fixes lints from the last rust release

## Proposed Changes
Fix the lints, most of the lints by `clippy::question-mark` are false positives in the form of rust-lang/rust-clippy#9518 so it's allowed for now

## Additional Info
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
Development

No branches or pull requests

3 participants