Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Performance/CollectionLiteralInLoop to not register offenses for Array#include? that are optimized directly in Ruby. #488

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 is 1.62.0
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'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The presence or absence of this string interpolation does not change the testing result, but is this code meaningful in relation to the test's behavior?

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