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

useless_let_if_seq should not emit a warning if multiple assignments happen #2749

Open
DaanVandenBosch opened this issue May 12, 2018 · 2 comments

Comments

@DaanVandenBosch
Copy link

DaanVandenBosch commented May 12, 2018

I have code that looks like this:

let img_x: u32;
let img_y: u32;
let pattern_x: u32;
let pattern_y: u32;

if forward {
    img_x = start_x + scan_x;
    img_y = start_y + scan_y;
    pattern_x = scan_x;
    pattern_y = scan_y;
} else {
    img_x = start_x - scan_x;
    img_y = start_y - scan_y;
    pattern_x = scan_width - scan_x - 1;
    pattern_y = scan_height - scan_y - 1;
}

Am I right in thinking that this should be ok with clippy or is there actually a more idiomatic way of doing something like this?

If people agree with me, I'll fix the bug.

@jdnewman85
Copy link

jdnewman85 commented Jul 7, 2018

(New to rust, so I may be wrong.)

I think something like this:

let (img_x, img_y, pattern_x, pattern_y) = if forward {
    start_x + scan_x,
    start_y + scan_y,
    scan_x,
    scan_y
} else {
    start_x - scan_x,
    start_y - scan_y,
    scan_width - scan_x - 1,
    scan_height - scan_y - 1
}

There is a bit less typing, but primarily combining the declaration with initialization reduces the chances of creating a bug later when editing. If many more variables are added, then it'd probably be better to wrap them in something.

@TimNN
Copy link
Contributor

TimNN commented Dec 31, 2018

I'm very much in agreement with the original request in this issue. While using a tuple is possible, I think it can be much clearer to assign the variables directly in the if body. Especially since the compiler will ensure that all the variables are initialized exactly once before they are used.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants