From 6f754c5f82c6f06b5299362a1f3c9dc004efd47c Mon Sep 17 00:00:00 2001 From: Eugene Kenny Date: Mon, 10 Aug 2020 23:50:02 +0100 Subject: [PATCH] Handle to_h with block in hash transformation cops Since Ruby 2.6, `Hash#to_h` accepts a block which is called with each pair and must return a key and value to include in the new hash. If either the key or the value are always returned unchanged, using `Hash#transform_keys` or `Hash#transform_values` is faster and clearer. This is similar to the existing logic that detects `map { ... }.to_h`. --- CHANGELOG.md | 1 + config/default.yml | 6 ++++-- docs/modules/ROOT/pages/cops_style.adoc | 12 +++++++---- .../cop/mixin/hash_transform_method.rb | 17 +++++++++++++++ lib/rubocop/cop/style/hash_transform_keys.rb | 15 ++++++++++++- .../cop/style/hash_transform_values.rb | 15 ++++++++++++- .../cop/style/hash_transform_keys_spec.rb | 21 +++++++++++++++++++ .../cop/style/hash_transform_values_spec.rb | 21 +++++++++++++++++++ 8 files changed, 100 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d95480a8e6..06210e20d06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * [#8362](https://github.com/rubocop-hq/rubocop/issues/8362): Add numbers of correctable offenses to summary. ([@nguyenquangminh0711][]) * [#8513](https://github.com/rubocop-hq/rubocop/pull/8513): Clarify the ruby warning mentioned in the `Lint/ShadowingOuterLocalVariable` documentation. ([@chocolateboy][]) +* [#8517](https://github.com/rubocop-hq/rubocop/pull/8517): Make `Style/HashTransformKeys` and `Style/HashTransformValues` aware of `to_h` with block. ([@eugeneius][]) ## 0.89.1 (2020-08-10) diff --git a/config/default.yml b/config/default.yml index d8e2d72c4d7..18f78270b33 100644 --- a/config/default.yml +++ b/config/default.yml @@ -3101,15 +3101,17 @@ Style/HashSyntax: PreferHashRocketsForNonAlnumEndingSymbols: false Style/HashTransformKeys: - Description: 'Prefer `transform_keys` over `each_with_object` and `map`.' + Description: 'Prefer `transform_keys` over `each_with_object`, `map`, or `to_h`.' Enabled: 'pending' VersionAdded: '0.80' + VersionChanged: '0.90' Safe: false Style/HashTransformValues: - Description: 'Prefer `transform_values` over `each_with_object` and `map`.' + Description: 'Prefer `transform_values` over `each_with_object`, `map`, or `to_h`.' Enabled: 'pending' VersionAdded: '0.80' + VersionChanged: '0.90' Safe: false Style/IdenticalConditionalBranches: diff --git a/docs/modules/ROOT/pages/cops_style.adoc b/docs/modules/ROOT/pages/cops_style.adoc index a5b887b6c4b..5e2bb89bbfd 100644 --- a/docs/modules/ROOT/pages/cops_style.adoc +++ b/docs/modules/ROOT/pages/cops_style.adoc @@ -3794,7 +3794,7 @@ NOTE: Required Ruby version: 2.5 | No | Yes (Unsafe) | 0.80 -| - +| 0.90 |=== This cop looks for uses of `_.each_with_object({}) {...}`, @@ -3815,7 +3815,9 @@ This cop should only be enabled on Ruby version 2.5 or newer ---- # bad {a: 1, b: 2}.each_with_object({}) { |(k, v), h| h[foo(k)] = v } -{a: 1, b: 2}.map { |k, v| [k.to_s, v] } +Hash[{a: 1, b: 2}.collect { |k, v| [foo(k), v] }] +{a: 1, b: 2}.map { |k, v| [k.to_s, v] }.to_h +{a: 1, b: 2}.to_h { |k, v| [k.to_s, v] } # good {a: 1, b: 2}.transform_keys { |k| foo(k) } @@ -3831,7 +3833,7 @@ This cop should only be enabled on Ruby version 2.5 or newer | No | Yes (Unsafe) | 0.80 -| - +| 0.90 |=== This cop looks for uses of `_.each_with_object({}) {...}`, @@ -3852,7 +3854,9 @@ This cop should only be enabled on Ruby version 2.4 or newer ---- # bad {a: 1, b: 2}.each_with_object({}) { |(k, v), h| h[k] = foo(v) } -{a: 1, b: 2}.map { |k, v| [k, v * v] } +Hash[{a: 1, b: 2}.collect { |k, v| [k, foo(v)] }] +{a: 1, b: 2}.map { |k, v| [k, v * v] }.to_h +{a: 1, b: 2}.to_h { |k, v| [k, v * v] } # good {a: 1, b: 2}.transform_values { |v| foo(v) } diff --git a/lib/rubocop/cop/mixin/hash_transform_method.rb b/lib/rubocop/cop/mixin/hash_transform_method.rb index c0ac161a321..f86a4771b12 100644 --- a/lib/rubocop/cop/mixin/hash_transform_method.rb +++ b/lib/rubocop/cop/mixin/hash_transform_method.rb @@ -9,6 +9,12 @@ def on_block(node) on_bad_each_with_object(node) do |*match| handle_possible_offense(node, match, 'each_with_object') end + + return if target_ruby_version < 2.6 + + on_bad_to_h(node) do |*match| + handle_possible_offense(node, match, 'to_h {...}') + end end def on_send(node) @@ -43,6 +49,11 @@ def on_bad_map_to_h(_node) raise NotImplementedError end + # @abstract Implemented with `def_node_matcher` + def on_bad_to_h(_node) + raise NotImplementedError + end + def handle_possible_offense(node, match, match_desc) captures = extract_captures(match) @@ -82,6 +93,8 @@ def prepare_correction(node) Autocorrection.from_hash_brackets_map(node, match) elsif (match = on_bad_map_to_h(node)) Autocorrection.from_map_to_h(node, match) + elsif (match = on_bad_to_h(node)) + Autocorrection.from_to_h(node, match) else raise 'unreachable' end @@ -137,6 +150,10 @@ def self.from_map_to_h(node, match) new(match, node.children.first, 0, strip_trailing_chars) end + def self.from_to_h(node, match) + new(match, node, 0, 0) + end + def strip_prefix_and_suffix(node, corrector) expression = node.loc.expression corrector.remove_leading(expression, leading) diff --git a/lib/rubocop/cop/style/hash_transform_keys.rb b/lib/rubocop/cop/style/hash_transform_keys.rb index b145b7698d7..87e45fc0b40 100644 --- a/lib/rubocop/cop/style/hash_transform_keys.rb +++ b/lib/rubocop/cop/style/hash_transform_keys.rb @@ -18,7 +18,9 @@ module Style # @example # # bad # {a: 1, b: 2}.each_with_object({}) { |(k, v), h| h[foo(k)] = v } - # {a: 1, b: 2}.map { |k, v| [k.to_s, v] } + # Hash[{a: 1, b: 2}.collect { |k, v| [foo(k), v] }] + # {a: 1, b: 2}.map { |k, v| [k.to_s, v] }.to_h + # {a: 1, b: 2}.to_h { |k, v| [k.to_s, v] } # # # good # {a: 1, b: 2}.transform_keys { |k| foo(k) } @@ -68,6 +70,17 @@ class HashTransformKeys < Base :to_h) PATTERN + def_node_matcher :on_bad_to_h, <<~PATTERN + (block + ({send csend} + !{(send _ :each_with_index) (array ...)} + :to_h) + (args + (arg $_) + (arg _val)) + (array $_ $(lvar _val))) + PATTERN + private def extract_captures(match) diff --git a/lib/rubocop/cop/style/hash_transform_values.rb b/lib/rubocop/cop/style/hash_transform_values.rb index dfb9a970c03..0b343563023 100644 --- a/lib/rubocop/cop/style/hash_transform_values.rb +++ b/lib/rubocop/cop/style/hash_transform_values.rb @@ -18,7 +18,9 @@ module Style # @example # # bad # {a: 1, b: 2}.each_with_object({}) { |(k, v), h| h[k] = foo(v) } - # {a: 1, b: 2}.map { |k, v| [k, v * v] } + # Hash[{a: 1, b: 2}.collect { |k, v| [k, foo(v)] }] + # {a: 1, b: 2}.map { |k, v| [k, v * v] }.to_h + # {a: 1, b: 2}.to_h { |k, v| [k, v * v] } # # # good # {a: 1, b: 2}.transform_values { |v| foo(v) } @@ -65,6 +67,17 @@ class HashTransformValues < Base :to_h) PATTERN + def_node_matcher :on_bad_to_h, <<~PATTERN + (block + ({send csend} + !{(send _ :each_with_index) (array ...)} + :to_h) + (args + (arg _key) + (arg $_)) + (array $(lvar _key) $_)) + PATTERN + private def extract_captures(match) diff --git a/spec/rubocop/cop/style/hash_transform_keys_spec.rb b/spec/rubocop/cop/style/hash_transform_keys_spec.rb index 83140dba94d..394de365041 100644 --- a/spec/rubocop/cop/style/hash_transform_keys_spec.rb +++ b/spec/rubocop/cop/style/hash_transform_keys_spec.rb @@ -171,4 +171,25 @@ expect_no_offenses('x.each_with_object({}) {|(k, v), h| h[foo(k)] = v}') end end + + context 'when using Ruby 2.6 or newer', :ruby26 do + it 'flags _.to_h{...} when transform_keys could be used' do + expect_offense(<<~RUBY) + x.to_h {|k, v| [k.to_sym, v]} + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `transform_keys` over `to_h {...}`. + RUBY + + expect_correction(<<~RUBY) + x.transform_keys {|k| k.to_sym} + RUBY + end + end + + context 'below Ruby 2.6', :ruby25 do + it 'does not flag _.to_h{...}' do + expect_no_offenses(<<~RUBY) + x.to_h {|k, v| [k.to_sym, v]} + RUBY + end + end end diff --git a/spec/rubocop/cop/style/hash_transform_values_spec.rb b/spec/rubocop/cop/style/hash_transform_values_spec.rb index 534de782027..614c960e282 100644 --- a/spec/rubocop/cop/style/hash_transform_values_spec.rb +++ b/spec/rubocop/cop/style/hash_transform_values_spec.rb @@ -133,4 +133,25 @@ {a: 1, b: 2}.transform_values {|v| foo(v)}.to_h {|k, v| [v, k]} RUBY end + + context 'when using Ruby 2.6 or newer', :ruby26 do + it 'flags _.to_h{...} when transform_values could be used' do + expect_offense(<<~RUBY) + x.to_h {|k, v| [k, foo(v)]} + ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer `transform_values` over `to_h {...}`. + RUBY + + expect_correction(<<~RUBY) + x.transform_values {|v| foo(v)} + RUBY + end + end + + context 'below Ruby 2.6', :ruby25 do + it 'does not flag _.to_h{...}' do + expect_no_offenses(<<~RUBY) + x.to_h {|k, v| [k, foo(v)]} + RUBY + end + end end