Skip to content

Commit

Permalink
[Fix #482] Change Performance/CollectionLiteralInLoop to not regist…
Browse files Browse the repository at this point in the history
…er 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
```
  • Loading branch information
Earlopain committed Jan 30, 2025
1 parent 2d36cac commit a07c8e5
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 4 deletions.
1 change: 1 addition & 0 deletions changelog/change_collection_literal_include_ruby34.md
Original file line number Diff line number Diff line change
@@ -0,0 +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][])
39 changes: 35 additions & 4 deletions lib/rubocop/cop/performance/collection_literal_in_loop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@ module Performance
# You can set the minimum number of elements to consider
# an offense with `MinSize`.
#
# NOTE: Since Ruby 3.4, certain simple arguments to `Array#include?` are
# optimized directly in Ruby. This avoids allocations without changing the
# code, as such no offense will be registered in those cases. Currently that
# includes: strings, `self`, local variables, instance variables, and method
# calls without arguments. Additionally, any number of methods can be chained:
# `[1, 2, 3].include?(@foo)` and `[1, 2, 3].include?(@foo.bar.baz)` both avoid
# the array allocation.
#
# @example
# # bad
# users.select do |user|
Expand Down Expand Up @@ -55,6 +63,8 @@ class CollectionLiteralInLoop < Base

ARRAY_METHODS = (ENUMERABLE_METHOD_NAMES | NONMUTATING_ARRAY_METHODS).to_set.freeze

ARRAY_INCLUDE_OPTIMIZED_TYPES = %i[str self lvar ivar send].freeze

NONMUTATING_HASH_METHODS = %i[< <= == > >= [] any? assoc compact dig
each each_key each_pair each_value empty?
eql? fetch fetch_values filter flatten has_key?
Expand All @@ -80,21 +90,42 @@ class CollectionLiteralInLoop < Base
PATTERN

def on_send(node)
receiver, method, = *node.children
return unless check_literal?(receiver, method) && parent_is_loop?(receiver)
receiver, method, *arguments = *node.children
return unless check_literal?(receiver, method, arguments) && parent_is_loop?(receiver)

message = format(MSG, literal_class: literal_class(receiver))
add_offense(receiver, message: message)
end

private

def check_literal?(node, method)
def check_literal?(node, method, arguments)
!node.nil? &&
nonmutable_method_of_array_or_hash?(node, method) &&
node.children.size >= min_size &&
node.recursive_basic_literal?
node.recursive_basic_literal? &&
!optimized_array_include?(node, method, arguments)
end

# Since Ruby 3.4, simple arguments to Array#include? are optimized.
# See https://github.com/ruby/ruby/pull/12123 for more details.
# rubocop:disable Metrics/CyclomaticComplexity
def optimized_array_include?(node, method, arguments)
return false unless target_ruby_version >= 3.4 && node.array_type? && method == :include?
# Disallow include?(1, 2)
return false if arguments.count != 1

arg = arguments.first
# Allow `include?(foo.bar.baz.bat)`
while arg.send_type?
return false if arg.arguments.any? # Disallow include?(foo(bar))
break unless arg.receiver

arg = arg.receiver
end
ARRAY_INCLUDE_OPTIMIZED_TYPES.include?(arg.type)
end
# rubocop:enable Metrics/CyclomaticComplexity

def nonmutable_method_of_array_or_hash?(node, method)
(node.array_type? && ARRAY_METHODS.include?(method)) ||
Expand Down
70 changes: 70 additions & 0 deletions spec/rubocop/cop/performance/collection_literal_in_loop_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -269,4 +269,74 @@
RUBY
end
end

context 'when Ruby >= 3.4', :ruby34 do
# TODO: Remove when the minimum supported RuboCop knows about Ruby 3.4
let(:ruby_version) { 3.4 }

it 'registers an offense for `include?` on a Hash literal' do
expect_offense(<<~RUBY)
each do
{ foo: :bar }.include?(:foo)
^^^^^^^^^^^^^ Avoid immutable Hash literals in loops. It is better to extract it into a local variable or a constant.
end
RUBY
end

it 'registers an offense for other array methods' do
expect_offense(<<~RUBY)
each do
[1, 2, 3].index(foo)
^^^^^^^^^ Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant.
end
RUBY
end

context 'when using an Array literal and calling `include?`' do
[
'"string"',
'self',
'local_variable',
'method_call',
'@instance_variable'
].each do |argument|
it "registers no offense when the argument is #{argument}" do
expect_no_offenses(<<~RUBY)
#{'local_variable = 123' if argument == 'local_variable'}
array.all? do |e|
[1, 2, 3].include?(#{argument})
end
RUBY
end

it "registers no offense when the argument is #{argument} with method chain" do
expect_no_offenses(<<~RUBY)
#{'local_variable = 123' if argument == 'local_variable'}
array.all? do |e|
[1, 2, 3].include?(#{argument}.call)
end
RUBY
end

it "registers no offense when the argument is #{argument} with double method chain" do
expect_no_offenses(<<~RUBY)
#{'local_variable = 123' if argument == 'local_variable'}
array.all? do |e|
[1, 2, 3].include?(#{argument}.call.call)
end
RUBY
end

it "registers no offense when the argument is #{argument} with method chain and arguments" do
expect_offense(<<~RUBY)
#{'local_variable = 123' if argument == 'local_variable'}
array.all? do |e|
[1, 2, 3].include?(#{argument}.call(true))
^^^^^^^^^ Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant.
end
RUBY
end
end
end
end
end

0 comments on commit a07c8e5

Please sign in to comment.