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

Incorrect suggestion by useless_let_if_seq lint #2918

Open
df5602 opened this issue Jul 15, 2018 · 3 comments
Open

Incorrect suggestion by useless_let_if_seq lint #2918

df5602 opened this issue Jul 15, 2018 · 3 comments
Labels
C-bug Category: Clippy is not doing the correct thing I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied L-suggestion Lint: Improving, adding or fixing lint suggestions

Comments

@df5602
Copy link

df5602 commented Jul 15, 2018

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...)

MaksymZavershynskyi added a commit to near/nearcore that referenced this issue Nov 23, 2018
It does not understand when return, break, continue are used. Relevant: rust-lang/rust-clippy#2918
@sigmaSd
Copy link

sigmaSd commented May 30, 2019

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

@flip1995 flip1995 added L-suggestion Lint: Improving, adding or fixing lint suggestions C-bug Category: Clippy is not doing the correct thing labels May 30, 2019
bors added a commit that referenced this issue May 15, 2020
Downgrade useless_let_if_seq to nursery

I feel that this lint has the wrong balance of incorrect suggestions for a default-enabled lint.

The immediate code I faced was something like:

```rust
fn main() {
    let mut good = do1();
    if !do2() {
        good = false;
    }
    if good {
        println!("good");
    }
}

fn do1() -> bool { println!("1"); false }
fn do2() -> bool { println!("2"); false }
```

On this code Clippy calls it unidiomatic and suggests the following diff, which has different behavior in a way that I don't necessarily want.

```diff
- let mut good = do1();
- if !do2() {
-     good = false;
- }
+ let good = if !do2() {
+     false
+ } else {
+     do1()
+ };
```

On exploring issues filed about this lint, I have found that other users have also struggled with inappropriate suggestions (#4124, #3043, #2918, #2176) and suggestions that make the code worse (#3769, #2749). Overall I believe that this lint is still at nursery quality for now and should not be enabled.

---

changelog: Remove useless_let_if_seq from default set of enabled lints
bors added a commit that referenced this issue May 15, 2020
Downgrade useless_let_if_seq to nursery

I feel that this lint has the wrong balance of incorrect suggestions for a default-enabled lint.

The immediate code I faced was something like:

```rust
fn main() {
    let mut good = do1();
    if !do2() {
        good = false;
    }
    if good {
        println!("good");
    }
}

fn do1() -> bool { println!("1"); false }
fn do2() -> bool { println!("2"); false }
```

On this code Clippy calls it unidiomatic and suggests the following diff, which has different behavior in a way that I don't necessarily want.

```diff
- let mut good = do1();
- if !do2() {
-     good = false;
- }
+ let good = if !do2() {
+     false
+ } else {
+     do1()
+ };
```

On exploring issues filed about this lint, I have found that other users have also struggled with inappropriate suggestions (#4124, #3043, #2918, #2176) and suggestions that make the code worse (#3769, #2749). Overall I believe that this lint is still at nursery quality for now and should not be enabled.

---

changelog: Remove useless_let_if_seq from default set of enabled lints
@phansch phansch added the I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied label Dec 22, 2020
@repi
Copy link

repi commented Mar 8, 2021

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
Copy link

repi commented Sep 19, 2021

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

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-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied L-suggestion Lint: Improving, adding or fixing lint suggestions
Projects
None yet
Development

No branches or pull requests

5 participants