Rust-clippy: never_loop false positive

[crumblingstatue]

fn main() {
    let mut i = 0;
    loop {
        if i < 10 {
            i += 1;
            println!("Iteration {}", i);
            continue;
        }
        return;
    }
}

warning: this loop never actually loops
  --> src/main.rs:3:2
   |
3  |    loop {
   |  __^ starting here...
4  | |      if i < 10 {
5  | |          i += 1;
6  | |          println!("Iteration {}", i);
7  | |          continue;
8  | |      }
9  | |      return;
10 | |  }
   | |__^ ...ending here
   |
   = note: #[warn(never_loop)] on by default
   = help: for further information visit https://github.com/Manishearth/rust-clippy/wiki#never_loop

Also the wiki entry for never_loop is missing.

[mcarton]

Also the wiki entry for never_loop is missing.

We really should deprecate the wiki and use https://manishearth.github.io/rust-clippy/current/ at some point. Cc @Manishearth, @oli-obk, @llogiq, @killercup

[crumblingstatue]

At a simple glance, it seems to me that never_loop is actually very naively implemented. It basically just looks for statements that unconditionally terminate the loop, but disregards continue completely.

It might need to do a more complicated control flow analysis if it wants to be an accurate lint, or just bail out if it sees continue.

[crumblingstatue]

Here's another "interesting" example, using labelled loops:

fn main() {
    'outer: loop {
        println!("I always loop");
        loop {
            println!("I indeed never loop");
            continue 'outer; // false negative, this unconditionally redirects control flow
                             // and ends this inner loop
        }
        break; // false positive, unreachable. rustc's control flow analysis correctly detects this statement as unreachable. I wonder if clippy could tap into this power, or if it only has the AST available to do the analysis.
    }
}

[camsteffen]

Here's some more examples with match:

loop {
    match 'a' {
        'a' => continue,
        _ => (),
    }
    break
}

loop {
    return match 'a' {
        'a' => continue,
        _ => (),
    }
}

[birkenfeld]

Another instance in walkdir, with the structure:

loop {
    if pred() {
        continue;
    }
    return;
}

The lint should probably be disabled until it is either removed or made more aware of continues.

[camsteffen]

I'd like to try and fix this

[mcarton]

@camsteffen 👍 Go ahead! Don't hesitate to ask if you have any question or to open a PR with a partial fix to get some review before you have finished.

[oxalica]

It seem that #1804 doesn't fix it completely. This example mentioned above still causes the warning.

loop {
    return match 'a' {
        'a' => continue,
        _ => (),
    }
}

@camsteffen

[oli-obk]

Thanks for the report and fix @uHOOCCOOHu