New Cop Performance/HashEachMethods

This cop checks that `Hash#each_key`/`Hash#each_value` is used
instead of `Hash#keys.each`/`Hash#values.each`/`Hash#each`.
See https://github.com/bbatsov/ruby-style-guide#hash-each

CHANGELOG.md

@@ -64,6 +64,7 @@
 * [#1725](https://github.com/bbatsov/rubocop/issues/1725): --color CLI option forces color output, even when not printing to a TTY. ([@alexdowad][])
 * [#2549](https://github.com/bbatsov/rubocop/issues/2549): New `ConsistentQuotesInMultiline` config param for `Style/StringLiterals` forces all literals which are concatenated using \ to use the same quote style. ([@alexdowad][])
 * [#2560](https://github.com/bbatsov/rubocop/issues/2560): `Style/AccessModifierIndentation`, `Style/CaseIndentation`, `Style/FirstParameterIndentation`, `Style/IndentArray`, `Style/IndentAssignment`, `Style/IndentHash`, `Style/MultilineMethodCallIndentation`, and `Style/MultilineOperationIndentation` all have a new `IndentationWidth` parameter which can be used to override the indentation width from `Style/IndentationWidth`. ([@alexdowad][])
+* Add new `Performance/HashEachMethods` cop. ([@ojab][])
 
 ### Bug Fixes
 

config/enabled.yml

@@ -1197,6 +1197,14 @@ Performance/FlatMap:
   # This can be dangerous since `flat_map` will only flatten 1 level, and
   # `flatten` without any parameters can flatten multiple levels.
 
+Performance/HashEachMethods:
+  Description: >-
+                 Use `Hash#each_key` and `Hash#each_value` instead of
+                 `Hash#keys.each` and `Hash#values.each`.
+  StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#hash-each'
+  Enabled: true
+  AutoCorrect: false
+
 Performance/LstripRstrip:
   Description: 'Use `strip` instead of `lstrip.rstrip`.'
   Enabled: true

lib/rubocop.rb

@@ -147,6 +147,7 @@
 require 'rubocop/cop/performance/end_with'
 require 'rubocop/cop/performance/fixed_size'
 require 'rubocop/cop/performance/flat_map'
+require 'rubocop/cop/performance/hash_each'
 require 'rubocop/cop/performance/lstrip_rstrip'
 require 'rubocop/cop/performance/range_include'
 require 'rubocop/cop/performance/redundant_block_call'

lib/rubocop/config.rb

@@ -28,7 +28,7 @@ def initialize(hash = {}, loaded_path = nil)
     end
 
     def make_excludes_absolute
-      keys.each do |key|
+      each_key do |key|
         validate_section_presence(key)
         next unless self[key]['Exclude']
 

lib/rubocop/cop/performance/hash_each.rb

@@ -0,0 +1,85 @@
+# encoding: utf-8
+
+module RuboCop
+  module Cop
+    module Performance
+      # This cop checks for uses of `each_key` & `each_value` Hash methods.
+      #
+      # @example
+      #   # bad
+      #   hash.keys.each { |k| p k }
+      #   hash.values.each { |v| p v }
+      #   hash.each { |k, _v| p k }
+      #   hash.each { |_k, v| p v }
+      #
+      #   # good
+      #   hash.each_key { |k| p k }
+      #   hash.each_value { |v| p v }
+      class HashEachMethods < Cop
+        include Lint::UnusedArgument
+
+        MSG = 'Use `%s` instead of `%s`.'.freeze
+
+        def_node_matcher :plain_each, <<-END
+          (block $(send (send _ :hash) :each) (args (arg $_k) (arg $_v)) ...)
+        END
+
+        def_node_matcher :kv_each, <<-END
+          (block $(send (send (send _ :hash) ${:keys :values}) :each) ...)
+        END
+
+        def on_block(node)
+          plain_each(node) do |target, k, v|
+            return if @args[k] && @args[v]
+            used = @args[k] ? :key : :value
+            add_offense(target, range(target), format(message,
+                                                      "each_#{used}",
+                                                      :each))
+          end
+          kv_each(node) do |target, method|
+            add_offense(target, range(target), format(message,
+                                                      "each_#{method[0..-2]}",
+                                                      "#{method}.each"))
+          end
+        end
+
+        def check_argument(variable)
+          return unless variable.block_argument?
+          (@args ||= {})[variable.name] = variable.used?
+        end
+
+        def autocorrect(node)
+          receiver, _second_method = *node
+          caller, first_method = *receiver
+          lambda do |corrector|
+            if first_method == :hash
+              method = @args.values.first ? :key : :value
+              new_source = receiver.source + ".each_#{method}"
+              corrector.replace(node.loc.expression, new_source)
+              correct_args(node, corrector)
+            else
+              new_source = caller.source + ".each_#{first_method[0..-2]}"
+              corrector.replace(node.loc.expression, new_source)
+            end
+          end
+        end
+
+        private
+
+        def correct_args(node, corrector)
+          args = node.parent.children[1]
+          used_arg = "|#{@args.detect { |_k, v| v }.first}|"
+          args_range = Parser::Source::Range.new(node.parent.source,
+                                                 args.loc.begin.begin_pos,
+                                                 args.loc.end.end_pos)
+          corrector.replace(args_range, used_arg)
+        end
+
+        def range(outer_node)
+          inner_node = outer_node.children.first
+          inner_node.loc.selector.join(outer_node.loc.selector)
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/performance/hash_each_spec.rb

@@ -0,0 +1,76 @@
+# encoding: utf-8
+
+require 'spec_helper'
+
+describe RuboCop::Cop::Performance::HashEachMethods do
+  subject(:cop) { described_class.new }
+  let(:hash) { { key: 'value' } }
+
+  it 'registers an offense for Hash#keys.each' do
+    inspect_source(cop, 'hash.keys.each { |k| p k }')
+    expect(cop.offenses.size).to eq(1)
+    expect(cop.messages)
+      .to eq(['Use `each_key` instead of `keys.each`.'])
+  end
+
+  it 'registers an offense for Hash#values.each' do
+    inspect_source(cop, 'hash.values.each { |v| p v }')
+    expect(cop.offenses.size).to eq(1)
+    expect(cop.messages)
+      .to eq(['Use `each_value` instead of `values.each`.'])
+  end
+
+  it 'registers an offense for Hash#each with unused value' do
+    inspect_source(cop, 'hash.each { |k, _v| p k }')
+    expect(cop.offenses.size).to eq(1)
+    expect(cop.messages)
+      .to eq(['Use `each_key` instead of `each`.'])
+  end
+
+  it 'registers an offense for Hash#each with unused key' do
+    inspect_source(cop, 'hash.each { |_k, v| p v }')
+    expect(cop.offenses.size).to eq(1)
+    expect(cop.messages)
+      .to eq(['Use `each_value` instead of `each`.'])
+  end
+
+  it 'does not register an offense for Hash#each_key' do
+    inspect_source(cop, 'hash.each_key { |k| p k }')
+    expect(cop.messages).to be_empty
+  end
+
+  it 'does not register an offense for Hash#each_value' do
+    inspect_source(cop, 'hash.each_value { |v| p v }')
+    expect(cop.messages).to be_empty
+  end
+
+  it 'does not register an offense for Hash#each if both key/value are used' do
+    inspect_source(cop, 'hash.each { |k, v| p "#{k}_#{v}" }')
+    expect(cop.messages).to be_empty
+  end
+
+  it 'does not register an offense for Hash#each if block takes only one arg' do
+    inspect_source(cop, 'hash.each { |kv| p kv }')
+    expect(cop.messages).to be_empty
+  end
+
+  it 'auto-corrects Hash#keys.each with Hash#each_key' do
+    new_source = autocorrect_source(cop, 'hash.keys.each { |k| p k }')
+    expect(new_source).to eq('hash.each_key { |k| p k }')
+  end
+
+  it 'auto-corrects Hash#values.each with Hash#each_value' do
+    new_source = autocorrect_source(cop, 'hash.values.each { |v| p v }')
+    expect(new_source).to eq('hash.each_value { |v| p v }')
+  end
+
+  it 'auto-corrects Hash#each with unused value argument with Hash#each_key' do
+    new_source = autocorrect_source(cop, 'hash.each { |k, _v| p k }')
+    expect(new_source).to eq('hash.each_key { |k| p k }')
+  end
+
+  it 'auto-corrects Hash#each with unused key argument with Hash#each_value' do
+    new_source = autocorrect_source(cop, 'hash.each { |_k, v| p v }')
+    expect(new_source).to eq('hash.each_value { |v| p v }')
+  end
+end