Skip to content

Commit

Permalink
[Fix #12370] Make Style/HashEachMethods aware of unused block value
Browse files Browse the repository at this point in the history
Fixes #12370.

This PR makes `Style/HashEachMethods` aware of unused block value.
And this PR suppresses the following new offense in this repository:

```console
$ bundle exec rubocop --only Style/HashEachMethods -A
(snip)

Offenses:

lib/rubocop/cop/style/bisected_attr_accessor.rb:36:11: C: [Corrected] Style/HashEachMethods:
Use each_value instead of each, and remove the unused _visibility block argument.
          find_macros(class_node.body).each do |_visibility, macros| ...
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1519 files inspected, 1 offense detected, 1 offense corrected
```
  • Loading branch information
koic authored and bbatsov committed Nov 21, 2023
1 parent 10c8d32 commit de86a68
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 4 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#12370](https://github.com/rubocop/rubocop/issues/12370): Make `Style/HashEachMethods` aware of unused block value. ([@koic][])
4 changes: 2 additions & 2 deletions lib/rubocop/cop/style/bisected_attr_accessor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def on_new_investigation
def on_class(class_node)
@macros_to_rewrite[class_node] = Set.new

find_macros(class_node.body).each do |_visibility, macros|
find_macros(class_node.body).each_value do |macros|
bisected = find_bisection(macros)
next unless bisected.any?

Expand Down Expand Up @@ -74,7 +74,7 @@ def after_class(class_node)
def find_macros(class_def)
# Find all the macros (`attr_reader`, `attr_writer`, etc.) in the class body
# and turn them into `Macro` objects so that they can be processed.
return [] if !class_def || class_def.def_type?
return {} if !class_def || class_def.def_type?

send_nodes =
if class_def.send_type?
Expand Down
49 changes: 47 additions & 2 deletions lib/rubocop/cop/style/hash_each_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,16 @@ module Style
# @example
# # bad
# hash.keys.each { |k| p k }
# hash.values.each { |v| p v }
# hash.each { |k, unused_value| p k }
#
# # good
# hash.each_key { |k| p k }
#
# # bad
# hash.values.each { |v| p v }
# hash.each { |unused_key, v| p v }
#
# # good
# hash.each_value { |v| p v }
#
# @example AllowedReceivers: ['execute']
Expand All @@ -33,22 +39,44 @@ class HashEachMethods < Base
extend AutoCorrector

MSG = 'Use `%<prefer>s` instead of `%<current>s`.'
UNUSED_BLOCK_ARG_MSG = "#{MSG.chop} and remove the unused `%<unused_code>s` block argument."

# @!method kv_each(node)
def_node_matcher :kv_each, <<~PATTERN
({block numblock} $(send (send _ ${:keys :values}) :each) ...)
PATTERN

# @!method each_arguments(node)
def_node_matcher :each_arguments, <<~PATTERN
(block (send _ :each)(args $_key $_value) ...)
PATTERN

# @!method kv_each_with_block_pass(node)
def_node_matcher :kv_each_with_block_pass, <<~PATTERN
(send $(send _ ${:keys :values}) :each (block_pass (sym _)))
PATTERN

# rubocop:disable Metrics/AbcSize
def on_block(node)
kv_each(node) do |target, method|
register_kv_offense(target, method)
register_kv_offense(target, method) and return
end

return unless (key, value = each_arguments(node))

if unused_block_arg_exist?(node, value.source)
message = message('each_key', node.method_name, value.source)
unused_range = key.source_range.end.join(value.source_range.end)

register_each_args_offense(node, message, 'each_key', unused_range)
elsif unused_block_arg_exist?(node, key.source)
message = message('each_value', node.method_name, key.source)
unused_range = key.source_range.begin.join(value.source_range.begin)

register_each_args_offense(node, message, 'each_value', unused_range)
end
end
# rubocop:enable Metrics/AbcSize

alias on_numblock on_block

Expand All @@ -69,6 +97,23 @@ def register_kv_offense(target, method)
end
end

def unused_block_arg_exist?(node, block_arg_source)
node.body.each_descendant(:lvar).map(&:source).none?(block_arg_source)
end

def message(prefer, method_name, unused_code)
format(
UNUSED_BLOCK_ARG_MSG, prefer: prefer, current: method_name, unused_code: unused_code
)
end

def register_each_args_offense(node, message, prefer, unused_range)
add_offense(node, message: message) do |corrector|
corrector.replace(node.send_node.loc.selector, prefer)
corrector.remove(unused_range)
end
end

def register_kv_with_block_pass_offense(node, target, method)
return unless (parent_receiver = node.parent.receiver.receiver)
return if allowed_receiver?(parent_receiver)
Expand Down
34 changes: 34 additions & 0 deletions spec/rubocop/cop/style/hash_each_methods_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,40 @@
RUBY
end

it 'does not register an offense when the key and value block arguments of `Enumerable#each` method are used' do
expect_no_offenses('foo.each { |k, v| do_something(k, v) }')
end

it 'does not register an offense when the single block argument of `Enumerable#each` method is used' do
expect_no_offenses('foo.each { |e| do_something(e) }')
end

it 'does not register an offense when the parenthesized key and value block arguments of `Enumerable#each` method are unused' do
expect_no_offenses('foo.each { |(k, v)| do_something(e) }')
end

it 'registers an offense when the value block argument of `Enumerable#each` method is unused' do
expect_offense(<<~RUBY)
foo.each { |k, unused_value| do_something(k) }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `each_key` instead of `each` and remove the unused `unused_value` block argument.
RUBY

expect_correction(<<~RUBY)
foo.each_key { |k| do_something(k) }
RUBY
end

it 'registers an offense when the key block argument of `Enumerable#each` method is unused' do
expect_offense(<<~RUBY)
foo.each { |unused_key, v| do_something(v) }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `each_value` instead of `each` and remove the unused `unused_key` block argument.
RUBY

expect_correction(<<~RUBY)
foo.each_value { |v| do_something(v) }
RUBY
end

it 'does not register an offense for foo#each_key' do
expect_no_offenses('foo.each_key { |k| p k }')
end
Expand Down

0 comments on commit de86a68

Please sign in to comment.