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

lazy not accounted for in ChainArrayAllocation cop #230

Closed
reedjosh1 opened this issue Mar 31, 2021 · 3 comments · Fixed by #233
Closed

lazy not accounted for in ChainArrayAllocation cop #230

reedjosh1 opened this issue Mar 31, 2021 · 3 comments · Fixed by #233
Labels
bug Something isn't working

Comments

@reedjosh1
Copy link

reedjosh1 commented Mar 31, 2021

lazy in a chain should negate the ChainArrayAllocation cop.

Expected behavior

some_array.lazy.map(&:some_obj_method).reject(&:nil?).first

Should not trigger a perf cop since this is actually a very performant (at least not too aweful) way of getting the first non-nil result. lazy should negate the chain cop as it no longer is a performance issue.

Actual behavior

Cop triggers as follows:

[Performance/ChainArrayAllocation] [I]  Use unchained `map` and `reject!`(followed by `return array` if required) instead of chaining `map...reject`.

Steps to reproduce the problem

Run perf cops on above code snippet.

RuboCop version

$ grep cop Gemfile.lock
    rubocop (1.6.1)
      rubocop-ast (>= 1.2.0, < 2.0)
    rubocop-ast (1.4.1)
    rubocop-performance (1.9.1)
      rubocop (>= 0.90.0, < 2.0)
      rubocop-ast (>= 0.4.0)
    rubocop-rails (2.9.1)
      rubocop (>= 0.90.0, < 2.0)
    rubocop-rake (0.5.1)
      rubocop
    rubocop-rspec (2.1.0)
      rubocop (~> 1.0)
      rubocop-ast (>= 1.1.0)
      rubocop (>= 0.52)
  rubocop (= 1.6.1)
  rubocop-performance (= 1.9.1)
  rubocop-rails (= 2.9.1)
  rubocop-rake (= 0.5.1)
  rubocop-rspec (= 2.1.0)
@marcandre
Copy link
Contributor

Indeed, looks like a bug.

BTW, lazy is typically much less performant than the non lazy version in practice...

Also not clear in your example why some_array.find(&:some_obj_method) isn't the way to go...

@reedjosh1
Copy link
Author

reedjosh1 commented Mar 31, 2021

Also not clear in your example why some_array.find(&:some_obj_method) isn't the way to go...

find returns the object for which some_obj_method is non-nil. I want the results of the method some_obj_method for the first that is non-nil.

BTW, lazy is typically much less performant than the non lazy version in practice...

This is true, I'm sure.

But, for most things the perf difference is small enough to justify the one-liner vs the 3-4 line equivalent. It is particularly so in my use case.

Also, thanks for the cool cop!

@marcandre
Copy link
Contributor

marcandre commented Mar 31, 2021

🤦, of course.

@koic koic added the bug Something isn't working label Apr 12, 2021
koic added a commit to koic/rubocop-performance that referenced this issue Apr 14, 2021
…ocation`

Fixes rubocop#230.

This PR fixes a false positive for `Performance/ChainArrayAllocation`
when using `Enumerable#lazy`.
@koic koic closed this as completed in #233 Apr 16, 2021
koic added a commit that referenced this issue Apr 16, 2021
…chain_array_allocation

[Fix #230] Fix a false positive for `Performance/ChainArrayAllocation`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants