Merge pull request #221 from koic/fix_false_positive_for_performance_…

…reverse_each

[Fix #36] Fix a false positive for `Performance/ReverseEach`

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### Bug fixes
 
 * [#162](https://github.com/rubocop/rubocop-performance/issues/162): Fix a false positive for `Performance/RedundantBlockCall` when an optional block that is overridden by block variable. ([@koic][])
+* [#36](https://github.com/rubocop/rubocop-performance/issues/36): Fix a false positive for `Performance/ReverseEach` when `each` is called on `reverse` and using the result value. ([@koic][])
 
 ## 1.10.1 (2021-03-02)
 

docs/modules/ROOT/pages/cops_performance.adoc

@@ -1530,15 +1530,23 @@ end
 This cop is used to identify usages of `reverse.each` and
 change them to use `reverse_each` instead.
 
+If the return value is used, it will not be detected because the result will be different.
+
+[source,ruby]
+----
+[1, 2, 3].reverse.each {} #=> [3, 2, 1]
+[1, 2, 3].reverse_each {} #=> [1, 2, 3]
+----
+
 === Examples
 
 [source,ruby]
 ----
 # bad
-[].reverse.each
+items.reverse.each
 
 # good
-[].reverse_each
+items.reverse_each
 ----
 
 === References

lib/rubocop/cop/performance/reverse_each.rb

@@ -6,12 +6,20 @@ module Performance
       # This cop is used to identify usages of `reverse.each` and
       # change them to use `reverse_each` instead.
       #
+      # If the return value is used, it will not be detected because the result will be different.
+      #
+      # [source,ruby]
+      # ----
+      # [1, 2, 3].reverse.each {} #=> [3, 2, 1]
+      # [1, 2, 3].reverse_each {} #=> [1, 2, 3]
+      # ----
+      #
       # @example
       #   # bad
-      #   [].reverse.each
+      #   items.reverse.each
       #
       #   # good
-      #   [].reverse_each
+      #   items.reverse_each
       class ReverseEach < Base
         include RangeHelp
         extend AutoCorrector
@@ -24,6 +32,8 @@ class ReverseEach < Base
         MATCHER
 
         def on_send(node)
+          return if use_return_value?(node)
+
           reverse_each?(node) do
             range = offense_range(node)
 
@@ -35,6 +45,12 @@ def on_send(node)
 
         private
 
+        def use_return_value?(node)
+          !!node.ancestors.detect do |ancestor|
+            ancestor.assignment? || ancestor.send_type? || ancestor.return_type?
+          end
+        end
+
         def offense_range(node)
           range_between(node.children.first.loc.selector.begin_pos, node.loc.selector.end_pos)
         end

spec/rubocop/cop/performance/reverse_each_spec.rb

@@ -129,4 +129,46 @@ def arr
       RUBY
     end
   end
+
+  it 'does not register an offense when each is called on reverse and assign the result to lvar' do
+    expect_no_offenses(<<~RUBY)
+      ret = [1, 2, 3].reverse.each { |e| puts e }
+    RUBY
+  end
+
+  it 'does not register an offense when each is called on reverse and assign the result to ivar' do
+    expect_no_offenses(<<~RUBY)
+      @ret = [1, 2, 3].reverse.each { |e| puts e }
+    RUBY
+  end
+
+  it 'does not register an offense when each is called on reverse and assign the result to cvar' do
+    expect_no_offenses(<<~RUBY)
+      @@ret = [1, 2, 3].reverse.each { |e| puts e }
+    RUBY
+  end
+
+  it 'does not register an offense when each is called on reverse and assign the result to gvar' do
+    expect_no_offenses(<<~RUBY)
+      $ret = [1, 2, 3].reverse.each { |e| puts e }
+    RUBY
+  end
+
+  it 'does not register an offense when each is called on reverse and assign the result to constant' do
+    expect_no_offenses(<<~RUBY)
+      RET = [1, 2, 3].reverse.each { |e| puts e }
+    RUBY
+  end
+
+  it 'does not register an offense when each is called on reverse and using the result to method chain' do
+    expect_no_offenses(<<~RUBY)
+      [1, 2, 3].reverse.each { |e| puts e }.last
+    RUBY
+  end
+
+  it 'does not register an offense when each is called on reverse and returning the result' do
+    expect_no_offenses(<<~RUBY)
+      return [1, 2, 3].reverse.each { |e| puts e }
+    RUBY
+  end
 end