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

Add new Performance/CollectionLiteralInLoop cop #140

Merged
merged 2 commits into from
Jul 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,10 @@ Performance/Caller:
Performance/ChainArrayAllocation:
Enabled: false

Performance/CollectionLiteralInLoop:
Exclude:
- 'spec/**/*.rb'

RSpec/PredicateMatcher:
EnforcedStyle: explicit

Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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][])
Expand Down
7 changes: 7 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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

Choose a reason for hiding this comment

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

Shouldn't this have been pending?

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
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
52 changes: 52 additions & 0 deletions docs/modules/ROOT/pages/cops_performance.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -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

|===
Expand Down
140 changes: 140 additions & 0 deletions lib/rubocop/cop/performance/collection_literal_in_loop.rb
Original file line number Diff line number Diff line change
@@ -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 %<literal_class>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
1 change: 1 addition & 0 deletions lib/rubocop/cop/performance_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down