From 24548560a599899e6365e4c3df86354f1f568499 Mon Sep 17 00:00:00 2001
From: Koichi ITO <koic.ito@gmail.com>
Date: Thu, 4 Mar 2021 02:11:07 +0900
Subject: [PATCH] [Fix #36] Fix a false positive for `Performance/ReverseEach`

Fixes #36.

This PR fixes a false positive for `Performance/ReverseEach`
when `each` is called on `reverse` and using the result value.
---
 CHANGELOG.md                                  |  1 +
 docs/modules/ROOT/pages/cops_performance.adoc | 12 +++++-
 lib/rubocop/cop/performance/reverse_each.rb   | 20 ++++++++-
 .../cop/performance/reverse_each_spec.rb      | 42 +++++++++++++++++++
 4 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index ee252a142b..584898a884 100644
--- a/CHANGELOG.md
+++ b/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)
 
diff --git a/docs/modules/ROOT/pages/cops_performance.adoc b/docs/modules/ROOT/pages/cops_performance.adoc
index 977a727f47..b4a4033e98 100644
--- a/docs/modules/ROOT/pages/cops_performance.adoc
+++ b/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
diff --git a/lib/rubocop/cop/performance/reverse_each.rb b/lib/rubocop/cop/performance/reverse_each.rb
index c9cfcf896b..268c4e98cf 100644
--- a/lib/rubocop/cop/performance/reverse_each.rb
+++ b/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
diff --git a/spec/rubocop/cop/performance/reverse_each_spec.rb b/spec/rubocop/cop/performance/reverse_each_spec.rb
index 424b7300cb..0f988c52db 100644
--- a/spec/rubocop/cop/performance/reverse_each_spec.rb
+++ b/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