Skip to content

Commit

Permalink
[Fix #71] Add new Performance/CollectionLiteralInMethod cop
Browse files Browse the repository at this point in the history
  • Loading branch information
exoego committed Sep 4, 2022
1 parent 975d32a commit fdd3727
Show file tree
Hide file tree
Showing 5 changed files with 304 additions and 0 deletions.
1 change: 1 addition & 0 deletions changelog/new_collection_literal_in_method.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#303](https://github.com/rubocop/rubocop-performance/pull/303): Add new Performance/CollectionLiteralInMethod cop. ([@exoego][])
6 changes: 6 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,12 @@ Performance/CollectionLiteralInLoop:
# Min number of elements to consider an offense
MinSize: 1

Performance/CollectionLiteralInMethod:
Description: 'Extract Array and Hash literals outside of methods into class variables or constants.'
Enabled: 'pending'
VersionAdded: '1.15'
MinSize: 1

Performance/CompareWithBlock:
Description: 'Use `sort_by(&:foo)` instead of `sort { |a, b| a.foo <=> b.foo }`.'
Enabled: true
Expand Down
115 changes: 115 additions & 0 deletions lib/rubocop/cop/performance/collection_literal_in_method.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Performance
# Identifies places where Array and Hash literals are used within method.
# It is better to extract them into a constant to avoid unnecessary allocations
# on each iteration.
#
# You can set the minimum number of elements to consider
# an offense with `MinSize`.
#
# @example
# # bad
# def fallback_data(config)
# {
# foo: 'bar',
# sit: 'amet',
# }.merge(config)
# end
#
# # good
# FALLBACK_DATA = {
# foo: 'bar',
# sit: 'amet',
# }.freeze
#
# def fallback_data(config)
# FALLBACK_DATA.merge(config)
# end
#
class CollectionLiteralInMethod < Base
MSG = 'Avoid immutable %<literal_class>s literals in method definition. ' \
'It is better to extract it into a constant.'

ENUMERABLE_METHOD_NAMES = (Enumerable.instance_methods + [:each]).to_set.freeze
NONMUTATING_ARRAY_METHODS = %i[& * + - <=> == [] all? any? assoc at
bsearch bsearch_index collect combination
compact count cycle deconstruct difference dig
drop drop_while each each_index empty? eql?
fetch filter find_index first flatten hash
include? index inspect intersection join
last length map max min minmax none? one? pack
permutation product rassoc reject
repeated_combination repeated_permutation reverse
reverse_each rindex rotate sample select shuffle
size slice sort sum take take_while
to_a to_ary to_h to_s transpose union uniq
values_at zip |].freeze

ARRAY_METHODS = (ENUMERABLE_METHOD_NAMES | NONMUTATING_ARRAY_METHODS).to_set.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?
has_value? hash include? inspect invert key key?
keys? length member? merge rassoc rehash reject
select size slice to_a to_h to_hash to_proc to_s
transform_keys transform_values value? values values_at].freeze

HASH_METHODS = (ENUMERABLE_METHOD_NAMES | NONMUTATING_HASH_METHODS).to_set.freeze

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

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

private

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

def nonmutable_method_of_array_or_hash?(node, method)
(node.array_type? && ARRAY_METHODS.include?(method)) ||
(node.hash_type? && HASH_METHODS.include?(method))
end

def parent_is_method?(node)
node.each_ancestor.any? { |ancestor| method?(ancestor) || singleton_method?(ancestor) }
end

def method?(ancestor)
ancestor.def_type?
end

def singleton_method?(ancestor)
ancestor.defs_type?
end

def literal_class(node)
if node.array_type?
'Array'
elsif node.hash_type?
'Hash'
end
end

def enumerable_method?(method_name)
ENUMERABLE_METHOD_NAMES.include?(method_name)
end

def min_size
Integer(cop_config['MinSize'] || 1)
end
end
end
end
end
1 change: 1 addition & 0 deletions lib/rubocop/cop/performance_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
require_relative 'performance/case_when_splat'
require_relative 'performance/casecmp'
require_relative 'performance/collection_literal_in_loop'
require_relative 'performance/collection_literal_in_method'
require_relative 'performance/compare_with_block'
require_relative 'performance/concurrent_monotonic_time'
require_relative 'performance/constant_regexp'
Expand Down
181 changes: 181 additions & 0 deletions spec/rubocop/cop/performance/collection_literal_in_method_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Performance::CollectionLiteralInMethod, :config do
let(:cop_config) do
{ 'MinSize' => 1 }
end

