Add Hash#each_value and Hash#each_key cop (#7677)

CHANGELOG.md

@@ -8,6 +8,7 @@
 * [#7663](https://github.com/rubocop-hq/rubocop/pull/7663): Add new `Style/HashTransformKeys` and `Style/HashTransformValues` cops. ([@djudd][], [@eugeneius][])
 * [#7619](https://github.com/rubocop-hq/rubocop/issues/7619): Support autocorrect of legacy cop names for `Migration/DepartmentName`. ([@koic][])
 * [#7659](https://github.com/rubocop-hq/rubocop/pull/7659): Layout/LineLength autocorrect now breaks up long lines with blocks. ([@maxh][])
+* [#7677](https://github.com/rubocop-hq/rubocop/pull/7677): Add a cop for `Hash#each_key` and `Hash#each_value`. ([@jemmaissroff][])
 
 ### Bug fixes
 
@@ -4349,3 +4350,4 @@
 [@hanachin]: https://github.com/hanachin
 [@masarakki]: https://github.com/masarakki
 [@djudd]: https://github.com/djudd
+[@jemmaissroff]: https://github.com/jemmaissroff

config/default.yml

@@ -2778,6 +2778,13 @@ Style/GuardClause:
   # needs to have to trigger this cop
   MinBodyLength: 1
 
+Style/HashEachMethods:
+  Description: 'Use Hash#each_key and Hash#each_value.'
+  StyleGuide: '#hash-each'
+  Enabled: pending
+  VersionAdded: '0.80'
+  Safe: false
+
 Style/HashSyntax:
   Description: >-
                  Prefer Ruby 1.9 hash syntax { a: 1, b: 2 } over 1.8 syntax

lib/rubocop.rb

@@ -445,6 +445,7 @@
 require_relative 'rubocop/cop/style/frozen_string_literal_comment'
 require_relative 'rubocop/cop/style/global_vars'
 require_relative 'rubocop/cop/style/guard_clause'
+require_relative 'rubocop/cop/style/hash_each_methods'
 require_relative 'rubocop/cop/style/hash_syntax'
 require_relative 'rubocop/cop/style/hash_transform_keys'
 require_relative 'rubocop/cop/style/hash_transform_values'

lib/rubocop/cop/style/hash_each_methods.rb

@@ -0,0 +1,87 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Style
+      # This cop checks for uses of `each_key` and `each_value` Hash methods.
+      #
+      # Note: If you have an array of two-element arrays, you can put
+      #   parentheses around the block arguments to indicate that you're not
+      #   working with a hash, and suppress RuboCop offenses.
+      #
+      # @example
+      #   # bad
+      #   hash.keys.each { |k| p k }
+      #   hash.values.each { |v| p v }
+      #
+      #   # good
+      #   hash.each_key { |k| p k }
+      #   hash.each_value { |v| p v }
+      class HashEachMethods < Cop
+        include Lint::UnusedArgument
+
+        MSG = 'Use `%<prefer>s` instead of `%<current>s`.'
+
+        def_node_matcher :kv_each, <<~PATTERN
+          (block $(send (send _ ${:keys :values}) :each) ...)
+        PATTERN
+
+        def on_block(node)
+          register_kv_offense(node)
+        end
+
+        def autocorrect(node)
+          lambda do |corrector|
+            correct_key_value_each(node, corrector)
+          end
+        end
+
+        private
+
+        def register_kv_offense(node)
+          kv_each(node) do |target, method|
+            msg = format(message, prefer: "each_#{method[0..-2]}",
+                                  current: "#{method}.each")
+
+            add_offense(target, location: kv_range(target), message: msg)
+          end
+        end
+
+        def check_argument(variable)
+          return unless variable.block_argument?
+
+          (@block_args ||= []).push(variable)
+        end
+
+        def used?(arg)
+          @block_args.find { |var| var.declaration_node.loc == arg.loc }.used?
+        end
+
+        def correct_implicit(node, corrector, method_name)
+          corrector.replace(node.loc.expression, method_name)
+          correct_args(node, corrector)
+        end
+
+        def correct_key_value_each(node, corrector)
+          receiver = node.receiver.receiver
+          name = "each_#{node.receiver.method_name.to_s.chop}"
+          return correct_implicit(node, corrector, name) unless receiver
+
+          new_source = receiver.source + ".#{name}"
+          corrector.replace(node.loc.expression, new_source)
+        end
+
+        def correct_args(node, corrector)
+          args = node.parent.arguments
+          name, = *args.children.find { |arg| used?(arg) }
+
+          corrector.replace(args.source_range, "|#{name}|")
+        end
+
+        def kv_range(outer_node)
+          outer_node.receiver.loc.selector.join(outer_node.loc.selector)
+        end
+      end
+    end
+  end
+end

manual/cops.md

@@ -357,6 +357,7 @@ In the following section you find all available cops:
 * [Style/FrozenStringLiteralComment](cops_style.md#stylefrozenstringliteralcomment)
 * [Style/GlobalVars](cops_style.md#styleglobalvars)
 * [Style/GuardClause](cops_style.md#styleguardclause)
+* [Style/HashEachMethods](cops_style.md#stylehasheachmethods)
 * [Style/HashSyntax](cops_style.md#stylehashsyntax)
 * [Style/HashTransformKeys](cops_style.md#stylehashtransformkeys)
 * [Style/HashTransformValues](cops_style.md#stylehashtransformvalues)

manual/cops_style.md

@@ -2391,6 +2391,34 @@ MinBodyLength | `1` | Integer
 
 * [https://rubystyle.guide#no-nested-conditionals](https://rubystyle.guide#no-nested-conditionals)
 
+## Style/HashEachMethods
+
+Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+--- | --- | --- | --- | ---
+Pending | No | Yes (Unsafe) | 0.80 | -
+
+This cop checks for uses of `each_key` and `each_value` Hash methods.
+
+Note: If you have an array of two-element arrays, you can put
+  parentheses around the block arguments to indicate that you're not
+  working with a hash, and suppress RuboCop offenses.
+
+### Examples
+
+```ruby
+# bad
+hash.keys.each { |k| p k }
+hash.values.each { |v| p v }
+
+# good
+hash.each_key { |k| p k }
+hash.each_value { |v| p v }
+```
+
+### References
+
+* [https://rubystyle.guide#hash-each](https://rubystyle.guide#hash-each)
+
 ## Style/HashSyntax
 
 Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged

spec/rubocop/cop/style/hash_each_methods_spec.rb

@@ -0,0 +1,103 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Style::HashEachMethods do
+  subject(:cop) { described_class.new }
+
+  context 'when node matches a keys#each or values#each' do
+    context 'when receiver is a send' do
+      it 'registers offense, auto-corrects foo#keys.each to foo#each_key' do
+        expect_offense(<<~RUBY)
+          foo.keys.each { |k| p k }
+              ^^^^^^^^^ Use `each_key` instead of `keys.each`.
+        RUBY
+
+        expect_correction(<<~RUBY)
+          foo.each_key { |k| p k }
+        RUBY
+      end
+
+      it 'registers offense, auto-corrects foo#values.each to foo#each_value' do
+        expect_offense(<<~RUBY)
+          foo.values.each { |v| p v }
+              ^^^^^^^^^^^ Use `each_value` instead of `values.each`.
+        RUBY
+
+        expect_correction(<<~RUBY)
+          foo.each_value { |v| p v }
+        RUBY
+      end
+
+      it 'does not register an offense for foo#each_key' do
+        expect_no_offenses('foo.each_key { |k| p k }')
+      end
+
+      it 'does not register an offense for Hash#each_value' do
+        expect_no_offenses('foo.each_value { |v| p v }')
+      end
+    end
+
+    context 'when receiver is a hash literal' do
+      it 'registers offense, auto-corrects {}#keys.each with {}#each_key' do
+        expect_offense(<<~RUBY)
+          {}.keys.each { |k| p k }
+             ^^^^^^^^^ Use `each_key` instead of `keys.each`.
+        RUBY
+
+        expect_correction(<<~RUBY)
+          {}.each_key { |k| p k }
+        RUBY
+      end
+
+      it 'registers offense, auto-corrects {}#values.each with {}#each_value' do
+        expect_offense(<<~RUBY)
+          {}.values.each { |k| p k }
+             ^^^^^^^^^^^ Use `each_value` instead of `values.each`.
+        RUBY
+
+        expect_correction(<<~RUBY)
+          {}.each_value { |k| p k }
+        RUBY
+      end
+
+      it 'does not register an offense for {}#each_key' do
+        expect_no_offenses('{}.each_key { |k| p k }')
+      end
+
+      it 'does not register an offense for {}#each_value' do
+        expect_no_offenses('{}.each_value { |v| p v }')
+      end
+    end
+
+    context 'when receiver is implicit' do
+      it 'registers an offense and auto-corrects keys.each with each_key' do
+        expect_offense(<<~RUBY)
+          keys.each { |k| p k }
+          ^^^^^^^^^ Use `each_key` instead of `keys.each`.
+        RUBY
+
+        expect_correction(<<~RUBY)
+          each_key { |k| p k }
+        RUBY
+      end
+
+      it 'registers an offense and auto-corrects values.each with each_value' do
+        expect_offense(<<~RUBY)
+          values.each { |v| p v }
+          ^^^^^^^^^^^ Use `each_value` instead of `values.each`.
+        RUBY
+
+        expect_correction(<<~RUBY)
+          each_value { |v| p v }
+        RUBY
+      end
+
+      it 'does not register an offense for each_key' do
+        expect_no_offenses('each_key { |k| p k }')
+      end
+
+      it 'does not register an offense for each_value' do
+        expect_no_offenses('each_value { |v| p v }')
+      end
+    end
+  end
+end