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

Lint for is_empty doesn't show the required negation in the error message #4304

Closed
kornelski opened this issue Jul 28, 2019 · 3 comments · Fixed by #4314
Closed

Lint for is_empty doesn't show the required negation in the error message #4304

kornelski opened this issue Jul 28, 2019 · 3 comments · Fixed by #4314
Labels
good-first-issue These issues are a good way to get started with Clippy L-suggestion Lint: Improving, adding or fixing lint suggestions

Comments

@kornelski
Copy link
Contributor

fn main() {
    let v = vec![1];
    if v.len() > 0 {        
    }
}

Lints:

| if v.len() > 0 {
| ^^^^^^^^^^^ help: using is_empty is clearer and more explicit: !v.is_empty()

The problem is that if user only reads the first part of the message, they will accidentally flip the condition to v.is_empty(). This is a problem reported in the user forum.

It'd be safer to include negation whenever is_empty is mentioned:

help: using !is_empty is clearer and more explicit

@flip1995 flip1995 added L-suggestion Lint: Improving, adding or fixing lint suggestions good-first-issue These issues are a good way to get started with Clippy labels Jul 28, 2019
@chansuke
Copy link
Contributor

@kornelski Hi, I started working on this and updated the fn check_len() method in len_zero.rs.
In this case, do I need to update testcase?

@flip1995
Copy link
Member

flip1995 commented Jul 29, 2019

Thanks for working on this!

Yeah, the stderr files in the tests/ui/ directory represent the exact output of the compiler (except some normalizations). So every time you change something on a lint output (like in this case), you also have to change the stderr files.

You don't have to do this by hand, just run sh ./tests/ui/update-all-references.sh after running cargo +master uitest and you should see updated stderr files (git status)


BTW: Don't hesitate to open a WIP PR. Most of the time it is easier to answer questions, when we can see your code. Also travis will check automatically, whether you still have to do something (update tests, format code, fix Clippy / Compiler warnings / errors, ...)

@chansuke
Copy link
Contributor

@flip1995 Thanks so much for your comment. I got it.

bors added a commit that referenced this issue Aug 1, 2019
Add negation to `len_zero` lint to show more explicit message.

Fixes #4304 I have updated the `len_zero` to show the required negation in case of like the below case.

```
fn main() {
    let v = vec![1];
    if v.len() > 0 {
    }
}
```

changelog: Clarify suggestion of `len_zero` lint.
@bors bors closed this as completed in #4314 Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue These issues are a good way to get started with Clippy L-suggestion Lint: Improving, adding or fixing lint suggestions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants