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

False positive for clippy::question_mark #13417

Open
nojima opened this issue Sep 19, 2024 · 2 comments
Open

False positive for clippy::question_mark #13417

nojima opened this issue Sep 19, 2024 · 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

@nojima
Copy link

nojima commented Sep 19, 2024

Summary

clippy claims that let ... else should be replaced by ? in the following code. However, actually replacing it with ? results in a compile error.

Lint Name

question_mark

Reproducer

I tried this code:

#![allow(clippy::disallowed_names)]

struct Foo {
    opt_x: Option<String>,
}

fn bar(foo: &mut Foo) -> Option<String> {
    let Some(ref x) = foo.opt_x else {
        return None;
    };
    let opt_y = Some(x.clone());
    std::mem::replace(&mut foo.opt_x, opt_y)
}

I saw this happen:

❱ cargo clippy
warning: this `let...else` may be rewritten with the `?` operator
  --> src/main.rs:8:5
   |
8  | /     let Some(ref x) = foo.opt_x else {
9  | |         return None;
10 | |     };
   | |______^ help: replace it with: `let ref x = foo.opt_x?;`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#question_mark
   = note: `#[warn(clippy::question_mark)]` on by default

warning: `rust-sandbox` (bin "rust-sandbox") generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.07s

I expected to see this happen:

No warnings should be emitted, or a correct correction should be suggested.

Version

rustc 1.81.0 (eeb90cda1 2024-09-04)
binary: rustc
commit-hash: eeb90cda1969383f56a2637cbd3037bdf598841c
commit-date: 2024-09-04
host: x86_64-unknown-linux-gnu
release: 1.81.0
LLVM version: 18.1.7

Additional Labels

No response

@nojima nojima 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 19, 2024
@nojima
Copy link
Author

nojima commented Sep 19, 2024

Replacing the code as clippy suggests will result in the following compile error.

#![allow(clippy::disallowed_names)]

struct Foo {
    opt_x: Option<String>,
}

fn bar(foo: &mut Foo) -> Option<String> {
    let ref x = foo.opt_x()?; // ← replaced with ? operator
    let opt_y = Some(x.clone());
    std::mem::replace(&mut foo.opt_x, opt_y)
}
error[E0507]: cannot move out of `foo.opt_x` which is behind a mutable reference
   --> src/main.rs:8:17
    |
8   |     let ref x = foo.opt_x?;
    |                 ^^^^^^^^^-
    |                 |
    |                 `foo.opt_x` moved due to this method call
    |                 move occurs because `foo.opt_x` has type `Option<String>`, which does not implement the `Copy` trait
    |

This is because the ? operator takes ownership of opt_x. Therefore, we can avoid this error by inserting as_ref in the middle. The following code compiles:

fn bar(foo: &mut Foo) -> Option<String> {
    let x = foo.opt_x.as_ref()?; // ← inserted as_ref() here
    ...
}

In general, a sentence of the following form

let Some(ref var) = expr else { return None };

should be replaced with

let var = expr.as_ref()?;

@sanpii
Copy link

sanpii commented Oct 26, 2024

And:

let Some(ref mut var) = expr else { return None };

with:

let var = expr.as_mut()?;

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

2 participants