Skip to content

Commit a07c8e5

Browse files
committed
[Fix #482] Change Performance/CollectionLiteralInLoop to not register offenses for Array#include? that are optimized directly in Ruby.
Since `include?` on arrays are the only instances I ever run across this cop, I think it makes sense to add special handling for this. On Ruby 3.4 this does no array allocations: ``` require "memory_profiler" class Foo def bar Bar.new end end class Bar def baz end end foo = Foo.new report = MemoryProfiler.report do [1,2].include?(foo.bar.baz) end.pretty_print ``` Also, this code is simply slower on Ruby 3.4: ```rb require "benchmark/ips" local = [301, 302] val = 301 Benchmark.ips do |x| x.report('local') do local.include?(val) end x.report('literal') do [301, 302].include?(val) end x.compare! end ``` ``` $ ruby test.rb ruby 3.4.1 (2024-12-25 revision 48d4efcb85) +PRISM [x86_64-linux] Warming up -------------------------------------- local 1.822M i/100ms literal 2.296M i/100ms Calculating ------------------------------------- local 18.235M (± 1.6%) i/s (54.84 ns/i) - 92.906M in 5.096277s literal 22.807M (± 1.5%) i/s (43.85 ns/i) - 114.789M in 5.034289s Comparison: literal: 22806932.0 i/s local: 18235340.7 i/s - 1.25x slower ```
1 parent 2d36cac commit a07c8e5

File tree

3 files changed

+106
-4
lines changed

3 files changed

+106
-4
lines changed
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
* [#482](https://github.com/rubocop/rubocop-performance/issues/482): Change `Performance/CollectionLiteralInLoop` to not register offenses for `Array#include?` that are optimized directly in Ruby. ([@earlopain][])

lib/rubocop/cop/performance/collection_literal_in_loop.rb

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,14 @@ module Performance
1212
# You can set the minimum number of elements to consider
1313
# an offense with `MinSize`.
1414
#
15+
# NOTE: Since Ruby 3.4, certain simple arguments to `Array#include?` are
16+
# optimized directly in Ruby. This avoids allocations without changing the
17+
# code, as such no offense will be registered in those cases. Currently that
18+
# includes: strings, `self`, local variables, instance variables, and method
19+
# calls without arguments. Additionally, any number of methods can be chained:
20+
# `[1, 2, 3].include?(@foo)` and `[1, 2, 3].include?(@foo.bar.baz)` both avoid
21+
# the array allocation.
22+
#
1523
# @example
1624
# # bad
1725
# users.select do |user|
@@ -55,6 +63,8 @@ class CollectionLiteralInLoop < Base
5563

5664
ARRAY_METHODS = (ENUMERABLE_METHOD_NAMES | NONMUTATING_ARRAY_METHODS).to_set.freeze
5765

66+
ARRAY_INCLUDE_OPTIMIZED_TYPES = %i[str self lvar ivar send].freeze
67+
5868
NONMUTATING_HASH_METHODS = %i[< <= == > >= [] any? assoc compact dig
5969
each each_key each_pair each_value empty?
6070
eql? fetch fetch_values filter flatten has_key?
@@ -80,21 +90,42 @@ class CollectionLiteralInLoop < Base
8090
PATTERN
8191

8292
def on_send(node)
83-
receiver, method, = *node.children
84-
return unless check_literal?(receiver, method) && parent_is_loop?(receiver)
93+
receiver, method, *arguments = *node.children
94+
return unless check_literal?(receiver, method, arguments) && parent_is_loop?(receiver)
8595

8696
message = format(MSG, literal_class: literal_class(receiver))
8797
add_offense(receiver, message: message)
8898
end
8999

90100
private
91101

92-
def check_literal?(node, method)
102+
def check_literal?(node, method, arguments)
93103
!node.nil? &&
94104
nonmutable_method_of_array_or_hash?(node, method) &&
95105
node.children.size >= min_size &&
96-
node.recursive_basic_literal?
106+
node.recursive_basic_literal? &&
107+
!optimized_array_include?(node, method, arguments)
108+
end
109+
110+
# Since Ruby 3.4, simple arguments to Array#include? are optimized.
111+
# See https://github.com/ruby/ruby/pull/12123 for more details.
112+
# rubocop:disable Metrics/CyclomaticComplexity
113+
def optimized_array_include?(node, method, arguments)
114+
return false unless target_ruby_version >= 3.4 && node.array_type? && method == :include?
115+
# Disallow include?(1, 2)
116+
return false if arguments.count != 1
117+
118+
arg = arguments.first
119+
# Allow `include?(foo.bar.baz.bat)`
120+
while arg.send_type?
121+
return false if arg.arguments.any? # Disallow include?(foo(bar))
122+
break unless arg.receiver
123+
124+
arg = arg.receiver
125+
end
126+
ARRAY_INCLUDE_OPTIMIZED_TYPES.include?(arg.type)
97127
end
128+
# rubocop:enable Metrics/CyclomaticComplexity
98129

99130
def nonmutable_method_of_array_or_hash?(node, method)
100131
(node.array_type? && ARRAY_METHODS.include?(method)) ||

spec/rubocop/cop/performance/collection_literal_in_loop_spec.rb

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,4 +269,74 @@
269269
RUBY
270270
end
271271
end
272+
273+
context 'when Ruby >= 3.4', :ruby34 do
274+
# TODO: Remove when the minimum supported RuboCop knows about Ruby 3.4
275+
let(:ruby_version) { 3.4 }
276+
277+
it 'registers an offense for `include?` on a Hash literal' do
278+
expect_offense(<<~RUBY)
279+
each do
280+
{ foo: :bar }.include?(:foo)
281+
^^^^^^^^^^^^^ Avoid immutable Hash literals in loops. It is better to extract it into a local variable or a constant.
282+
end
283+
RUBY
284+
end
285+
286+
it 'registers an offense for other array methods' do
287+
expect_offense(<<~RUBY)
288+
each do
289+
[1, 2, 3].index(foo)
290+
^^^^^^^^^ Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant.
291+
end
292+
RUBY
293+
end
294+
295+
context 'when using an Array literal and calling `include?`' do
296+
[
297+
'"string"',
298+
'self',
299+
'local_variable',
300+
'method_call',
301+
'@instance_variable'
302+
].each do |argument|
303+
it "registers no offense when the argument is #{argument}" do
304+
expect_no_offenses(<<~RUBY)
305+
#{'local_variable = 123' if argument == 'local_variable'}
306+
array.all? do |e|
307+
[1, 2, 3].include?(#{argument})
308+
end
309+
RUBY
310+
end
311+
312+
it "registers no offense when the argument is #{argument} with method chain" do
313+
expect_no_offenses(<<~RUBY)
314+
#{'local_variable = 123' if argument == 'local_variable'}
315+
array.all? do |e|
316+
[1, 2, 3].include?(#{argument}.call)
317+
end
318+
RUBY
319+
end
320+
321+
it "registers no offense when the argument is #{argument} with double method chain" do
322+
expect_no_offenses(<<~RUBY)
323+
#{'local_variable = 123' if argument == 'local_variable'}
324+
array.all? do |e|
325+
[1, 2, 3].include?(#{argument}.call.call)
326+
end
327+
RUBY
328+
end
329+
330+
it "registers no offense when the argument is #{argument} with method chain and arguments" do
331+
expect_offense(<<~RUBY)
332+
#{'local_variable = 123' if argument == 'local_variable'}
333+
array.all? do |e|
334+
[1, 2, 3].include?(#{argument}.call(true))
335+
^^^^^^^^^ Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant.
336+
end
337+
RUBY
338+
end
339+
end
340+
end
341+
end
272342
end

0 commit comments

Comments
 (0)