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

Suggest replacing simple loops with specialized folds #2401

Open
10 tasks
killercup opened this issue Jan 25, 2018 · 3 comments
Open
10 tasks

Suggest replacing simple loops with specialized folds #2401

killercup opened this issue Jan 25, 2018 · 3 comments

Comments

@killercup
Copy link
Member

killercup commented Jan 25, 2018

A lot of simple loops can be written using fold-like methods in iterators. Clippy should have a set of lints (maybe even a lint group1) that promotes the more functional style.

Have a look at this code:

let xs = &[1, 2, 3, 4]; // <- ignore that this is a slice for a second

let mut res = vec![];
for x in xs {
    res.push(x);
}

Seasoned functional programmers recognize that is is just a fold/reduce. In Rust, you'd write it like this:

xs.iter().fold(vec![], |mut acc, x| { acc.push(x); acc })

Seasoned Rust programmers would use a specialized fold here (basically a shortcut that makes use of FromIterator):

let res: Vec<_> = xs.iter().collect();
  • clippy should suggest using .collect::<Vec<_>>() for loops that just build vectors

Let's make this a bit more complicated:

let xs = &[1, 2, 3, 4];

let mut res = vec![];
for x in xs {
    if *x > 2 { res.push(x); } 
}

This is still a pretty simple loop, and we can rewrite it like this:

let res: Vec<_> = xs.iter().filter(|&x| *x > 2).collect();

(Sadly, it's not a trivial as it first appeared to be because filter passes a reference to the closure, and we need to rewrite that in the comparison we copied from the if. Instead of |&x| *x > 2 we could also generate |x| **x > 2 or |&&x| x > 2.)

  • clippy should suggest using .filter.collect for these kinds of loops

There are of course a bunch more specialized folders we can recognize. For example:

let mut res = false;
for x in xs {
    if *x > 2 { res = true; } 
}

Isn't it wonderfully procedural? It takes 4 lines and inline mutation to find out if an item in that iterator is greater than two. How about suggesting any instead? (Also see this exisiting lint.)

let res = xs.iter().any(|x| *x > 2);
  • clippy should suggest using any instead of simple loops (that on condition match set a boolean to true and break)
  • clippy should suggest using all instead of simple loops (that on condition match set a boolean to false and break)
  • clippy should suggest using find instead of simple loops (that on condition match set an optional value to Some and break)
  • clippy should suggest using position/rposition instead of simple loops (that count iterations and on condition match store the current iteration number in a variable and break)
  • clippy should suggest using max{,_by,_by_key} instead of simple loops (that compare values and store larger one in variable)
  • clippy should suggest using min{,_by,_by_key} instead of simple loops (that compare values and store smaller one in variable)
  • clippy should suggest using sum/product instead of simple loops
  • ➡️ suggest using contains(y) instead of iter().any(|x| x == y) (Suggest replacing 'iter().any(|x| x == y)' with 'contains' where appropriate #2534)

Haven't found an open issue about that, searched for "loop", "reduce", "fold".

Footnotes

  1. Please name the group after an Office Assistant!

@shaleh
Copy link

shaleh commented Feb 9, 2018

This is a great idea. Whenever I bring up fold it is like I unveiled an unknown maths or something :-)

@scottmcm
Copy link
Member

And as a bonus, fold (or all or position or ...) is sometimes faster than for, and is never slower.

@TheIronBorn
Copy link

TheIronBorn commented Mar 17, 2018

Instead of |&x| *x > 2 we could also generate |x| **x > 2 or |&&x| x > 2

I prefer |&&x| in most situations because I find it removes the need to type *x for each mention (similar to match_ref_pats). It doesn't work for every case though.

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

No branches or pull requests

4 participants