Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autocorrect Style/HashTransformValues is broken #7767

Closed
mdemare opened this issue Feb 28, 2020 · 6 comments · Fixed by #7787
Closed

Autocorrect Style/HashTransformValues is broken #7767

mdemare opened this issue Feb 28, 2020 · 6 comments · Fixed by #7787

Comments

@mdemare
Copy link

mdemare commented Feb 28, 2020

Firstly, it transforms some usages of each_with_object where the receiver is an Array.
Secondly, looking up a missing value in the resulting hash will return a Proc object, instead of nil.

This is not a safe transformation.

@koic
Copy link
Member

koic commented Mar 1, 2020

Style/HashTransformValues is marked as unsafe.
https://github.com/rubocop-hq/rubocop/blob/v0.80.1/config/default.yml#L2824

Using rubocop --safe and rubocop --safe-auto-correct command-line options run to only safe cops. So users can omit unsafe cops using this option.

By the way, could you show me the reproduction example code of the issue?

@lucascaton
Copy link

Hi @koic! 👋

I have a reproduction example code:

[[1, 2], [3, 4]].map { |k, v| [k, [v, nil]] }.to_h
# => {1=>[2, nil], 3=>[4, nil]}

When I run $ rubocop, this is the output:

code.rb:3:1: C: Style/HashTransformValues: Prefer transform_values over map {...}.to_h.
[[1, 2], [3, 4]].map { |k, v| [k, [v, nil]] }.to_h

And running $ rubocop -a, it auto-fixes it to:

-[[1, 2], [3, 4]].map { |k, v| [k, [v, nil]] }.to_h
+[[1, 2], [3, 4]].transform_values { |v| [v, nil] }

However, the produced (above) code isn't valid:

[[1, 2], [3, 4]].transform_values { |v| [v, nil] }
# NoMethodError: undefined method `transform_values' for [[1, 2], [3, 4]]:Array

@tejasbubane
Copy link
Contributor

@koic I think this cop should skip array literals?

@koic
Copy link
Member

koic commented Mar 9, 2020

Surely Array instance does not have the transform_values method, so I agree to skip for array literals.

@koic
Copy link
Member

koic commented Mar 9, 2020

Also the transform_keys method (Style/HashTransformKeys cop) is the same.

tejasbubane added a commit to tejasbubane/rubocop that referenced this issue Mar 10, 2020
koic added a commit that referenced this issue Mar 10, 2020
[Fix #7767] Skip array literals in `Style/HashTransformValues` & `Style/HashTransformKeys`
@fsoikin
Copy link

fsoikin commented Jan 9, 2024

This is not limited to literals though. In our codebase I found hit rate of this rule (and other similar hash-related rules) to be about 60% overall. For example, this is also considered a hit:

{ x: 1,  y: 5 }.sort_by(&:first).to_h { |k, v| [k, v * 2] }

Even though sort_by returns an Array, not a Hash.

Just in general, this check reacts to the code pattern, but doesn't always make sure that the receiver object is actually a hash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants