rust-lang / rust-clippy

A bunch of lints to catch common mistakes and improve your Rust code. Book: https://doc.rust-lang.org/clippy/
https://rust-lang.github.io/rust-clippy/
Other
11.37k stars 1.53k forks source link

Incorrect suggestion by `useless_let_if_seq` lint #2918

Open df5602 opened 6 years ago

df5602 commented 6 years ago

Hi,

Given the following code:

fn test(idx: usize) -> bool {
    idx % 2 == 0
}

fn algo(mut x: usize, y: usize, height: usize, width: usize) {
    let mut last_row_length = 0;
    let mut sx = x;

    if last_row_length != 0 && test(y * width + x) {
        loop {
            last_row_length -= 1;
            if last_row_length == 0 {
                return;
            }
            x += 1;
            if !test(y * width + x) {
                break;
            }
        }
        sx = x;
    } else {
        while x != 0 && !test(y * width + x - 1) {
            x -= 1;

            if y != 0 && !test((y - 1) * width + x) {
                algo(x, y - 1, width, height);
            }

            last_row_length += 1;
        }
    }

    println!("{}", sx);
}

clippy suggests the following:

warning: `if _ { .. } else { .. }` is an expression
  --> src/main.rs:7:5
   |
7  | /     let mut sx = x;
8  | |
9  | |     if last_row_length != 0 && test(y * width + x) {
10 | |         loop {
...  |
30 | |         }
31 | |     }
   | |_____^ help: it is more idiomatic to write: `let <mut> sx = if last_row_length != 0 && test(y * width + x) { ..; x } else { ..; x };`
   |
   = note: #[warn(useless_let_if_seq)] on by default
   = note: you might not need `mut` at all
   = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.212/index.html#useless_let_if_seq

This is incorrect, since x is modified in the loop in the else path, but NOT assigned to sx.

(Playground link)

(Note: the code itself is not the most idiomatic, since I'm transcribing an algorithm from another language...)

sigmaSd commented 5 years ago

Hi, here is another incorrect suggestion: code:

fn main() {
    let v: Vec<String> = vec![];
    let mut x = 0;
    if v.iter().any(|s| s.len() > x) {
        x = 5;
    }

    dbg!(x);
}

suggestion:

help: it is more idiomatic to write: `let <mut> x = if v.iter().any(|s| s.len() > x) { 5 } else { 0 };`

which is wrong since x wont be initialized in the condition

repi commented 3 years ago

Ran into a case today where the suggestion generated is also incorrect, source:

        let mut changed = false;
        if self.pos != p {
            self.pos = p;
            changed = true;
        }
        if self.rot != r {
            self.rot = r;
            changed = true;
        }

        if changed {
            // do the thing
        }

suggestion:

help: it is more idiomatic to write
    |
106 |         let <mut> changed = if self.rot != r { ..; true } else { if self.pos != p {
107 |             self.pos = p;
108 |             true
109 |         } else {
110 |             false
111 |         } };

The suggestion incorrectly changes the flow control to only update self.pos when self.rot is not changed, compared to the original code

repi commented 3 years ago

Retested on latest nightly clippy 0.1.57 (aa8f2d43 2021-09-18) and this issue is still valid.