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

Performance/RedundantBlockCall could use a complementary cop to auto-remove unused blocks passed to method #2879

Closed
lamont-granquist opened this issue Feb 23, 2016 · 4 comments

Comments

@lamont-granquist
Copy link

Kind of like Style/UnusedMethodArgument but targeted specifically at &block arguments and autofixes them by removing them from the method signature.

When fixing this:

def foo(&block)
  block.call
end

It gets fixed to this via Performance/RedundantBlockCall and Style/UnusedMethodArgument:

def foo(&_block)
  yield
end

That is still slow, and i think rubocop should be able to autocorrect that further to:

def foo
  yield
end

Don't know if that's additional functionality to one of those two cops, or if its a third cop to add.

I pretty sure its safe to do this since ruby always accepts a block even if its not declared on the method signature, so its totally useless to have it there if its never called?

@mikegee
Copy link
Contributor

mikegee commented Feb 23, 2016

I work with some people that like to leave &block In the signature for documentation purposes. I prefer removing it, I'm just letting you know about that point of view.

@lamont-granquist
Copy link
Author

But block+yield is nearly as slow as block+call:

https://github.com/JuanitoFatas/fast-ruby#proccall-and-block-arguments-vs-yieldcode
https://docs.omniref.com/ruby/2.2.0/symbols/Proc/yield#annotation=4087638&line=711

So "Performance/RedundantBlockCall" is a bit of a lie without also cleaning up the method signature.

@mikegee
Copy link
Contributor

mikegee commented Feb 23, 2016

I was trying to address this part:

its totally useless to have it there if its never called?

I agree that the argument should be removed for performance. If somebody doesn't like it, they'll need to disable a cop, just like any other time people disagree with rubocop.

@lamont-granquist
Copy link
Author

Yeah, I'm considering it to be 'useless' from ruby's perspective -- IOW, the code won't break if its programmatically removed from a large codebase and there's no odd unintended side effects from the ruby interpreter that I've not considered.

@bbatsov bbatsov closed this as completed in cf43099 Mar 9, 2016
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

2 participants