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

Conversation

fatkodima
Copy link
Contributor

@fatkodima fatkodima commented Jun 12, 2020

I have seen code, like the following, a lot:

users.select do |user|
  %i[superadmin admin].include?(user.role)
end

It unnecessarily allocates array for each iteration of the loop. As soon as the iteration completes, the newly created array becomes unreachable garbage. It is better to extract it out of the loop, for example, to a local variable:

admin_roles = %i[superadmin admin]
users.select do |user|
  admin_roles.include?(user.role)
end

or to a constant:

# somewhere at the top of the class
ADMIN_ROLES = %i[superadmin admin]
...
users.select do |user|
  ADMIN_ROLES.include?(user.role)
end

This cop also works for hashes. It finds for non mutated array/hash literals inside loops.

I can provide benchmarks, if needed.

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
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a feeling that the logic for immutable collections was needed somewhere else as well, so maybe it should eventually be made reusable in rubocop-ast.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Yes, I have found something similar https://github.com/rubocop-hq/rubocop/blob/b571ba252fba0e13331871b065b28f27c107eeda/lib/rubocop/cop/lint/void.rb#L56-L62

  2. And I already used this ENUMERABLE_METHOD_NAMES constant in another PR https://github.com/rubocop-hq/rubocop-performance/pull/127/files#diff-6e1da1392214a67f5b4f8919f93f9b20R31 , so worth extracting

So how do you feel about:
3. extracting those POST_CONDITION_LOOP_TYPES and LOOP_TYPES into some properly named constants in rubocop-asts AST::Node

  1. and those ENUMERABLE_METHOD_NAMES, NONMUTATING_ARRAY_METHODS and NONMUTATING_HASH_METHODS into rubocop-asts AST::CollectionNode module?

  2. And probably it would be useful to extract those https://github.com/rubocop-hq/rubocop/blob/b571ba252fba0e13331871b065b28f27c107eeda/lib/rubocop/cop/lint/void.rb#L52-L54 into AST::Node.

  3. And probably we should extend AST::Node with missing keywords to replace this hardcoded list? https://github.com/rubocop-hq/rubocop/blob/b571ba252fba0e13331871b065b28f27c107eeda/lib/rubocop/cop/style/redundant_self.rb#L146-L150

Copy link
Contributor

Choose a reason for hiding this comment

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

3-6 all make sense to me.

@marcandre
Copy link
Contributor

Have you considered autocorrecting it to create a Set? Unless it calls join. This would be a stronger performance fix

@fatkodima
Copy link
Contributor Author

No, haven't considered this. The original idea was to extract such code somewhere, not not replace it with something faster. Maybe extracted code then would be better transformed into set, but I think thats a story for another cop.

@fatkodima fatkodima changed the title Add new Performance/LiteralInLoop cop Add new Performance/CollectionLiteralInLoop cop Jun 17, 2020
@bbatsov bbatsov merged commit 7d2c6b1 into rubocop:master Jul 18, 2020
@@ -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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants