Add Performance/ReduceMerge cop

changelog/new_add_performancereducemerge_cop.md

@@ -0,0 +1 @@
+* [#328](https://github.com/rubocop/rubocop-performance/pull/328): Add Performance/ReduceMerge cop. ([@sambostock][])

config/default.yml

@@ -213,6 +213,11 @@ Performance/RangeInclude:
   VersionChanged: '1.7'
   Safe: false
 
+Performance/ReduceMerge:
+  Description: 'Checks that `Enumerable#reduce` and `Hash#merge` are not combined to build up a `Hash`.'
+  Enabled: pending
+  VersionAdded: '<<next>>'
+
 Performance/RedundantBlockCall:
   Description: 'Use `yield` instead of `block.call`.'
   Reference: 'https://github.com/JuanitoFatas/fast-ruby#proccall-and-block-arguments-vs-yieldcode'

lib/rubocop/cop/performance/reduce_merge.rb

@@ -0,0 +1,143 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Performance
+      # Detects use of `Enumerable#reduce` and `Hash#merge` together, which
+      # should be avoided as it needlessly copies the hash on each iteration.
+      # Using `Hash#merge!` with `reduce` is acceptable, although it is better
+      # to use `Enumerable#each_with_object` to mutate the `Hash` directly.
+      #
+      # @safety
+      #   This cop is unsafe because it cannot know if the outer method being
+      #   called is actually `Enumerable#reduce` or if the inner method is
+      #   `Hash#merge`.
+      #
+      #   # bad
+      #   [[:key, :value]].reduce({}) do |hash, (key, value)|
+      #     hash.merge(key => value)
+      #   end
+      #
+      #   # bad
+      #   [{ key: :value }].reduce({}) do |hash, element|
+      #     hash.merge(element)
+      #   end
+      #
+      #   # bad
+      #   [object].reduce({}) do |hash, element|
+      #     key, value = element.something
+      #     hash.merge(key => value)
+      #   end
+      #
+      #   # okay
+      #   [[:key, :value]].reduce({}) do |hash, (key, value)|
+      #     hash.merge!(key => value)
+      #   end
+      #
+      #   # good
+      #   [[:key, :value]].each_with_object({}) do |(key, value), hash|
+      #     hash[key] = value
+      #   end
+      #
+      #   # good
+      #   [{ key: :value }].each_with_object({}) do |element, hash|
+      #     hash.merge!(element)
+      #   end
+      #
+      #   # good
+      #   [object].each_with_object({}) do |element, hash|
+      #     key, value = element.something
+      #     hash[key] = value
+      #   end
+      #
+      class ReduceMerge < Base
+        extend AutoCorrector
+
+        MSG = 'Do not use `Hash#merge` to build new hashes within `Enumerable#reduce`. ' \
+              'Use `Enumerable#each_with_object({})` and mutate a single `Hash` instead.`'
+
+        # @!method reduce_with_merge(node)
+        def_node_matcher :reduce_with_merge, <<~PATTERN
+          (block
+            $(send _ :reduce ...)
+            $(args (arg $_) ...) 
+            {
+              (begin
+                ...
+                $(send (lvar $_) :merge ...)
+              )
+
+              $(send (lvar $_) :merge ...)
+            }
+          )
+        PATTERN
+
+        # @!method set_new?(node)
+        def_node_matcher :set_new?, <<~PATTERN
+          (send (const {nil? cbase} :Set) :new ...)
+        PATTERN
+
+        def on_block(node)
+          reduce_with_merge(node) do |reduce_send, block_args, first_block_arg, merge_send, merge_receiver|
+            return unless first_block_arg == merge_receiver
+            return if set_new?(reduce_send.first_argument)
+
+            add_offense(node) do |corrector|
+              replace_method_name(corrector, reduce_send, 'each_with_object')
+
+              # reduce passes previous element first; each_with_object passes memo object last
+              rotate_block_arguments(corrector, block_args)
+
+              replace_merge(corrector, merge_send)
+            end
+          end
+        end
+
+        private
+
+        def replace_method_name(corrector, send_node, new_method_name)
+          corrector.replace(send_node.loc.selector, new_method_name)
+        end
+
+        def rotate_block_arguments(corrector, args_node, by: 1)
+          corrector.replace(
+            args_node.source_range,
+            "|#{args_node.each_child_node.map(&:source).rotate!(by).join(', ')}|"
+          )
+        end
+
+        def replace_merge(corrector, merge_send_node)
+          receiver = merge_send_node.receiver.source
+          indentation = merge_send_node.source_range.source_line[/^\s+/]
+          replacement = merge_send_node
+                        .arguments
+                        .chunk(&:hash_type?)
+                        .flat_map { |are_hash_type, args| replacements_for_args(receiver, args, are_hash_type) }
+                        .join("\n#{indentation}")
+
+          corrector.replace(merge_send_node.source_range, replacement)
+        end
+
+        def replacements_for_args(receiver, arguments, arguments_are_hash_literals)
+          if arguments_are_hash_literals
+            replacements_for_hash_literals(receiver, arguments)
+          else
+            replacement_for_other_arguments(receiver, arguments)
+          end
+        end
+
+        def replacements_for_hash_literals(receiver, hash_literals)
+          hash_literals.flat_map do |hash|
+            hash.pairs.map do |pair|
+              "#{receiver}[#{pair.key.source}] = #{pair.value.source}"
+            end
+          end
+        end
+
+        def replacement_for_other_arguments(receiver, arguments)
+          "#{receiver}.merge!(#{arguments.map(&:source).join(', ')})"
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/performance_cops.rb

@@ -29,6 +29,7 @@
 require_relative 'performance/open_struct'
 require_relative 'performance/range_include'
 require_relative 'performance/io_readlines'
+require_relative 'performance/reduce_merge'
 require_relative 'performance/redundant_block_call'
 require_relative 'performance/redundant_equality_comparison_block'
 require_relative 'performance/redundant_match'

spec/rubocop/cop/performance/reduce_merge_spec.rb

@@ -0,0 +1,172 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Performance::ReduceMerge, :config do
+  let(:message) { RuboCop::Cop::Performance::ReduceMerge::MSG }
+
+  context 'when using `Enumerable#reduce`' do
+    context 'with `Hash#merge`' do
+      it 'registers an offense with an implicit hash literal argument' do
+        expect_offense(<<~RUBY)
+          enumerable.reduce({}) do |hash, (key, value)|
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
+            other(stuff)
+            hash.merge(key => value)
+          end
+        RUBY
+
+        expect_correction(<<~RUBY)
+          enumerable.each_with_object({}) do |(key, value), hash|
+            other(stuff)
+            hash[key] = value
+          end
+        RUBY
+      end
+
+      it 'registers an offense with an explicit hash literal argument' do
+        expect_offense(<<~RUBY)
+          enumerable.reduce({}) do |hash, (key, value)|
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
+            other(stuff)
+            hash.merge({ key => value })
+          end
+        RUBY
+
+        expect_correction(<<~RUBY)
+          enumerable.each_with_object({}) do |(key, value), hash|
+            other(stuff)
+            hash[key] = value
+          end
+        RUBY
+      end
+
+      it 'registers an offense with `Hash.new` initial value' do
+        expect_offense(<<~RUBY)
+          enumerable.reduce(Hash.new) do |hash, (key, value)|
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
+            other(stuff)
+            hash.merge(key => value)
+          end
+        RUBY
+
+        expect_correction(<<~RUBY)
+          enumerable.each_with_object(Hash.new) do |(key, value), hash|
+            other(stuff)
+            hash[key] = value
+          end
+        RUBY
+      end
+
+      it 'registers no offense with `Set.new` initial value' do
+        # Set#merge mutates the receiver, like Hash#merge!
+        expect_no_offenses(<<~RUBY)
+          enumerable.reduce(Set.new) do |set, values|
+            other(stuff)
+            set.merge(values)
+          end
+        RUBY
+      end
+
+      it 'registers an offense with many key-value pairs' do
+        expect_offense(<<~RUBY)
+          enumerable.reduce({}) do |hash, (key, value)|
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
+            other(stuff)
+            hash.merge(key => value, another => pair)
+          end
+        RUBY
+
+        expect_correction(<<~RUBY)
+          enumerable.each_with_object({}) do |(key, value), hash|
+            other(stuff)
+            hash[key] = value
+            hash[another] = pair
+          end
+        RUBY
+      end
+
+      it 'registers an offense with a hash variable argument' do
+        expect_offense(<<~RUBY)
+          enumerable.reduce({}) do |hash, element|
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
+            other(stuff)
+            hash.merge(element)
+          end
+        RUBY
+
+        expect_correction(<<~RUBY)
+          enumerable.each_with_object({}) do |element, hash|
+            other(stuff)
+            hash.merge!(element)
+          end
+        RUBY
+      end
+
+      it 'registers an offense with multiple varied arguments' do
+        expect_offense(<<~RUBY)
+          enumerable.reduce({}) do |hash, element|
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
+            other(stuff)
+            hash.merge({ k1 => v1, k2 => v2}, element, another_hash, {k3 => v3}, {k4 => v4}, yet_another_hash)
+          end
+        RUBY
+
+        expect_correction(<<~RUBY)
+          enumerable.each_with_object({}) do |element, hash|
+            other(stuff)
+            hash[k1] = v1
+            hash[k2] = v2
+            hash.merge!(element, another_hash)
+            hash[k3] = v3
+            hash[k4] = v4
+            hash.merge!(yet_another_hash)
+          end
+        RUBY
+      end
+
+      it 'registers an offense with a single line block' do
+        expect_offense(<<~RUBY)
+          enumerable.reduce({}) { |hash, (k, v)| hash.merge(k => v) }
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
+        RUBY
+
+        expect_correction(<<~RUBY)
+          enumerable.each_with_object({}) { |(k, v), hash| hash[k] = v }
+        RUBY
+      end
+
+      it 'registers an offense with a single line block and multiple keys' do
+        expect_offense(<<~RUBY)
+          enumerable.reduce({}) { |hash, (k, v)| hash.merge(k => v, foo => bar) }
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
+        RUBY
+
+        # Not this cop's responsibility to decide how to format multiline blocks
+
+        expect_correction(<<~RUBY)
+          enumerable.each_with_object({}) { |(k, v), hash| hash[k] = v
+          hash[foo] = bar }
+        RUBY
+      end
+    end
+
+    context 'with `Hash#merge!`' do
+      it 'does not register an offense' do
+        expect_no_offenses(<<~RUBY)
+          enumerable.reduce({}) do |hash, (key, value)|
+            hash.merge!(key => value)
+          end
+        RUBY
+      end
+    end
+  end
+
+  context 'when using `Enumerable#each_with_object`' do
+    it 'does not register an offense' do
+      expect_no_offenses(<<~RUBY)
+        enumerable.each_with_object({}) do |hash, (key, value)|
+          hash[key] = value
+        end
+      RUBY
+    end
+  end
+end