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

Downgrade useless_let_if_seq to nursery #5599

Merged
merged 1 commit into from
May 15, 2020
Merged

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented May 14, 2020

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:

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.

- 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

@flip1995
Copy link
Member

@bors r+

Thanks!

@bors
Copy link
Collaborator

bors commented May 15, 2020

📌 Commit 95399f8 has been approved by flip1995

@bors
Copy link
Collaborator

bors commented May 15, 2020

⌛ Testing commit 95399f8 with merge 775e8f4...

bors added a commit that referenced this pull request 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
Copy link
Collaborator

bors commented May 15, 2020

💥 Test timed out

@dtolnay
Copy link
Member Author

dtolnay commented May 15, 2020

Can't tell what failed; I think this needs to be retried.

@matthiaskrgr
Copy link
Member

@bors retry

@bors
Copy link
Collaborator

bors commented May 15, 2020

⌛ Testing commit 95399f8 with merge cac9ad0...

@bors
Copy link
Collaborator

bors commented May 15, 2020

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing cac9ad0 to master...

@bors bors merged commit cac9ad0 into rust-lang:master May 15, 2020
@dtolnay dtolnay deleted the letif branch May 15, 2020 23:12
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

Successfully merging this pull request may close these issues.

None yet

4 participants