context 'when inside `def` method definition' do
it 'registers an offense when using Array literal' do
expect_offense(<<~RUBY)
def foo(e)
[1, 2, 3].include?(e)
^^^^^^^^^ Avoid immutable Array literals in method definition. It is better to extract it into a constant.
end
RUBY
end

it 'registers an offense when using Hash literal' do
expect_offense(<<~RUBY)
def method
{ foo: :bar }.key?(:foo)
^^^^^^^^^^^^^ Avoid immutable Hash literals in method definition. It is better to extract it into a constant.
end
RUBY
end
end

context 'when inside singleton method definition' do
it 'registers an offense when using Array literal' do
expect_offense(<<~RUBY)
def bar.foo(e)
[1, 2, 3].include?(e)
^^^^^^^^^ Avoid immutable Array literals in method definition. It is better to extract it into a constant.
end
RUBY
end

it 'registers an offense when using Hash literal' do
expect_offense(<<~RUBY)
def Bar.method
{ foo: :bar }.key?(:foo)
^^^^^^^^^^^^^ Avoid immutable Hash literals in method definition. It is better to extract it into a constant.
end
RUBY
end
end

context 'when inside loop inside `def` method definition' do
it 'registers an offense when using Array literal' do
expect_offense(<<~RUBY)
def foo(e)
while i < 100
[1, 2, 3].include?(e)
^^^^^^^^^ Avoid immutable Array literals in method definition. It is better to extract it into a constant.
end
end
RUBY
end

it 'registers an offense when using Hash literal' do
expect_offense(<<~RUBY)
def method
while i < 100
{ foo: :bar }.key?(:foo)
^^^^^^^^^^^^^ Avoid immutable Hash literals in method definition. It is better to extract it into a constant.
end
end
RUBY
end
end

context 'when not inside any type of method definition' do
it 'does not register an offense when using Array literal' do
expect_no_offenses(<<~RUBY)
module Foo
[1, 2, 3].include?(e)
end
RUBY
end

it 'does not register an offense when using Hash literal' do
expect_no_offenses(<<~RUBY)
class Foo
{ foo: :bar }.key?(:foo)
end
RUBY
end
end

context 'when literal contains element of non basic type' do
it 'does not register an offense when using Array literal' do
expect_no_offenses(<<~RUBY)
def method(e)
[1, 2, variable].include?(e)
end
RUBY
end

it 'does not register an offense when using Hash literal' do
expect_no_offenses(<<~RUBY)
def method(e)
{ foo: { bar: variable } }.key?(:foo)
end
RUBY
end
end

context 'when destructive method is called' do
it 'does not register an offense when using Array literal' do
expect_no_offenses(<<~RUBY)
def method
[1, nil, 3].compact!
end
RUBY
end

it 'does not register an offense when using Hash literal' do
expect_no_offenses(<<~RUBY)
def method
{ foo: :bar, baz: nil }.select! { |_k, v| !v.nil? }
end
RUBY
end
end

context 'when none method is called' do
it 'does not register an offense when using Array literal' do
expect_no_offenses(<<~RUBY)
def method
array = [1, nil, 3]
end
RUBY
end

it 'does not register an offense when using Hash literal' do
expect_no_offenses(<<~RUBY)
def method()
hash = { foo: :bar, baz: nil }
end
RUBY
end
end

it 'does not register an offense when there are no literals in a def' do
expect_no_offenses(<<~RUBY)
def foo(x)
puts x
end
RUBY
end

it 'does not register an offense when nondestructive method is called on nonliteral' do
expect_no_offenses(<<~RUBY)
def bar(array)
array.all? { |x| x > 100 }
end
RUBY
end

context 'with MinSize of 2' do
let(:cop_config) do
{ 'MinSize' => 2 }
end

it 'does not register an offense when using Array literal' do
expect_no_offenses(<<~RUBY)
def foo(e)
[1].include?(e)
end
RUBY
end

it 'does not register an offense when using Hash literal' do
expect_no_offenses(<<~RUBY)
def foo(bar)
{ foo: :bar }.key?(bar)
end
RUBY
end
end
end

0 comments on commit fdd3727

Please sign in to comment.