Performance/HashEachMethods matches non-hashes

[akerl]

The change in 0.50.0 for Performance/HashEachMethods causes it to match on non-hashes where the block takes 2 args.

Expected behavior

On the following, no cop failure should be reported:

❯ cat test.rb
foo = [
  [1, 2],
  [3, 4]
]

foo.each { |a, _| puts a }

Actual behavior

A failure is reported:

❯ rubocop test.rb
Inspecting 1 file
C

Offenses:

test.rb:6:5: C: Use each_key instead of each.
foo.each { |a, _| puts a }
    ^^^^

1 file inspected, 1 offense detected

Steps to reproduce the problem

The above code sample should be able to reproduce this

RuboCop version

❯ rubocop -V
0.50.0 (using Parser 2.4.0.0, running on ruby 2.4.1 x86_64-darwin16)

[bbatsov]

That's unavoidable and unfixable due to the nature of Ruby. We can't really tell apart a hash from something that looks like a hash. You should either disable the cop or simply using # rubocop:disable for such relatively rare situations.

[akerl]

It doesn't end up being very rare, given that a handful of common Hash methods themselves return an array of 2-length arrays:

❯ rubocop -V
0.50.0 (using Parser 2.4.0.0, running on ruby 2.4.1 x86_64-darwin16)

❯ cat test.rb
hash = { foo: 1, bar: 2, baz: 3 }

hash.select { |_, v| v.odd? }.each { |k, _| puts k }

❯ rubocop -D test.rb
Inspecting 1 file
C

Offenses:

test.rb:3:31: C: Performance/HashEachMethods: Use each_key instead of each.
hash.select { |_, v| v.odd? }.each { |k, _| puts k }
                              ^^^^

1 file inspected, 1 offense detected

[Drenmi]

[...] a handful of common Hash methods themselves return an array of 2-length arrays:

hash = { foo: 1, bar: 2, baz: 3 }

hash.select { |_, v| v.odd? }.each { |k, _| puts k }

Not sure what you mean. Hash#select returns a hash. 🤔 The only method on Hash I know of which returns an array of two-element arrays is Hash#to_a. The good news is we can check for that! (And potential other methods.) As long as it's chained.

In the end, because we don't know what something is at runtime, we need to work with trade-offs. I'm inclined to think hashes are more common than arrays of two-element arrays. If it's not the case for your project, then there are tools to disable selectively or altogether. 🙂

[jaredbeck]

In 0.50.0 I'm seeing HashEachMethods ask me to call each_key on an ActionController::Parameters, but Parameters does not respond to each_key.

app/controllers/redacted/redacted_controller.rb:69:25: C: Performance/HashEachMethods: Use each_key instead of each.
      params[:redacted].each do |id, _attributes|
                        ^^^^

[AlexWayfer]

(snapshot.to_a - last_snapshot.to_a).each do |file, _mtime|
  @changes[file] = last_snapshot[file] ? :updated : :created
end

😕

[Drenmi]

In 0.50.0 I'm seeing HashEachMethods ask me to call each_key on an ActionController::Parameters, but Parameters does not respond to each_key.

I'm inclined to make a PR to Rails regarding this, actually. The Parameters object basically just delegates to an underlying hash, and already delegates #keys, #key? and #has_key?. Looks like an oversight that it doesn't also delegate #each_key.

[bbatsov]

(snapshot.to_a - last_snapshot.to_a).each do |file, _mtime|
  @changes[file] = last_snapshot[file] ? :updated : :created
end

@AlexWayfer I'd change the code to:

(snapshot.to_a - last_snapshot.to_a).each do |(file, _mtime)|
  @changes[file] = last_snapshot[file] ? :updated : :created
end

I know that's it's more or less the same thing, but I like denoting that I'm explicitly breaking one argument apart when dealing with arrays and this won't be flagged by RuboCop as a bonus.

[bbatsov]

Same goes for @akerl's example.

[Drenmi]

@bbatsov: Never thought of that. 🤔 I think it's good enough to go in the style guide!

[AlexWayfer]

@bbatsov OK, thank you.

[jaredbeck]

In 0.50.0 I'm seeing HashEachMethods ask me to call each_key on an ActionController::Parameters, but Parameters does not respond to each_key.

I'm still seeing this issue in rubocop 0.51.0 + rails 5.1.4. I have disabled this cop for now.

I'm inclined to make a PR to Rails regarding this, actually. The Parameters object basically just delegates to an underlying hash, and already delegates #keys, #key? and #has_key?. Looks like an oversight that it doesn't also delegate #each_key.

Works for me, Ted. Did this PR to rails ever happen? If not, I can probably do it.

[abrom]

FYI @jaredbeck, for the ActionController::Parameters you can use the each_pair method (each is just an alias to that anyway) and avoid catching up the HashEachMethods cop