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

Suggest binding to a tuple if possible in the useless_let_if_seq lint #4448

Open
josephrocca opened this issue Aug 25, 2019 · 4 comments
Open
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages L-suggestion Lint: Improving, adding or fixing lint suggestions

Comments

@josephrocca
Copy link

I could be missing something here, but it seems like clippy (0.0.212 (e3cb40e 2019-06-25)) is giving a false positive for this code (useless_let_if_seq)?

fn main() {
    
    let i = 1;
    
    let mut arr0 = vec![0,0,0];
    let mut more_arrs = vec![ vec![1,1,1], vec![2,2,2], vec![3,3,3] ];
    
    let first;
    let second;
    if i == 0 {
        first = &mut arr0;
        second = &mut more_arrs[0];
    } else {
        let (start, end) = more_arrs.split_at_mut(i);
        first = &mut start[start.len()-1]; 
        second = &mut end[0];
    }
    
    println!("{:?}", first);
    println!("{:?}", second);
    
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=0cbbd72da8e9a800131ca458f1e76016

Might be similar to #2176 and #2918?

@phansch phansch added the C-bug Category: Clippy is not doing the correct thing label Aug 25, 2019
@flip1995
Copy link
Member

Couldn't this be rewritten to

    let (first, second) = if i == 0 {
        (&mut arr0, &mut more_arrs[0])
    } else {
        let (start, end) = more_arrs.split_at_mut(i);
        (&mut start[start.len()-1], &mut end[0])
    };

?

@josephrocca
Copy link
Author

@flip1995 Good point! Since Clippy's suggestion is let second = if ..., maybe this issue should remain open so that it can be corrected?

warning: `if _ { .. } else { .. }` is an expression
  --> src/main.rs:9:5
   |
9  | /     let second;
10 | |     if i == 0 {
11 | |         first = &mut arr0;
12 | |         second = &mut more_arrs[0];
...  |
16 | |         second = &mut end[0];
17 | |     }
   | |_____^ help: it is more idiomatic to write: `let second = if i == 0 { ..; &mut more_arrs[0] } else { ..; &mut end[0] };`

@flip1995 flip1995 added L-suggestion Lint: Improving, adding or fixing lint suggestions C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages and removed C-bug Category: Clippy is not doing the correct thing labels Aug 26, 2019
@flip1995
Copy link
Member

I mean the suggested code also works, but it can still be improved. So yeah, the suggestion should account for multiple bindings.

@flip1995 flip1995 changed the title False positive for useless_let_if_seq? Suggest binding to a tuple if possible in the useless_let_if_seq lint Aug 26, 2019
@josephrocca
Copy link
Author

josephrocca commented Aug 26, 2019

I also noticed that if you switch the order of the second = ... and first = ... lines in the if, or the if else, then Clippy's suggestion goes away (playground). But I imagine that will be fixed as a by-product of the above improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages L-suggestion Lint: Improving, adding or fixing lint suggestions
Projects
None yet
Development

No branches or pull requests

3 participants