diff --git a/.rubocop.yml b/.rubocop.yml index 9bb21de3ab..3b4baaa991 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -88,6 +88,10 @@ Performance/Caller: Performance/ChainArrayAllocation: Enabled: false +Performance/CollectionLiteralInLoop: + Exclude: + - 'spec/**/*.rb' + RSpec/PredicateMatcher: EnforcedStyle: explicit diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a311bb88f..872233db97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ ### New features +* [#140](https://github.com/rubocop-hq/rubocop-performance/pull/140): Add new `Performance/CollectionLiteralInLoop` cop. ([@fatkodima][]) * [#137](https://github.com/rubocop-hq/rubocop-performance/pull/137): Add new `Performance/Sum` cop. ([@fatkodima][]) * [#141](https://github.com/rubocop-hq/rubocop-performance/pull/141): Add new `Performance/RedundantStringChars` cop. ([@fatkodima][]) * [#127](https://github.com/rubocop-hq/rubocop-performance/pull/127): Add new `Performance/IoReadlines` cop. ([@fatkodima][]) diff --git a/config/default.yml b/config/default.yml index d2d1920b0b..7f55e5cc15 100644 --- a/config/default.yml +++ b/config/default.yml @@ -49,6 +49,13 @@ Performance/ChainArrayAllocation: Enabled: false VersionAdded: '0.59' +Performance/CollectionLiteralInLoop: + Description: 'Extract Array and Hash literals outside of loops into local variables or constants.' + Enabled: true + VersionAdded: '1.7' + # Min number of elements to consider an offense + MinSize: 1 + Performance/CompareWithBlock: Description: 'Use `sort_by(&:foo)` instead of `sort { |a, b| a.foo <=> b.foo }`.' Enabled: true diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 554ce5af40..eebb4887b8 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -9,6 +9,7 @@ * xref:cops_performance.adoc#performancecasewhensplat[Performance/CaseWhenSplat] * xref:cops_performance.adoc#performancecasecmp[Performance/Casecmp] * xref:cops_performance.adoc#performancechainarrayallocation[Performance/ChainArrayAllocation] +* xref:cops_performance.adoc#performancecollectionliteralinloop[Performance/CollectionLiteralInLoop] * xref:cops_performance.adoc#performancecomparewithblock[Performance/CompareWithBlock] * xref:cops_performance.adoc#performancecount[Performance/Count] * xref:cops_performance.adoc#performancedeleteprefix[Performance/DeletePrefix] diff --git a/docs/modules/ROOT/pages/cops_performance.adoc b/docs/modules/ROOT/pages/cops_performance.adoc index 82701dd815..3910112429 100644 --- a/docs/modules/ROOT/pages/cops_performance.adoc +++ b/docs/modules/ROOT/pages/cops_performance.adoc @@ -279,6 +279,58 @@ array * https://twitter.com/schneems/status/1034123879978029057 +== Performance/CollectionLiteralInLoop + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Enabled +| Yes +| No +| 1.7 +| - +|=== + +This cop identifies places where Array and Hash literals are used +within loops. It is better to extract them into a local variable or constant +to avoid unnecessary allocations on each iteration. + +You can set the minimum number of elements to consider +an offense with `MinSize`. + +=== Examples + +[source,ruby] +---- +# bad +users.select do |user| + %i[superadmin admin].include?(user.role) +end + +# good +admin_roles = %i[superadmin admin] +users.select do |user| + admin_roles.include?(user.role) +end + +# good +ADMIN_ROLES = %i[superadmin admin] +... +users.select do |user| + ADMIN_ROLES.include?(user.role) +end +---- + +=== Configurable attributes + +|=== +| Name | Default value | Configurable values + +| MinSize +| `1` +| Integer +|=== + == Performance/CompareWithBlock |=== diff --git a/lib/rubocop/cop/performance/collection_literal_in_loop.rb b/lib/rubocop/cop/performance/collection_literal_in_loop.rb new file mode 100644 index 0000000000..c05defd4cf --- /dev/null +++ b/lib/rubocop/cop/performance/collection_literal_in_loop.rb @@ -0,0 +1,140 @@ +# frozen_string_literal: true + +require 'set' + +module RuboCop + module Cop + module Performance + # This cop identifies places where Array and Hash literals are used + # within loops. It is better to extract them into a local variable or constant + # to avoid unnecessary allocations on each iteration. + # + # You can set the minimum number of elements to consider + # an offense with `MinSize`. + # + # @example + # # bad + # users.select do |user| + # %i[superadmin admin].include?(user.role) + # end + # + # # good + # admin_roles = %i[superadmin admin] + # users.select do |user| + # admin_roles.include?(user.role) + # end + # + # # good + # ADMIN_ROLES = %i[superadmin admin] + # ... + # users.select do |user| + # ADMIN_ROLES.include?(user.role) + # end + # + class CollectionLiteralInLoop < Cop + MSG = 'Avoid immutable %s literals in loops. '\ + 'It is better to extract it into a local variable or a constant.' + + POST_CONDITION_LOOP_TYPES = %i[while_post until_post].freeze + LOOP_TYPES = (POST_CONDITION_LOOP_TYPES + %i[while until for]).freeze + + 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_node_matcher :kernel_loop?, <<~PATTERN + (block + (send {nil? (const nil? :Kernel)} :loop) + ...) + PATTERN + + def_node_matcher :enumerable_loop?, <<~PATTERN + (block + (send $_ #enumerable_method? ...) + ...) + PATTERN + + def on_send(node) + receiver, method, = *node.children + return unless check_literal?(receiver, method) && parent_is_loop?(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_loop?(node) + node.each_ancestor.any? { |ancestor| loop?(ancestor, node) } + end + + def loop?(ancestor, node) + keyword_loop?(ancestor.type) || + kernel_loop?(ancestor) || + node_within_enumerable_loop?(node, ancestor) + end + + def keyword_loop?(type) + LOOP_TYPES.include?(type) + end + + def node_within_enumerable_loop?(node, ancestor) + enumerable_loop?(ancestor) do |receiver| + receiver != node && !receiver.descendants.include?(node) + end + 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 ead5a9aae4..282aa0a821 100644 --- a/lib/rubocop/cop/performance_cops.rb +++ b/lib/rubocop/cop/performance_cops.rb @@ -9,6 +9,7 @@ require_relative 'performance/caller' require_relative 'performance/case_when_splat' require_relative 'performance/casecmp' +require_relative 'performance/collection_literal_in_loop.rb' require_relative 'performance/compare_with_block' require_relative 'performance/count' require_relative 'performance/delete_prefix' diff --git a/spec/rubocop/cop/performance/collection_literal_in_loop_spec.rb b/spec/rubocop/cop/performance/collection_literal_in_loop_spec.rb new file mode 100644 index 0000000000..29a601c7e2 --- /dev/null +++ b/spec/rubocop/cop/performance/collection_literal_in_loop_spec.rb @@ -0,0 +1,265 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Performance::CollectionLiteralInLoop, :config do + subject(:cop) { described_class.new(config) } + + let(:cop_config) do + { 'MinSize' => 1 } + end + + context 'when inside `while` loop' do + it 'registers an offense when using Array literal' do + expect_offense(<<~RUBY) + while true + [1, 2, 3].include?(e) + ^^^^^^^^^ Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant. + end + RUBY + end + + it 'registers an offense when using Hash literal' do + expect_offense(<<~RUBY) + while i < 100 + { foo: :bar }.key?(:foo) + ^^^^^^^^^^^^^ Avoid immutable Hash literals in loops. It is better to extract it into a local variable or a constant. + end + RUBY + end + end + + context 'when inside `until` loop' do + it 'registers an offense when using Array literal' do + expect_offense(<<~RUBY) + until i < 100 + [1, 2, 3].include?(e) + ^^^^^^^^^ Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant. + end + RUBY + end + + it 'registers an offense when using Hash literal' do + expect_offense(<<~RUBY) + until i < 100 + { foo: :bar }.key?(:foo) + ^^^^^^^^^^^^^ Avoid immutable Hash literals in loops. It is better to extract it into a local variable or a constant. + end + RUBY + end + end + + context 'when inside `for` loop' do + it 'registers an offense when using Array literal' do + expect_offense(<<~RUBY) + for i in 1..100 + [1, 2, 3].include?(e) + ^^^^^^^^^ Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant. + end + RUBY + end + + it 'registers an offense when using Hash literal' do + expect_offense(<<~RUBY) + for i in 1..100 + { foo: :bar }.key?(:foo) + ^^^^^^^^^^^^^ Avoid immutable Hash literals in loops. It is better to extract it into a local variable or a constant. + end + RUBY + end + end + + context 'when inside `while modifier` loop' do + it 'registers an offense when using Array literal' do + expect_offense(<<~RUBY) + begin + [1, 2, 3].include?(e) + ^^^^^^^^^ Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant. + end while i < 100 + RUBY + end + + it 'registers an offense when using Hash literal' do + expect_offense(<<~RUBY) + begin + { foo: :bar }.key?(:foo) + ^^^^^^^^^^^^^ Avoid immutable Hash literals in loops. It is better to extract it into a local variable or a constant. + end while i < 100 + RUBY + end + end + + context 'when inside `until modifier` loop' do + it 'registers an offense when using Array literal' do + expect_offense(<<~RUBY) + begin + [1, 2, 3].include?(e) + ^^^^^^^^^ Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant. + end until i < 100 + RUBY + end + + it 'registers an offense when using Hash literal' do + expect_offense(<<~RUBY) + begin + { foo: :bar }.key?(:foo) + ^^^^^^^^^^^^^ Avoid immutable Hash literals in loops. It is better to extract it into a local variable or a constant. + end until i < 100 + RUBY + end + end + + context 'when inside `Kernel#loop` loop' do + it 'registers an offense when using Array literal' do + expect_offense(<<~RUBY) + loop do + [1, 2, 3].include?(e) + ^^^^^^^^^ Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant. + end + RUBY + end + + it 'registers an offense when using Hash literal' do + expect_offense(<<~RUBY) + loop do + { foo: :bar }.key?(:foo) + ^^^^^^^^^^^^^ Avoid immutable Hash literals in loops. It is better to extract it into a local variable or a constant. + end + RUBY + end + end + + context 'when inside one of `Enumerable` loop-like methods' do + it 'registers an offense when using Array literal' do + expect_offense(<<~RUBY) + array.all? do |e| + [1, 2, 3].include?(e) + ^^^^^^^^^ Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant. + end + RUBY + end + + it 'registers an offense when using Hash literal' do + expect_offense(<<~RUBY) + array.all? do |e| + { foo: :bar }.key?(:foo) + ^^^^^^^^^^^^^ Avoid immutable Hash literals in loops. It is better to extract it into a local variable or a constant. + end + RUBY + end + end + + context 'when not inside loop' do + it 'does not register an offense when using Array literal' do + expect_no_offenses(<<~RUBY) + [1, 2, 3].include?(e) + RUBY + end + + it 'does not register an offense when using Hash literal' do + expect_no_offenses(<<~RUBY) + { foo: :bar }.key?(:foo) + 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) + [1, 2, variable].include?(e) + RUBY + end + + it 'does not register an offense when using Hash literal' do + expect_no_offenses(<<~RUBY) + { foo: { bar: variable } }.key?(:foo) + RUBY + end + end + + context 'when is an argument for `Enumerable` loop-like method' do + it 'does not register an offense when using Array literal' do + expect_no_offenses(<<~RUBY) + [[1, 2, 3] | [2, 3, 4]].each { |e| puts e } + RUBY + end + + it 'does not register an offense when using Hash literal' do + expect_no_offenses(<<~RUBY) + ({ foo: :bar }.merge(baz: :quux)).each { |k, v| puts k + v } + 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) + loop do + [1, nil, 3].compact! + end + RUBY + end + + it 'does not register an offense when using Hash literal' do + expect_no_offenses(<<~RUBY) + loop do + { 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) + loop do + array = [1, nil, 3] + end + RUBY + end + + it 'does not register an offense when using Hash literal' do + expect_no_offenses(<<~RUBY) + loop do + hash = { foo: :bar, baz: nil } + end + RUBY + end + end + + it 'does not register an offense when there are no literals in a loop' do + expect_no_offenses(<<~RUBY) + while x < 100 + puts x + end + RUBY + end + + it 'does not register an offense when nondestructive method is called on nonliteral' do + expect_no_offenses(<<~RUBY) + loop do + 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) + while true + [1].include?(e) + end + RUBY + end + + it 'does not register an offense when using Hash literal' do + expect_no_offenses(<<~RUBY) + while i < 100 + { foo: :bar }.key?(:foo) + end + RUBY + end + end +end