Rubocop: Performance/HashEachMethods false positives for non-Hashes

[tonyta]

Performance/HashEachMethods suggests using each_value for non-Hashes:

# Array
[[:a, 1]].each { |_v1, v2| puts v2 }

# CSV::Row
row = CSV.parse("a\n1", headers: true).first
row.each { |_header, value| puts value }

This does not appear when using parallel-assignment:

 # Array
-[[:a, 1]].each { |_v1, v2| puts v2 }
+[[:a, 1]].each { |(_v1, v2)| puts v2 }

 # CSV::Row
 row = CSV.parse("a\n1", headers: true).first
-row.each { |_header, value| puts value }
+row.each { |(_header, value)| puts value }

Expected behavior

This should not be a violation when the object is not a Hash. It might not be possible to confirm if the object is a Hash, but perhaps a note about using parallel assignment in the error message or the included link might be helpful.

Actual behavior

False positives when the object is an Array, CSV::Row, or other object that does not implement #each_row or #each_key.

Steps to reproduce the problem

.rubocop.yml

AllCops:
  TargetRubyVersion: 2.3.3
Performance/HashEachMethods:
  Enabled: true

hash_each_methods.rb

# frozen_string_literal: true

require 'csv'

csv_string = "a,b,c\n1,2,3"
CSV.parse(csv_string, headers: true) do |row|
  row.each { |_header, value| puts value }
end

array = [['x', 9], ['y', 8], ['z', 7]]
array.each { |_v1, v2| puts v2 }

Console:

$ rubocop --version
0.51.0

$ rubocop hash_each_methods.rb 
Inspecting 1 file
C

Offenses:

hash_each_methods.rb:7:7: C: Use each_value instead of each.
  row.each { |_header, value| puts value }
      ^^^^
hash_each_methods.rb:11:7: C: Use each_value instead of each.
array.each { |_v1, v2| puts v2 }
      ^^^^

1 file inspected, 2 offenses detected

RuboCop version

Reproduced on:

• v0.51.0
• master (9e0e8a5e)

$ rubocop -V
0.51.0 (using Parser 2.4.0.0, running on ruby 2.3.3 x86_64-darwin16)

Related Issues (Maybe)

https://github.com/bbatsov/rubocop/issues/4872 Performance/HashEachMethods shouldn't try to enforce replacement of each (each_pair)
https://github.com/bbatsov/rubocop/issues/4777 Performance/HashEachMethods triggers with any "values.each"

[Drenmi]

It might not be possible to confirm if the object is a Hash [...]

Unfortunately it isn't. It also does not have to be a hash. Hash-like constructs with key-value pairs should ideally implement the same interface as Hash for polymorphism. 🙂

[...] perhaps a note about using parallel assignment in the error message or the included link might be helpful.

Yep. Adding the parentheses to indicate this is not a key-value pair is something that was pointed out as a useful convention. We should add this to the message.

[prashuchaudhary]

@tonyta I want to take this up

[prashuchaudhary]

So we want to skip all Object types which can be iterated and not necessarily classify as a HASH

[bbatsov]

There hasn't been much activity on this ticket and our Core Team is spread too thin on so many tasks, so I'll just close it for the sake of having a cleaner lists of tasks to focus on. We'd gladly review a PR, but it's unlikely that'd we're going to tackle this ourselves in the foreseeable future.

[Darhazer]

Performance/HashEachMethods was removed in RuboCop 0.53

[bbatsov]

Haha, seems this ticket cleanup was way overdue then. 😆