diff --git a/changelog/new_collection_literal_in_method.md b/changelog/new_collection_literal_in_method.md new file mode 100644 index 0000000000..d5923de1ab --- /dev/null +++ b/changelog/new_collection_literal_in_method.md @@ -0,0 +1 @@ +* [#303](https://github.com/rubocop/rubocop-performance/pull/303): Add new Performance/CollectionLiteralInMethod cop. ([@exoego][]) diff --git a/config/default.yml b/config/default.yml index ee6aaad3d8..add37c363a 100644 --- a/config/default.yml +++ b/config/default.yml @@ -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 diff --git a/lib/rubocop/cop/performance/collection_literal_in_method.rb b/lib/rubocop/cop/performance/collection_literal_in_method.rb new file mode 100644 index 0000000000..20a610ea89 --- /dev/null +++ b/lib/rubocop/cop/performance/collection_literal_in_method.rb @@ -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 %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 diff --git a/lib/rubocop/cop/performance_cops.rb b/lib/rubocop/cop/performance_cops.rb index 02a64a6300..dd4690e61e 100644 --- a/lib/rubocop/cop/performance_cops.rb +++ b/lib/rubocop/cop/performance_cops.rb @@ -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' diff --git a/spec/rubocop/cop/performance/collection_literal_in_method_spec.rb b/spec/rubocop/cop/performance/collection_literal_in_method_spec.rb new file mode 100644 index 0000000000..923ae339a6 --- /dev/null +++ b/spec/rubocop/cop/performance/collection_literal_in_method_spec.rb @@ -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