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 against needless uses of collect() #3109

Merged
merged 6 commits into from Sep 4, 2018

Conversation

@shssoichiro
Copy link
Contributor

@shssoichiro shssoichiro commented Aug 31, 2018

Handles cases of .collect().len(), .collect().is_empty(), and
.collect().contains(). This lint is intended to be generic enough to
be added to at a later time with other similar patterns that could be
optimized.

Closes #3034

Copy link
Collaborator

@oli-obk oli-obk left a comment

You found a case of this in clippy! https://travis-ci.org/rust-lang-nursery/rust-clippy/jobs/422867441#L1079

Can you fix it?

fn generate_needless_collect_is_empty_sugg<'a, 'tcx>(collect_expr: &'tcx Expr, cx: &LateContext<'a, 'tcx>) -> String {
if let ExprKind::MethodCall(_, _, ref args) = collect_expr.node {
let iter = snippet(cx, args[0].span, "??");
return format!("{}.any(|_| true)", iter);

This comment has been minimized.

@oli-obk

oli-obk Aug 31, 2018
Collaborator

Is that really the best suggestion? Feels a little hacky

This comment has been minimized.

@killercup

killercup Aug 31, 2018
Member

Is it even correct? If there is any item, is_empty should return false, right?

I'd suggest .next().is_none()

This comment has been minimized.

@shssoichiro

shssoichiro Aug 31, 2018
Author Contributor

Hah, you're right. My mistake. I agree, .next().is_none() feels cleaner.

--> $DIR/needless_collect.rs:5:15
|
5 | let len = sample.iter().collect::<Vec<_>>().len();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider replacing with: `sample.iter().count()`

This comment has been minimized.

@killercup

killercup Aug 31, 2018
Member

Would be awesome if you could limit the span to only cover the collect().len() here (and similarly below) and suggest to replace only that with .count(). Iterator chains can get quite long.

if let ExprKind::MethodCall(ref chain_method, _, _) = args[0].node {
if chain_method.ident.name == "collect" && match_trait_method(cx, &args[0], &paths::ITERATOR) {
if method.ident.name == "len" {
span_lint_and_sugg(

This comment has been minimized.

@killercup

killercup Aug 31, 2018
Member

How confident are you that these suggestions can be automatically applied? When can they not be auto-applied? If you can use span_suggestion_with_applicability here, rustfix can pick it up :)

(Yes I know most clippy lints don't do that right now, but maybe we can start doing it for new ones!)

fn generate_needless_collect_is_empty_sugg<'a, 'tcx>(collect_expr: &'tcx Expr, cx: &LateContext<'a, 'tcx>) -> String {
if let ExprKind::MethodCall(_, _, ref args) = collect_expr.node {
let iter = snippet(cx, args[0].span, "??");
return format!("{}.any(|_| true)", iter);

This comment has been minimized.

@killercup

killercup Aug 31, 2018
Member

Is it even correct? If there is any item, is_empty should return false, right?

I'd suggest .next().is_none()

@killercup
Copy link
Member

@killercup killercup commented Aug 31, 2018

You found a case of this in clippy! https://travis-ci.org/rust-lang-nursery/rust-clippy/jobs/422867441#L1079

⚠️ That's actually a false positive! It checks for the number of unique items in the iterator:

https://github.com/rust-lang-nursery/rust-clippy/blob/73e8416df3fbc2872738911cdfa83662f4a2fcf8/clippy_lints/src/lifetimes.rs#L264-L267

@shssoichiro
Copy link
Contributor Author

@shssoichiro shssoichiro commented Aug 31, 2018

That's actually a false positive! It checks for the number of unique items in the iterator:

Looks like it. Ideally I think using .unique().count() from itertools is the better solution, but I'm not sure if clippy should be recommending external crates. Although it looks like internally, itertools uses a HashMap to track the seen items, so the performance difference is negligible at best in this case. I'll just update the lint to only look at Vec methods for now.

@killercup
Copy link
Member

@killercup killercup commented Aug 31, 2018

I'll just update the lint to only look at Vec methods for now.

Sounds good for a start. Using other lists or maps should be fine, too, but sets have observably different properties.

Copy link
Member

@killercup killercup left a comment

LGTM except for that stray needless_collect file

You might be able to also get rid of some of the highly indented code by using the if_chain! macro, but that can also be done in separate PR later

@oli-obk
oli-obk approved these changes Sep 3, 2018
@oli-obk oli-obk closed this Sep 3, 2018
@oli-obk oli-obk reopened this Sep 3, 2018
shssoichiro added 6 commits Aug 30, 2018
Handles cases of `.collect().len()`, `.collect().is_empty()`, and
`.collect().contains()`. This lint is intended to be generic enough to
be added to at a later time with other similar patterns that could be
optimized.

Closes #3034
@shssoichiro shssoichiro force-pushed the shssoichiro:3034-needless-collect branch from d88eb48 to f7d2aee Sep 4, 2018
@shssoichiro
Copy link
Contributor Author

@shssoichiro shssoichiro commented Sep 4, 2018

Rebased and fixed rustup issues. Also swapped out some nesting for if_chain.

@oli-obk oli-obk merged commit df646a8 into rust-lang:master Sep 4, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants