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 false positive #1693

Open
Tracked by #12046
crumblingstatue opened this issue Apr 23, 2017 · 3 comments
Open
Tracked by #12046

while_let_loop false positive #1693

crumblingstatue opened this issue Apr 23, 2017 · 3 comments
Labels
C-bug Category: Clippy is not doing the correct thing E-medium Call for participation: Medium difficulty level problem and requires some initial experience. I-false-positive Issue: The lint was triggered on code it shouldn't have T-middle Type: Probably requires verifiying types

Comments

@crumblingstatue
Copy link

crumblingstatue commented Apr 23, 2017

fn main() {
    let mut lines = "foo".lines().peekable();
    // Gather remaining non-empty lines into a comment field.
    let mut comment = String::new();
    loop {
        if let Some(line) = lines.peek() {
            if line.is_empty() {
                break;
            } else {
                comment += line;
            }
        } else {
            break;
        }
        // Consume
        lines.next();
    }
}
warning: this loop could be written as a `while let` loop
  --> src/main.rs:5:5
   |
5  | /     loop {
6  | |         if let Some(line) = lines.peek() {
7  | |             if line.is_empty() {
8  | |                 break;
...  |
16 | |         lines.next();
17 | |     }
   | |_____^
   |
   = note: #[warn(while_let_loop)] on by default
help: try
   |     while let Some(line) = lines.peek() { .. }

This can't be trivially transformed into while let, since lines.next() is called unconditionally outside of the if let statement.

@oli-obk oli-obk added E-medium Call for participation: Medium difficulty level problem and requires some initial experience. C-bug Category: Clippy is not doing the correct thing T-middle Type: Probably requires verifiying types labels Apr 24, 2017
@ayosec
Copy link

ayosec commented Jul 4, 2017

I have the same issue. My code:

let mut level = 1;
loop {
    match blocks.peek().map(|n| is_tb(&n.data.borrow().value)) {
        Some(true) => level += 1,
        _ => break,
    }
    blocks.next();
}

Clippy suggests to change it to

while let Some(true) = blocks.peek().map(|n| is_tb(&n.data.borrow().value)) {
    ...
}

Maybe, clippy should ignore loops when peek() is used.


Full message:

warning: this loop could be written as a `while let` loop
   --> src/parser/blocks.rs:248:5
    |
248 | /     loop {
249 | |         match blocks.peek().map(|n| is_tb(&n.data.borrow().value)) {
250 | |             Some(true) => level += 1,
251 | |             _ => break,
252 | |         }
253 | |         blocks.next();
254 | |     }
    | |_____^ help: try `while let Some(true) = blocks.peek().map(|n| is_tb(&n.data.borrow().value)) { .. }`
    |
    = note: #[warn(while_let_loop)] on by default
    = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#while_let_loop

@tnielens
Copy link
Contributor

This can't be trivially transformed into while let, since lines.next() is called unconditionally outside of the if let statement.

In this case the lines.next(), or any code following the if let _ {} else {break} expression is reachable only if the pattern matches. The lint seems valid since it can be rewritten as

fn main() {
    let mut lines = "foo".lines().peekable();
    // Gather remaining non-empty lines into a comment field.
    let mut comment = String::new();
 
    while let Some(line) = lines.peek() {
        if line.is_empty() {
            break;
        } else {
            comment += line;
        }
        // Consume
        lines.next();
    }
}

Maybe the suggestion could highlight the necessary code mode.

Same case for second example:

while let Some(true) = blocks.peek().map(|n| is_tb(&n.data.borrow().value)) {
    level += 1;
    blocks.next();
}

@alexheretic
Copy link
Member

The above code doesn't compile though. This is still a false positive as while-peek-next does not compile.

However, while-peek-next does work with NLL. So maybe this lint should be disabled until nll lands in nightly default-enabled.

while-let-loop

while-let-loop-nll

@phansch phansch added the I-false-positive Issue: The lint was triggered on code it shouldn't have label Dec 18, 2020
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 E-medium Call for participation: Medium difficulty level problem and requires some initial experience. I-false-positive Issue: The lint was triggered on code it shouldn't have T-middle Type: Probably requires verifiying types
Projects
None yet
Development

No branches or pull requests

6 participants