Performance/ReverseEach fix is not equivalent

[cyclotron3k]

The ReverseEach performance cop suggests replacing .reverse.each with .reverse_each.

Unfortunately reverse_each is not a drop-in replacement for reverse.each as there is a subtle difference between the two:

payments = [200, 400, 800, 100]
max_payment = payments.sort.reverse.each { |x| "do something" }.last
# => 100
max_payment = payments.sort.reverse_each { |x| "do something" }.last
# => 800

In other words, reverse_each runs the block over each element in reverse order, as expected, but then it returns the un-reversed enumerable.

Solution

It's a valid performance improvement in most scenarios, but it needs to come with a big warning. I also think that auto-correction should be disabled for this cop because it can break code.

RuboCop version

rubocop -V
0.59.2 (using Parser 2.5.1.2, running on ruby 2.5.1 x86_64-linux)

[rrosenblum]

The example that you provided is a bit of an edge case that we can account for in the cop. I find it a bit unusual to chain a method onto each since each returns the original object rather than something that has been modified. I can also see where someone would do this to simplify the code.

We should also check to make sure that reverse.each isn't being used as the return value of a method or being assigned to a variable. Both of those scenarios could produce the same issue.

[luke-hill]

This may not be desireable. But if there is a noticeable performance upgrade, could you not just tack #reverse onto the end of the original run? So you get reverse_each {}.reverse