Skip to content

Commit 1d474dc

Browse files
sambostocksambostock
committed
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.
1 parent 33ee88d commit 1d474dc

File tree

5 files changed

+306
-0
lines changed

5 files changed

+306
-0
lines changed

‎changelog/new_add_performancereducemerge_cop.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
* [#328](https://github.com/rubocop/rubocop-performance/pull/328): Add Performance/ReduceMerge cop. ([@sambostock][])

‎config/default.yml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,11 @@ Performance/RangeInclude:
213213
VersionChanged: '1.7'
214214
Safe: false
215215

216+
Performance/ReduceMerge:
217+
Description: 'Checks that `Enumerable#reduce` and `Hash#merge` are not combined to build up a `Hash`.'
218+
Enabled: pending
219+
VersionAdded: '<<next>>'
220+
216221
Performance/RedundantBlockCall:
217222
Description: 'Use `yield` instead of `block.call`.'
218223
Reference: 'https://github.com/JuanitoFatas/fast-ruby#proccall-and-block-arguments-vs-yieldcode'

‎lib/rubocop/cop/performance/reduce_merge.rb

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
# frozen_string_literal: true
2+
3+
module RuboCop
4+
module Cop
5+
module Performance
6+
# Detects use of `Enumerable#reduce` and `Hash#merge` together, which
7+
# should be avoided as it needlessly copies the hash on each iteration.
8+
# Using `Hash#merge!` with `reduce` is acceptable, although it is better
9+
# to use `Enumerable#each_with_object` to mutate the `Hash` directly.
10+
#
11+
# @safety
12+
# This cop is unsafe because it cannot know if the outer method being
13+
# called is actually `Enumerable#reduce` or if the inner method is
14+
# `Hash#merge`.
15+
#
16+
# # bad
17+
# [[:key, :value]].reduce({}) do |hash, (key, value)|
18+
# hash.merge(key => value)
19+
# end
20+
#
21+
# # bad
22+
# [{ key: :value }].reduce({}) do |hash, element|
23+
# hash.merge(element)
24+
# end
25+
#
26+
# # bad
27+
# [object].reduce({}) do |hash, element|
28+
# key, value = element.something
29+
# hash.merge(key => value)
30+
# end
31+
#
32+
# # okay
33+
# [[:key, :value]].reduce({}) do |hash, (key, value)|
34+
# hash.merge!(key => value)
35+
# end
36+
#
37+
# # good
38+
# [[:key, :value]].each_with_object({}) do |(key, value), hash|
39+
# hash[key] = value
40+
# end
41+
#
42+
# # good
43+
# [{ key: :value }].each_with_object({}) do |element, hash|
44+
# hash.merge!(element)
45+
# end
46+
#
47+
# # good
48+
# [object].each_with_object({}) do |element, hash|
49+
# key, value = element.something
50+
# hash[key] = value
51+
# end
52+
#
53+
class ReduceMerge < Base
54+
extend AutoCorrector
55+
56+
MSG = 'Do not use `Hash#merge` to build new hashes within `Enumerable#reduce`. ' \
57+
'Use `Enumerable#each_with_object({})` and mutate a single `Hash` instead.`'
58+
59+
# @!method reduce_with_merge(node)
60+
def_node_matcher :reduce_with_merge, <<~PATTERN
61+
(block
62+
$(send _ :reduce ...)
63+
$(args (arg $_) ...)
64+
{
65+
(begin
66+
...
67+
$(send (lvar $_) :merge ...)
68+
)
69+
70+
$(send (lvar $_) :merge ...)
71+
}
72+
)
73+
PATTERN
74+
75+
def on_block(node)
76+
reduce_with_merge(node) do |reduce_send, block_args, first_block_arg, merge_send, merge_receiver|
77+
return unless first_block_arg == merge_receiver
78+
79+
add_offense(node) do |corrector|
80+
replace_method_name(corrector, reduce_send, 'each_with_object')
81+
82+
# reduce passes previous element first; each_with_object passes memo object last
83+
rotate_block_arguments(corrector, block_args)
84+
85+
replace_merge(corrector, merge_send)
86+
end
87+
end
88+
end
89+
90+
private
91+
92+
def replace_method_name(corrector, send_node, new_method_name)
93+
corrector.replace(send_node.loc.selector, new_method_name)
94+
end
95+
96+
def rotate_block_arguments(corrector, args_node, by: 1)
97+
corrector.replace(
98+
args_node.source_range,
99+
"|#{args_node.each_child_node.map(&:source).rotate!(by).join(', ')}|"
100+
)
101+
end
102+
103+
def replace_merge(corrector, merge_send_node)
104+
receiver = merge_send_node.receiver.source
105+
indentation = merge_send_node.source_range.source_line[/^\s+/]
106+
replacement = merge_send_node
107+
.arguments
108+
.chunk(&:hash_type?)
109+
.flat_map { |are_hash_type, args| replacements_for_args(receiver, args, are_hash_type) }
110+
.join("\n#{indentation}")
111+
112+
corrector.replace(merge_send_node.source_range, replacement)
113+
end
114+
115+
def replacements_for_args(receiver, arguments, arguments_are_hash_literals)
116+
if arguments_are_hash_literals
117+
replacements_for_hash_literals(receiver, arguments)
118+
else
119+
replacement_for_other_arguments(receiver, arguments)
120+
end
121+
end
122+
123+
def replacements_for_hash_literals(receiver, hash_literals)
124+
hash_literals.flat_map do |hash|
125+
hash.pairs.map do |pair|
126+
"#{receiver}[#{pair.key.source}] = #{pair.value.source}"
127+
end
128+
end
129+
end
130+
131+
def replacement_for_other_arguments(receiver, arguments)
132+
"#{receiver}.merge!(#{arguments.map(&:source).join(', ')})"
133+
end
134+
end
135+
end
136+
end
137+
end

‎lib/rubocop/cop/performance_cops.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
require_relative 'performance/open_struct'
3030
require_relative 'performance/range_include'
3131
require_relative 'performance/io_readlines'
32+
require_relative 'performance/reduce_merge'
3233
require_relative 'performance/redundant_block_call'
3334
require_relative 'performance/redundant_equality_comparison_block'
3435
require_relative 'performance/redundant_match'

‎spec/rubocop/cop/performance/reduce_merge_spec.rb

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
# frozen_string_literal: true
2+
3+
RSpec.describe RuboCop::Cop::Performance::ReduceMerge, :config do
4+
let(:message) { RuboCop::Cop::Performance::ReduceMerge::MSG }
5+
6+
context 'when using `Enumerable#reduce`' do
7+
context 'with `Hash#merge`' do
8+
it 'registers an offense with an implicit hash literal argument' do
9+
expect_offense(<<~RUBY)
10+
enumerable.reduce({}) do |hash, (key, value)|
11+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
12+
other(stuff)
13+
hash.merge(key => value)
14+
end
15+
RUBY
16+
17+
expect_correction(<<~RUBY)
18+
enumerable.each_with_object({}) do |(key, value), hash|
19+
other(stuff)
20+
hash[key] = value
21+
end
22+
RUBY
23+
end
24+
25+
it 'registers an offense with an explicit hash literal argument' do
26+
expect_offense(<<~RUBY)
27+
enumerable.reduce({}) do |hash, (key, value)|
28+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
29+
other(stuff)
30+
hash.merge({ key => value })
31+
end
32+
RUBY
33+
34+
expect_correction(<<~RUBY)
35+
enumerable.each_with_object({}) do |(key, value), hash|
36+
other(stuff)
37+
hash[key] = value
38+
end
39+
RUBY
40+
end
41+
42+
it 'registers an offense with `Hash.new` initial value' do
43+
expect_offense(<<~RUBY)
44+
enumerable.reduce(Hash.new) do |hash, (key, value)|
45+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
46+
other(stuff)
47+
hash.merge(key => value)
48+
end
49+
RUBY
50+
51+
expect_correction(<<~RUBY)
52+
enumerable.each_with_object(Hash.new) do |(key, value), hash|
53+
other(stuff)
54+
hash[key] = value
55+
end
56+
RUBY
57+
end
58+
59+
it 'registers an offense with many key-value pairs' do
60+
expect_offense(<<~RUBY)
61+
enumerable.reduce({}) do |hash, (key, value)|
62+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
63+
other(stuff)
64+
hash.merge(key => value, another => pair)
65+
end
66+
RUBY
67+
68+
expect_correction(<<~RUBY)
69+
enumerable.each_with_object({}) do |(key, value), hash|
70+
other(stuff)
71+
hash[key] = value
72+
hash[another] = pair
73+
end
74+
RUBY
75+
end
76+
77+
it 'registers an offense with a hash variable argument' do
78+
expect_offense(<<~RUBY)
79+
enumerable.reduce({}) do |hash, element|
80+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
81+
other(stuff)
82+
hash.merge(element)
83+
end
84+
RUBY
85+
86+
expect_correction(<<~RUBY)
87+
enumerable.each_with_object({}) do |element, hash|
88+
other(stuff)
89+
hash.merge!(element)
90+
end
91+
RUBY
92+
end
93+
94+
it 'registers an offense with multiple varied arguments' do
95+
expect_offense(<<~RUBY)
96+
enumerable.reduce({}) do |hash, element|
97+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
98+
other(stuff)
99+
hash.merge({ k1 => v1, k2 => v2}, element, another_hash, {k3 => v3}, {k4 => v4}, yet_another_hash)
100+
end
101+
RUBY
102+
103+
expect_correction(<<~RUBY)
104+
enumerable.each_with_object({}) do |element, hash|
105+
other(stuff)
106+
hash[k1] = v1
107+
hash[k2] = v2
108+
hash.merge!(element, another_hash)
109+
hash[k3] = v3
110+
hash[k4] = v4
111+
hash.merge!(yet_another_hash)
112+
end
113+
RUBY
114+
end
115+
116+
it 'registers an offense with a single line block' do
117+
expect_offense(<<~RUBY)
118+
enumerable.reduce({}) { |hash, (k, v)| hash.merge(k => v) }
119+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
120+
RUBY
121+
122+
expect_correction(<<~RUBY)
123+
enumerable.each_with_object({}) { |(k, v), hash| hash[k] = v }
124+
RUBY
125+
end
126+
127+
it 'registers an offense with a single line block and multiple keys' do
128+
expect_offense(<<~RUBY)
129+
enumerable.reduce({}) { |hash, (k, v)| hash.merge(k => v, foo => bar) }
130+
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{message}
131+
RUBY
132+
133+
# Not this cop's responsibility to decide how to format multiline blocks
134+
135+
expect_correction(<<~RUBY)
136+
enumerable.each_with_object({}) { |(k, v), hash| hash[k] = v
137+
hash[foo] = bar }
138+
RUBY
139+
end
140+
end
141+
142+
context 'with `Hash#merge!`' do
143+
it 'does not register an offense' do
144+
expect_no_offenses(<<~RUBY)
145+
enumerable.reduce({}) do |hash, (key, value)|
146+
hash.merge!(key => value)
147+
end
148+
RUBY
149+
end
150+
end
151+
end
152+
153+
context 'when using `Enumerable#each_with_object`' do
154+
it 'does not register an offense' do
155+
expect_no_offenses(<<~RUBY)
156+
enumerable.each_with_object({}) do |hash, (key, value)|
157+
hash[key] = value
158+
end
159+
RUBY
160+
end
161+
end
162+
end

0 commit comments

Comments
 (0)