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

while_let_loop fails to see that temporary value needs to be dropped #9206

Closed
tijsvd opened this issue Jul 19, 2022 · 4 comments
Closed

while_let_loop fails to see that temporary value needs to be dropped #9206

tijsvd opened this issue Jul 19, 2022 · 4 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

@tijsvd
Copy link

tijsvd commented Jul 19, 2022

Summary

There may be important temporary values in the if/while expression, like a mutex lock guard. In case of an if-statement-with-break, these are dropped after the statement. In case of a while-let loop, these are retained for the whole loop body.

I would expect that the lint might recognise this kind of thing and not warn. Seems hard though. But blindly "fixing" can lead to bad errors.

Lint Name

while_let_loop

Reproducer

I tried this code:

use std::sync::{Arc, Mutex};

fn handle_items(q: Arc<Mutex<Vec<i32>>>) {
    loop {
        let item = match q.lock().unwrap().pop() {
            Some(item) => item,
            None => break,
        };
        // lock guard dropped here
        println!("process: {}", item);
        if item > 10 {
            // deadlocks here if while-let
            q.lock().unwrap().insert(0, item - 1);
        }
    }
}

fn main() {
    let q = Arc::new(Mutex::new(vec![1, 2, 3, 12, 7]));
    handle_items(q);
}

I saw this happen:

warning: this loop could be written as a `while let` loop
  --> src/main.rs:4:2
   |
4  | /     loop {
5  | |         let item = match q.lock().unwrap().pop() {
6  | |             Some(item) => item,
7  | |             None => break,
...  |
14 | |         }
15 | |     }
   | |_____^ help: try: `while let Some(item) = q.lock().unwrap().pop() { .. }`
   |
   = note: `#[warn(clippy::while_let_loop)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#while_let_loop

I expected to see this happen:

Nothing. The suggested code will deadlock.

Version

rustc 1.62.0 (a8314ef7d 2022-06-27)
binary: rustc
commit-hash: a8314ef7d0ec7b75c336af2c9857bfaf43002bfc
commit-date: 2022-06-27
host: x86_64-unknown-linux-gnu
release: 1.62.0
LLVM version: 14.0.5

Additional Labels

No response

@tijsvd tijsvd 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 Jul 19, 2022
@Jarcho
Copy link
Contributor

Jarcho commented Jul 19, 2022

This doesn't lint on the latest nightly.

@tijsvd
Copy link
Author

tijsvd commented Jul 19, 2022

This doesn't lint on the latest nightly.

Right. If that's deliberate and not accidental, then I guess this can be closed.

@Jarcho
Copy link
Contributor

Jarcho commented Jul 20, 2022

Was changed in #8666.

@tijsvd
Copy link
Author

tijsvd commented Jul 20, 2022

Thank you, closing.

@tijsvd tijsvd closed this as completed Jul 20, 2022
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