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
Add cop to enforce use of transform_keys
and transform_values
#7663
Merged
bbatsov
merged 8 commits into
rubocop:master
from
djudd-stripe:hash-transform-methods-cop
Feb 4, 2020
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
740d477
Add cop to enforce use of `transform_keys` and `transform_values`
djudd-stripe b8d5caf
Add changelog for Style/HashTransformMethods
djudd-stripe 1806e4d
Add minimum_target_ruby_version for Style/HashTransformMethods
djudd-stripe e054541
PR feedback
djudd-stripe f3caf90
credit eugeneius
djudd-stripe da78815
Split cop into HashTransformKeys and HashTransformValues
djudd-stripe 09013f0
Fix to_h with block, and refactor
djudd-stripe 2ac7ef4
Merge branch 'master' into hash-transform-methods-cop
bbatsov File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,172 @@ | ||
# frozen_string_literal: true | ||
|
||
module RuboCop | ||
module Cop | ||
# Common functionality for Style/HashTransformKeys and | ||
# Style/HashTransformValues | ||
module HashTransformMethod | ||
def on_block(node) | ||
on_bad_each_with_object(node) do |*match| | ||
handle_possible_offense(node, match, 'each_with_object') | ||
end | ||
end | ||
|
||
def on_send(node) | ||
on_bad_hash_brackets_map(node) do |*match| | ||
handle_possible_offense(node, match, 'Hash[_.map {...}]') | ||
end | ||
on_bad_map_to_h(node) do |*match| | ||
handle_possible_offense(node, match, 'map {...}.to_h') | ||
end | ||
end | ||
|
||
def on_csend(node) | ||
on_bad_map_to_h(node) do |*match| | ||
handle_possible_offense(node, match, 'map {...}.to_h') | ||
end | ||
end | ||
|
||
def autocorrect(node) | ||
lambda do |corrector| | ||
correction = prepare_correction(node) | ||
execute_correction(corrector, node, correction) | ||
end | ||
end | ||
|
||
private | ||
|
||
# @abstract Implemented with `def_node_matcher` | ||
def on_bad_each_with_object(_node) | ||
raise NotImplementedError | ||
end | ||
|
||
# @abstract Implemented with `def_node_matcher` | ||
def on_bad_hash_brackets_map(_node) | ||
raise NotImplementedError | ||
end | ||
|
||
# @abstract Implemented with `def_node_matcher` | ||
def on_bad_map_to_h(_node) | ||
raise NotImplementedError | ||
end | ||
|
||
def handle_possible_offense(node, match, match_desc) | ||
puts node.class | ||
captures = extract_captures(match) | ||
|
||
# If key didn't actually change either, this is most likely a false | ||
# positive (receiver isn't a hash). | ||
return if captures.noop_transformation? | ||
|
||
# Can't `transform_keys` if key transformation uses value, or | ||
# `transform_values` if value transformation uses key. | ||
return if captures.transformation_uses_both_args? | ||
|
||
add_offense( | ||
node, | ||
message: "Prefer `#{new_method_name}` over `#{match_desc}`." | ||
) | ||
end | ||
|
||
# @abstract | ||
# | ||
# @return [Captures] | ||
def extract_captures(_match) | ||
raise NotImplementedError | ||
end | ||
|
||
# @abstract | ||
# | ||
# @return [String] | ||
def new_method_name | ||
raise NotImplementedError | ||
end | ||
|
||
def prepare_correction(node) | ||
if (match = on_bad_each_with_object(node)) | ||
Autocorrection.from_each_with_object(node, match) | ||
elsif (match = on_bad_hash_brackets_map(node)) | ||
Autocorrection.from_hash_brackets_map(node, match) | ||
elsif (match = on_bad_map_to_h(node)) | ||
Autocorrection.from_map_to_h(node, match) | ||
else | ||
raise 'unreachable' | ||
end | ||
end | ||
|
||
def execute_correction(corrector, node, correction) | ||
correction.strip_prefix_and_suffix(node, corrector) | ||
correction.set_new_method_name(new_method_name, corrector) | ||
|
||
captures = extract_captures(correction.match) | ||
correction.set_new_arg_name(captures.transformed_argname, corrector) | ||
correction.set_new_body_expression( | ||
captures.transforming_body_expr, | ||
corrector | ||
) | ||
end | ||
|
||
# Internal helper class to hold match data | ||
Captures = Struct.new( | ||
:transformed_argname, | ||
:transforming_body_expr, | ||
:unchanged_body_expr | ||
) do | ||
def noop_transformation? | ||
transforming_body_expr.lvar_type? && | ||
transforming_body_expr.children == [transformed_argname] | ||
end | ||
|
||
def transformation_uses_both_args? | ||
transforming_body_expr.descendants.include?(unchanged_body_expr) | ||
end | ||
end | ||
|
||
# Internal helper class to hold autocorrect data | ||
Autocorrection = Struct.new(:match, :block_node, :leading, :trailing) do # rubocop:disable Metrics/BlockLength | ||
def self.from_each_with_object(node, match) | ||
new(match, node, 0, 0) | ||
end | ||
|
||
def self.from_hash_brackets_map(node, match) | ||
new(match, node.children.last, 'Hash['.length, ']'.length) | ||
end | ||
|
||
def self.from_map_to_h(node, match) | ||
strip_trailing_chars = node.parent&.block_type? ? 0 : '.to_h'.length | ||
new(match, node.children.first, 0, strip_trailing_chars) | ||
end | ||
|
||
def strip_prefix_and_suffix(node, corrector) | ||
expression = node.loc.expression | ||
corrector.remove_leading(expression, leading) | ||
corrector.remove_trailing(expression, trailing) | ||
end | ||
|
||
def set_new_method_name(new_method_name, corrector) | ||
range = block_node.send_node.loc.selector | ||
if (send_end = block_node.send_node.loc.end) | ||
# If there are arguments (only true in the `each_with_object` | ||
# case) | ||
range = range.begin.join(send_end) | ||
end | ||
corrector.replace(range, new_method_name) | ||
end | ||
|
||
def set_new_arg_name(transformed_argname, corrector) | ||
corrector.replace( | ||
block_node.arguments.loc.expression, | ||
"|#{transformed_argname}|" | ||
) | ||
end | ||
|
||
def set_new_body_expression(transforming_body_expr, corrector) | ||
corrector.replace( | ||
block_node.body.loc.expression, | ||
transforming_body_expr.loc.expression.source | ||
) | ||
end | ||
end | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
# frozen_string_literal: true | ||
|
||
module RuboCop | ||
module Cop | ||
module Style | ||
# This cop looks for uses of `_.each_with_object({}) {...}`, | ||
# `_.map {...}.to_h`, and `Hash[_.map {...}]` that are actually just | ||
# transforming the keys of a hash, and tries to use a simpler & faster | ||
# call to `transform_keys` instead. | ||
# | ||
# This can produce false positives if we are transforming an enumerable | ||
# of key-value-like pairs that isn't actually a hash, e.g.: | ||
# `[[k1, v1], [k2, v2], ...]` | ||
# | ||
# This cop should only be enabled on Ruby version 2.5 or newer | ||
# (`transform_keys` was added in Ruby 2.5.) | ||
# | ||
# @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] } | ||
# | ||
# # good | ||
# {a: 1, b: 2}.transform_keys { |k| foo(k) } | ||
# {a: 1, b: 2}.transform_keys { |k| k.to_s } | ||
class HashTransformKeys < Cop | ||
extend TargetRubyVersion | ||
include HashTransformMethod | ||
|
||
minimum_target_ruby_version 2.5 | ||
|
||
def_node_matcher :on_bad_each_with_object, <<~PATTERN | ||
(block | ||
({send csend} !(send _ :each_with_index) :each_with_object (hash)) | ||
(args | ||
(mlhs | ||
(arg $_) | ||
(arg _val)) | ||
(arg _memo)) | ||
({send csend} (lvar _memo) :[]= $_ $(lvar _val))) | ||
PATTERN | ||
|
||
def_node_matcher :on_bad_hash_brackets_map, <<~PATTERN | ||
(send | ||
(const _ :Hash) | ||
:[] | ||
(block | ||
({send csend} !(send _ :each_with_index) {:map :collect}) | ||
(args | ||
(arg $_) | ||
(arg _val)) | ||
(array $_ $(lvar _val)))) | ||
PATTERN | ||
|
||
def_node_matcher :on_bad_map_to_h, <<~PATTERN | ||
({send csend} | ||
(block | ||
({send csend} !(send _ :each_with_index) {:map :collect}) | ||
(args | ||
(arg $_) | ||
(arg _val)) | ||
(array $_ $(lvar _val))) | ||
:to_h) | ||
PATTERN | ||
|
||
private | ||
|
||
def extract_captures(match) | ||
key_argname, key_body_expr, val_body_expr = *match | ||
Captures.new(key_argname, key_body_expr, val_body_expr) | ||
end | ||
|
||
def new_method_name | ||
'transform_keys' | ||
end | ||
end | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
# frozen_string_literal: true | ||
|
||
module RuboCop | ||
module Cop | ||
module Style | ||
# This cop looks for uses of `_.each_with_object({}) {...}`, | ||
# `_.map {...}.to_h`, and `Hash[_.map {...}]` that are actually just | ||
# transforming the values of a hash, and tries to use a simpler & faster | ||
# call to `transform_values` instead. | ||
# | ||
# This can produce false positives if we are transforming an enumerable | ||
# of key-value-like pairs that isn't actually a hash, e.g.: | ||
# `[[k1, v1], [k2, v2], ...]` | ||
# | ||
# This cop should only be enabled on Ruby version 2.4 or newer | ||
# (`transform_values` was added in Ruby 2.4.) | ||
# | ||
# @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] } | ||
# | ||
# # good | ||
# {a: 1, b: 2}.transform_values { |v| foo(v) } | ||
# {a: 1, b: 2}.transform_values { |v| v * v } | ||
class HashTransformValues < Cop | ||
extend TargetRubyVersion | ||
include HashTransformMethod | ||
|
||
minimum_target_ruby_version 2.4 | ||
|
||
def_node_matcher :on_bad_each_with_object, <<~PATTERN | ||
(block | ||
({send csend} !(send _ :each_with_index) :each_with_object (hash)) | ||
(args | ||
(mlhs | ||
(arg _key) | ||
(arg $_)) | ||
(arg _memo)) | ||
({send csend} (lvar _memo) :[]= $(lvar _key) $_)) | ||
PATTERN | ||
|
||
def_node_matcher :on_bad_hash_brackets_map, <<~PATTERN | ||
(send | ||
(const _ :Hash) | ||
:[] | ||
(block | ||
({send csend} !(send _ :each_with_index) {:map :collect}) | ||
(args | ||
(arg _key) | ||
(arg $_)) | ||
(array $(lvar _key) $_))) | ||
PATTERN | ||
|
||
def_node_matcher :on_bad_map_to_h, <<~PATTERN | ||
({send csend} | ||
(block | ||
({send csend} !(send _ :each_with_index) {:map :collect}) | ||
(args | ||
(arg _key) | ||
(arg $_)) | ||
(array $(lvar _key) $_)) | ||
:to_h) | ||
PATTERN | ||
|
||
private | ||
|
||
def extract_captures(match) | ||
val_argname, key_body_expr, val_body_expr = *match | ||
Captures.new(val_argname, val_body_expr, key_body_expr) | ||
end | ||
|
||
def new_method_name | ||
'transform_values' | ||
end | ||
end | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should have a cop for catching
puts
? 😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! For example, it would be good to prepare a cop of
InternalAffairs
department for lib/rubocop/cop directory. At this time,puts
method is not used in this directory.