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

Inconsistency between Style/ExplicitBlockArgument and Performance/RedundantBlockCall #9853

Open
maxp-hover opened this issue Jun 4, 2021 · 0 comments

Comments

@maxp-hover
Copy link

maxp-hover commented Jun 4, 2021

The code in question is something like this:

def my_function
  tap { yield }
  yield
end

The key detail is that yield is called once inside a block, and once outside a block.

Running rubocop against the above, I get a Style/ExplicitBlockArgument violation for the tap { yield }:

⋊> ~/H/hyperion on ex-fri-whodunnit-2  be rubocop --debug test.rb                15:38:49
For /Users/maxpleaner/Hover/hyperion: configuration from /Users/maxpleaner/Hover/hyperion/.rubocop.yml
configuration from /Users/maxpleaner/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/rubocop-performance-1.11.3/config/default.yml
configuration from /Users/maxpleaner/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/rubocop-performance-1.11.3/config/default.yml
Default configuration from /Users/maxpleaner/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/rubocop-1.16.0/config/default.yml
configuration from /Users/maxpleaner/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/rubocop-rails-2.10.1/config/default.yml
configuration from /Users/maxpleaner/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/rubocop-rails-2.10.1/config/default.yml
configuration from /Users/maxpleaner/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/rubocop-rspec-2.3.0/config/default.yml
configuration from /Users/maxpleaner/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/rubocop-rspec-2.3.0/config/default.yml
Inheriting configuration from /Users/maxpleaner/Hover/hyperion/.rubocop_todo.yml
Inspecting 1 file
Scanning /Users/maxpleaner/Hover/hyperion/test.rb
Loading cache from /Users/maxpleaner/.cache/rubocop_cache/d0319646db584b0ed0e7f6e1a8477d2db28bdbdc/296ebfdbe8735fac7b3e191e147f49950bec3527/9aec4e47aeb2bb49c6cd70d0576f53543cbf24b9
C

Offenses:

test.rb:2:3: C: [Correctable] Style/ExplicitBlockArgument: Consider using explicit block argument in the surrounding method's signature over yield.
  tap { yield }
  ^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense auto-correctable
Finished in 0.7227850002236664 seconds

If I change the code to:

def my_function(&blk)
  tap(&blk)
  blk.call
end

I instead get a Performance/RedundantBlockCall violation for the blk.call:

⋊> ~/H/hyperion on ex-fri-whodunnit-2  be rubocop --debug test.rb                15:32:21
For /Users/maxpleaner/Hover/hyperion: configuration from /Users/maxpleaner/Hover/hyperion/.rubocop.yml
configuration from /Users/maxpleaner/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/rubocop-performance-1.11.3/config/default.yml
configuration from /Users/maxpleaner/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/rubocop-performance-1.11.3/config/default.yml
Default configuration from /Users/maxpleaner/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/rubocop-1.16.0/config/default.yml
configuration from /Users/maxpleaner/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/rubocop-rails-2.10.1/config/default.yml
configuration from /Users/maxpleaner/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/rubocop-rails-2.10.1/config/default.yml
configuration from /Users/maxpleaner/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/rubocop-rspec-2.3.0/config/default.yml
configuration from /Users/maxpleaner/.rbenv/versions/2.6.5/lib/ruby/gems/2.6.0/gems/rubocop-rspec-2.3.0/config/default.yml
Inheriting configuration from /Users/maxpleaner/Hover/hyperion/.rubocop_todo.yml
Inspecting 1 file
Scanning /Users/maxpleaner/Hover/hyperion/test.rb
Loading cache from /Users/maxpleaner/.cache/rubocop_cache/d0319646db584b0ed0e7f6e1a8477d2db28bdbdc/296ebfdbe8735fac7b3e191e147f49950bec3527/0d25c58e72550e04bc30e6093d7eb187b89fd492
C

Offenses:

test.rb:3:3: C: [Correctable] Performance/RedundantBlockCall: Use yield instead of blk.call.
  blk.call
  ^^^^^^^^

1 file inspected, 1 offense detected, 1 offense auto-correctable
Finished in 0.8148109996691346 seconds

If I change the code to the following, it passes both checks:

def my_function(&blk)
  tap(&blk)
  yield
end

But this looks like bad code. I'm not even sure it would work properly.


Expected behavior

I understand that rubocop and rubocop-performance might have conflicting rules, and one or the other check will need to be turned off. But, my concern is that someone who follows the recommended fixes without thinking about it (e.g. auto-fixing) would end up with code that's worse than the original.

So, I would want rubocop to either accept both variations yield or { yield } or reject both variations, and similarly for rubocop-performance.

Also, it seems odd that Rubocop's recommendation would be specifically the opposite of rubocop-performance's. If yield is really so much better for performance, does it really make sense for Rubocop to enforce the opposite by default?

Actual behavior

See above

RuboCop version

⋊> ~/H/hyperion on ex-fri-whodunnit-2  be rubocop -V                             15:40:35
1.16.0 (using Parser 3.0.1.1, rubocop-ast 1.7.0, running on ruby 2.6.5 x86_64-darwin18)
  - rubocop-performance 1.11.3
  - rubocop-rails 2.10.1
  - rubocop-rspec 2.3.0
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

No branches or pull requests

1 participant