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

False positive on 'useless_let_if_seq' #2176

Open
topecongiro opened this issue Oct 26, 2017 · 4 comments
Open

False positive on 'useless_let_if_seq' #2176

topecongiro opened this issue Oct 26, 2017 · 4 comments
Labels
S-needs-discussion Status: Needs further discussion before merging or work can be started

Comments

@topecongiro
Copy link
Contributor

When I ran cargo clippy against rustfmt repository, I got the following suggestion, which is incorrect:

warning: `if _ { .. } else { .. }` is an expression
   --> src/visitor.rs:189:9
    |
189 | /         let mut unindent_comment = self.is_if_else_block && !b.stmts.is_empty();
190 | |         if unindent_comment {
191 | |             let end_pos = source!(self, b.span).hi() - brace_compensation - remove_len;
192 | |             let snippet = self.snippet(mk_sp(self.last_pos, end_pos));
193 | |             unindent_comment = snippet.contains("//") || snippet.contains("/*");
194 | |         }
    | |_________^ help: it is more idiomatic to write: `let <mut> unindent_comment = if unindent_comment { ..; snippet.contains("//") || snippet.contains("/*") } else { self.is_if_else_block && !b.stmts.is_empty() };`
    |
    = 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.166/index.html#useless_let_if_seq

This looks similar to #975.
I am using 0.0.166.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 26, 2017

Is there an idiomatic way to rewrite this? Or should we just not lint?

@oli-obk oli-obk added the S-needs-discussion Status: Needs further discussion before merging or work can be started label Oct 26, 2017
@topecongiro
Copy link
Contributor Author

let mut cond: bool = foo();
if cond {
    // ...;
    cond = bar();
};

could be rewritten as

let cond = if foo() {
    // ...;
   bar()
} else {
    false
};

but I am not certain if I can call this idiomatic.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 26, 2017

Well... you can also do

let cond = foo() || {
    // ...;
    bar()
};

EDIT: oops. messed up the bool operator

@topecongiro
Copy link
Contributor Author

@oli-obk Thanks! And sorry for nitpicking, that changes the semantics of the original code, as we want to execute bar() iff foo() is true. What we want is:

let cond = foo() && {
    // ...;
    bar()
};

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
S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

No branches or pull requests

2 participants