From d9dec98f2d844b882f982774422a2bd16c956124 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Thu, 29 Dec 2022 13:30:32 -0500 Subject: [PATCH] Add Performance/ReduceMerge cop This cop detects and corrects building up a `Hash` using `reduce` and `merge`, which creates a new copy of the `Hash` so far on each iteration. It is preferable to mutate a single `Hash`, successively adding entries, ideally avoiding small temporary hashes as well. --- .../new_add_performancereducemerge_cop.md | 1 + config/default.yml | 5 + lib/rubocop/cop/performance/reduce_merge.rb | 137 +++++++++++++++ lib/rubocop/cop/performance_cops.rb | 1 + .../cop/performance/reduce_merge_spec.rb | 164 ++++++++++++++++++ 5 files changed, 308 insertions(+) create mode 100644 changelog/new_add_performancereducemerge_cop.md create mode 100644 lib/rubocop/cop/performance/reduce_merge.rb create mode 100644 spec/rubocop/cop/performance/reduce_merge_spec.rb diff --git a/changelog/new_add_performancereducemerge_cop.md b/changelog/new_add_performancereducemerge_cop.md new file mode 100644 index 0000000000..bc806014c4 --- /dev/null +++ b/changelog/new_add_performancereducemerge_cop.md @@ -0,0 +1 @@ +* [#x](https://github.com/rubocop/rubocop-performance/pull/x): Add Performance/ReduceMerge cop. ([@sambostock][]) diff --git a/config/default.yml b/config/default.yml index ee6aaad3d8..919e183374 100644 --- a/config/default.yml +++ b/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: '<>' + Performance/RedundantBlockCall: Description: 'Use `yield` instead of `block.call`.' Reference: 'https://github.com/JuanitoFatas/fast-ruby#proccall-and-block-arguments-vs-yieldcode' diff --git a/lib/rubocop/cop/performance/reduce_merge.rb b/lib/rubocop/cop/performance/reduce_merge.rb new file mode 100644 index 0000000000..59b3485d7f --- /dev/null +++ b/lib/rubocop/cop/performance/reduce_merge.rb @@ -0,0 +1,137 @@ +# 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 + + 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 + + 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 diff --git a/lib/rubocop/cop/performance_cops.rb b/lib/rubocop/cop/performance_cops.rb index 02a64a6300..e3bc456451 100644 --- a/lib/rubocop/cop/performance_cops.rb +++ b/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' diff --git a/spec/rubocop/cop/performance/reduce_merge_spec.rb b/spec/rubocop/cop/performance/reduce_merge_spec.rb new file mode 100644 index 0000000000..c8794a5686 --- /dev/null +++ b/spec/rubocop/cop/performance/reduce_merge_spec.rb @@ -0,0 +1,164 @@ +# frozen_string_literal: true + +require 'pry' + +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 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