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 single_char_pattern with serde_json::Value.find(_) #830

Closed
llogiq opened this issue Apr 2, 2016 · 6 comments · Fixed by #867
Closed

False positive on single_char_pattern with serde_json::Value.find(_) #830

llogiq opened this issue Apr 2, 2016 · 6 comments · Fixed by #867
Labels
C-bug Category: Clippy is not doing the correct thing

Comments

@llogiq
Copy link
Contributor

llogiq commented Apr 2, 2016

fn foo(v: &serde_json::Value) {
    let _ = v.find("x")...
}

will trigger the lint, however Value cannot find single characters for some reason. (Found in https://github.com/Thinkofname/steven-rust)

@oli-obk
Copy link
Contributor

oli-obk commented Apr 3, 2016

That's not really a false positive, but a hint that Value should be fixed. How about we lint on implementing a PATTERN_METHODS method on a type, but it has the wrong signature? serde already runs clippy, so we'd catch that there.

@llogiq
Copy link
Contributor Author

llogiq commented Apr 3, 2016

Yes, I am aware that serde's Value implementation is to blame. However, the user likely isn't – and as long as we don't go around fixing incomplete PATTERN_METHODS implementations, to them this is a false positive, regardless if clippy is doing the right thing. So we should document that somehow.

Linting incomplete implementations would be a good way forward. I'm doing something similar with len_zero and len_without_is_empty.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 4, 2016

can we also check if the method's generics are what we think they should be and not lint in that case?

Basically:

  1. is a generic method?
  2. has just one generic argument?
  3. generic argument is also the type of the passed argument?
  4. generic argument requires Pattern trait?
  5. lint

@jugglerchris
Copy link
Contributor

There's a similar false positive on foo.contains("X") where foo is a HashSet<String>. HashSet<T>::contains takes a T, not a Pattern.

@mcarton
Copy link
Member

mcarton commented Apr 14, 2016

Yes, the current lint ignores the type of foo. I’ve looked at that, but finding the method generics at that point is tricky.

@mcarton mcarton added the C-bug Category: Clippy is not doing the correct thing label Apr 14, 2016
@shssoichiro
Copy link
Contributor

shssoichiro commented Apr 20, 2016

When this was originally implemented, I had also suggested searching for Pattern trait as an argument. At that time it was deemed too difficult, so this lint was implemented with a list of method names to check on. Hence this issue.

I think improving this lint to search for Pattern arguments rather than a list of method names would be the safest/most complete fix, but the consensus seems to be that implementing that would be difficult. If I have free time maybe I can go back in and give this a second shot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants