Check slow hash accessing in Array#sort by `Performance/CompareWith…

…Block`

Currently, `Performance/CompareWithBlock` cop accepts the following code.

```ruby
array.sort { |a, b| a[:foo] <=> b[:foo] }
```

But the code has same issue as `a.foo <=> b.foo`.
So, by this change, the cop registers an offense for this code.

Benchmark
========

```ruby
require 'benchmark'

array10000 = 10000.times.map do |i|
  {value: i}
end.shuffle

array100 = 100.times.map do |i|
  {value: i}
end.shuffle

array10 = 10.times.map do |i|
  {value: i}
end.shuffle

Benchmark.bm(15) do |x|
  x.report('sort_by 10000'){500.times{array10000.sort_by{|a| a[:value]}}}
  x.report('sort    10000'){500.times{array10000.sort{|a, b| a[:value] <=> b[:value]}}}

  x.report('sort_by 100'){50000.times{array100.sort_by{|a| a[:value]}}}
  x.report('sort    100'){50000.times{array100.sort{|a, b| a[:value] <=> b[:value]}}}

  x.report('sort_by 10'){500000.times{array10.sort_by{|a| a[:value]}}}
  x.report('sort    10'){500000.times{array10.sort{|a, b| a[:value] <=> b[:value]}}}
end
```

```bash
$ ruby test.rb
                      user     system      total        real
sort_by 10000     2.470000   0.000000   2.470000 (  2.480009)
sort    10000     4.790000   0.010000   4.800000 (  4.797943)
sort_by 100       1.140000   0.000000   1.140000 (  1.146861)
sort    100       1.910000   0.000000   1.910000 (  1.910717)
sort_by 10        0.880000   0.000000   0.880000 (  0.871086)
sort    10        1.000000   0.000000   1.000000 (  1.002856)
```

CHANGELOG.md

@@ -8,6 +8,7 @@
 * Add auto-correct support to `Style/MixinGrouping`. ([@rrosenblum][])
 * [#4236](https://github.com/bbatsov/rubocop/issues/4236): Add new `Rails/ApplicationJob` and `Rails/ApplicationRecord` cops. ([@tjwp][])
 * [#4078](https://github.com/bbatsov/rubocop/pull/4078): Add new `Performance/Caller` cop. ([@alpaca-tc][])
+* [#4314](https://github.com/bbatsov/rubocop/pull/4314): Check slow hash accessing in `Array#sort` by `Performance/CompareWithBlock`. ([@pocke][])
 
 ### Changes
 

lib/rubocop/cop/performance/compare_with_block.rb

@@ -12,6 +12,7 @@ module Performance
       #   array.sort { |a, b| a.foo <=> b.foo }
       #   array.max { |a, b| a.foo <=> b.foo }
       #   array.min { |a, b| a.foo <=> b.foo }
+      #   array.sort { |a, b| a[:foo] <=> b[:foo] }
       #
       #   @good
       #   array.sort_by(&:foo)
@@ -21,37 +22,82 @@ module Performance
       #   end
       #   array.max_by(&:foo)
       #   array.min_by(&:foo)
+      #   array.sort_by { |a| a[:foo] }
       class CompareWithBlock < Cop
-        MSG = 'Use `%s_by(&:%s)` instead of ' \
-              '`%s { |%s, %s| %s.%s <=> %s.%s }`.'.freeze
+        MSG = 'Use `%s_by%s` instead of ' \
+              '`%s { |%s, %s| %s <=> %s }`.'.freeze
 
         def_node_matcher :compare?, <<-END
-          (block $(send _ {:sort :min :max}) (args (arg $_a) (arg $_b)) (send (send (lvar _a) $_m) :<=> (send (lvar _b) $_m)))
+          (block
+            $(send _ {:sort :min :max})
+            (args (arg $_a) (arg $_b))
+            $send)
+        END
+
+        def_node_matcher :replaceable_body?, <<-END
+          (send
+            (send (lvar %1) $_method $...)
+            :<=>
+            (send (lvar %2) _method $...))
         END
 
         def on_block(node)
-          compare?(node) do |send, var_a, var_b, method|
-            range = compare_range(send, node)
-            compare_method = send.method_name
-            add_offense(node, range,
-                        format(MSG, compare_method, method,
-                               compare_method, var_a, var_b,
-                               var_a, method, var_b, method))
+          compare?(node) do |send, var_a, var_b, body|
+            replaceable_body?(body, var_a, var_b) do |method, args_a, args_b|
+              return unless slow_compare?(method, args_a, args_b)
+              range = compare_range(send, node)
+              add_offense(node, range,
+                          message(send, method, var_a, var_b, args_a))
+            end
           end
         end
 
         def autocorrect(node)
-          send, = *node
-
           lambda do |corrector|
-            method = node.children.last.children.last.children.last
+            send, var_a, var_b, body = compare?(node)
+            method, arg, = replaceable_body?(body, var_a, var_b)
+            replacement =
+              if method == :[]
+                "#{send.method_name}_by { |a| a[#{arg.first.source}] }"
+              else
+                "#{send.method_name}_by(&:#{method})"
+              end
             corrector.replace(compare_range(send, node),
-                              "#{send.method_name}_by(&:#{method})")
+                              replacement)
           end
         end
 
         private
 
+        def slow_compare?(method, args_a, args_b)
+          return false unless args_a == args_b
+          if method == :[]
+            return false unless args_a.size == 1
+            key = args_a.first
+            return false unless %i[sym str int].include?(key.type)
+          else
+            return false unless args_a.empty?
+          end
+          true
+        end
+
+        def message(send, method, var_a, var_b, args)
+          compare_method = send.method_name
+          if method == :[]
+            key = args.first
+            instead = " { |a| a[#{key.source}] }"
+            str_a = "#{var_a}[#{key.source}]"
+            str_b = "#{var_b}[#{key.source}]"
+          else
+            instead = "(&:#{method})"
+            str_a = "#{var_a}.#{method}"
+            str_b = "#{var_b}.#{method}"
+          end
+          format(MSG, compare_method, instead,
+                 compare_method, var_a, var_b,
+                 str_a, str_b)
+        end
+
         def compare_range(send, node)
           range_between(send.loc.selector.begin_pos, node.loc.end.end_pos)
         end

manual/cops_performance.md

@@ -123,6 +123,7 @@ This cop also checks `max` and `min` methods.
 array.sort { |a, b| a.foo <=> b.foo }
 array.max { |a, b| a.foo <=> b.foo }
 array.min { |a, b| a.foo <=> b.foo }
+array.sort { |a, b| a[:foo] <=> b[:foo] }
 
 # good
 array.sort_by(&:foo)
@@ -132,6 +133,7 @@ array.sort_by do |var|
 end
 array.max_by(&:foo)
 array.min_by(&:foo)
+array.sort_by { |a| a[:foo] }
 ```
 
 ## Performance/Count

spec/rubocop/cop/performance/compare_with_block_spec.rb

@@ -9,6 +9,26 @@
       expect(cop.offenses.size).to eq(1)
     end
 
+    it "registers an offense for #{method} with [:foo]" do
+      inspect_source(cop, "array.#{method} { |a, b| a[:foo] <=> b[:foo] }")
+      expect(cop.offenses.size).to eq(1)
+    end
+
+    it "registers an offense for #{method} with ['foo']" do
+      inspect_source(cop, "array.#{method} { |a, b| a['foo'] <=> b['foo'] }")
+      expect(cop.offenses.size).to eq(1)
+    end
+
+    it "registers an offense for #{method} with [1]" do
+      inspect_source(cop, "array.#{method} { |a, b| a[1] <=> b[1] }")
+      expect(cop.offenses.size).to eq(1)
+    end
+
+    it 'highlights compare method' do
+      inspect_source(cop, "array.#{method} { |a, b| a.foo <=> b.foo }")
+      expect(cop.highlights).to eq(["#{method} { |a, b| a.foo <=> b.foo }"])
+    end
+
     it "accepts valid #{method} usage" do
       inspect_source(cop, "array.#{method} { |a, b| b <=> a }")
       expect(cop.offenses).to be_empty
@@ -45,12 +65,44 @@
       expect(new_source).to eq "array.#{method}_by(&:foo)\n"
     end
 
+    it "autocorrects array.#{method} { |a, b| a[:foo] <=> b[:foo] }" do
+      new_source = autocorrect_source(
+        cop,
+        "array.#{method} { |a, b| a[:foo] <=> b[:foo] }"
+      )
+      expect(new_source).to eq "array.#{method}_by { |a| a[:foo] }"
+    end
+
+    it "autocorrects array.#{method} { |a, b| a['foo'] <=> b['foo'] }" do
+      new_source = autocorrect_source(
+        cop,
+        "array.#{method} { |a, b| a['foo'] <=> b['foo'] }"
+      )
+      expect(new_source).to eq "array.#{method}_by { |a| a['foo'] }"
+    end
+
+    it "autocorrects array.#{method} { |a, b| a[1] <=> b[1] }" do
+      new_source = autocorrect_source(
+        cop,
+        "array.#{method} { |a, b| a[1] <=> b[1] }"
+      )
+      expect(new_source).to eq "array.#{method}_by { |a| a[1] }"
+    end
+
     it 'formats the error message correctly for ' \
       "array.#{method} { |a, b| a.foo <=> b.foo }" do
       inspect_source(cop, "array.#{method} { |a, b| a.foo <=> b.foo }")
       expect(cop.messages).to eq(["Use `#{method}_by(&:foo)` instead of " \
                                   "`#{method} { |a, b| a.foo <=> b.foo }`."])
     end
+
+    it 'formats the error message correctly for ' \
+      "array.#{method} { |a, b| a[:foo] <=> b[:foo] }" do
+      inspect_source(cop, "array.#{method} { |a, b| a[:foo] <=> b[:foo] }")
+      expected = ["Use `#{method}_by { |a| a[:foo] }` instead of " \
+                  "`#{method} { |a, b| a[:foo] <=> b[:foo] }`."]
+      expect(cop.messages).to eq(expected)
+    end
   end
 
   include_examples 'compare with block', 'sort'