Skip to content

Commit d9dec98

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 d9dec98

File tree

5 files changed

+308
-0
lines changed

5 files changed

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

0 commit comments

Comments
 (0)