Add Performance/InefficientHashSearch cop #5848
Conversation
Usage of `Hash#keys.include?` can be replaced with the much more efficient `Hash#key?` method. Similarly, `Hash#values.include?` can be replaced with the more efficient `Hash#value?` method. This commit adds the `Performance/InefficientHashSearch` cop, which identifies and autocorrects these inefficient hash searches.
| PATTERN | ||
|
|
||
| def on_send(node) | ||
| add_offense(node, message: KEYS_MSG) if keys_include?(node) |
There was a problem hiding this comment.
This could maybe be an if-else, but I don't know enough about how ruby_parser works to say that one node couldn't potentially have both issues. Feedback welcome!
| lambda do |corrector| | ||
| # Replace `keys.include?` or `values.include?` with the appropriate | ||
| # `key?`/`value?` method. | ||
| corrector.replace( |
There was a problem hiding this comment.
I'm not sure if this is the best way to do this autocorrect; whitespace gets swallowed and the new code will have parens even if the original include? did not. Feedback welcome!
There was a problem hiding this comment.
You could probably try (from the top of my head):
receiver, include_method, _arg = *node
_, keys_or_values = *receiver
corrector.replace(include_method.source_range, '') # Maybe there's a `corrector.delete` ?
corrector.replace(keys_or_values.source_range, autocorrect_method(keys_or_values))There was a problem hiding this comment.
Thanks! This already merged but I'll play around with this approach next time!
There was a problem hiding this comment.
Yeah, just came across this PR while checking the changelog and saw your comment without any reply
There was a problem hiding this comment.
I really appreciate it!
|
|
||
| def autocorrect_method(node) | ||
| old_method = node.children[0].loc.selector.source | ||
| old_method == 'keys' ? 'key?' : 'value?' |
There was a problem hiding this comment.
I wasn't sure if this should incorporate the codebase's Style/PreferredHashMethods preference (and potentially use has_key? and has_value? instead). Or whether autocorrect runs in multiple passes so that would get corrected anyway. Thoughts?
There was a problem hiding this comment.
Great question. We try to avoid having one cop produce offenses in another cop. Checking the configuration of Style/PreferredHashMethods and using the short or verbose form would be preferred.
config/enabled.yml
Outdated
| @@ -983,6 +983,11 @@ Performance/FlatMap: | |||
| # This can be dangerous since `flat_map` will only flatten 1 level, and | |||
| # `flatten` without any parameters can flatten multiple levels. | |||
|
|
|||
| Performance/InefficientHashSearch: | |||
| Description: 'Use `key?` or `value?` instead of `keys.include?` or `values.include?`' | |||
| Reference: 'https://github.com/JuanitoFatas/fast-ruby#hashkey-instead-of-hashkeysinclude-and-hashvalue-instead-of-hashvaluesinclude-code' | |||
There was a problem hiding this comment.
This reference doesn't exist yet, but I have an outstanding PR which will create it: fastruby/fast-ruby#154
|
This is my first RuboCop PR and I'd appreciate any feedback! |
|
|
||
| def autocorrect_method(node) | ||
| old_method = node.children[0].loc.selector.source | ||
| old_method == 'keys' ? 'key?' : 'value?' |
There was a problem hiding this comment.
Great question. We try to avoid having one cop produce offenses in another cop. Checking the configuration of Style/PreferredHashMethods and using the short or verbose form would be preferred.
| private | ||
|
|
||
| def autocorrect_method(node) | ||
| old_method = node.children[0].loc.selector.source |
There was a problem hiding this comment.
node.children[0].method_name could be used here. It will return a symbol instead of a string for the method name.
There was a problem hiding this comment.
Thanks for the tip! Changed.
| PATTERN | ||
|
|
||
| def_node_matcher :values_include?, <<-PATTERN | ||
| (send (send _ :values) :include? _) |
There was a problem hiding this comment.
You can probably simplify some of the auto-correct logic by using $ as a capture in the node pattern.
(send $(send _ ${:keys :values}) :include? $_)
The first return would be node.children[0].
The second return would be the method name.
The third return would be the arguments being passed to include?.
If you go this route, the patterns can be combined into a single matcher and the message can be determined based off the method name.
There was a problem hiding this comment.
Thanks for the tip! Changed.
|
Thanks for all the feedback, @rrosenblum! Just pushed a commit that addresses your comments. |
| # | ||
| class InefficientHashSearch < Cop | ||
| def_node_matcher :inefficient_include?, <<-PATTERN | ||
| (send $(send _ ${:keys :values}) :include? $_) |
There was a problem hiding this comment.
Should the cop also check and fix exclude? ?
There was a problem hiding this comment.
exclude is in ActiveSupport, so probably not.
|
Great first contribution! Keep them coming! 😉 |
|
Thanks! |
Usage of
Hash#keys.include?can be replaced with themuch more efficient
Hash#key?method. Similarly,Hash#values.include?can be replaced with the moreefficient
Hash#value?method.This commit adds the
Performance/InefficientHashSearchcop, which identifies and autocorrects these
inefficient hash searches.
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).and description in grammatically correct, complete sentences.
rake defaultorrake parallel. It executes all tests and RuboCop for itself, and generates the documentation.