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
[Fix #8859] Add new Lint/UnmodifiedReduceAccumulator
cop
#8916
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,164 @@ | ||
# frozen_string_literal: true | ||
|
||
module RuboCop | ||
module Cop | ||
module Lint | ||
# Looks for `reduce` or `inject` blocks where the value returned (implicitly or | ||
# explicitly) does not include the accumulator. A block is considered valid as | ||
# long as at least one return value includes the accumulator. | ||
# | ||
# If the accumulator is not included in the return value, then the entire | ||
# block will just return a transformation of the last element value, and | ||
# could be rewritten as such without a loop. | ||
# | ||
# Also catches instances where an index of the accumulator is returned, as | ||
# this may change the type of object being retained. | ||
# | ||
# NOTE: For the purpose of reducing false positives, this cop only flags | ||
# returns in `reduce` blocks where the element is the only variable in | ||
# the expression (since we will not be able to tell what other variables | ||
# relate to via static analysis). | ||
# | ||
# @example | ||
# | ||
# # bad | ||
# (1..4).reduce(0) do |acc, el| | ||
# el * 2 | ||
# end | ||
# | ||
# # bad, may raise a NoMethodError after the first iteration | ||
# %w(a b c).reduce({}) do |acc, letter| | ||
# acc[letter] = true | ||
# end | ||
Comment on lines
+30
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue is in the word def chain(*values)
values.inject({value: nil}) do |acc, elem|
node = {value: elem, next: acc}
acc[:prev] = node
end
end There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I concede that that is a valid use that will be registered by the cop, but I'd argue that this is going to be much less prevalent than mistakenly forgetting to return the accumulator when setting a hash key. I see a few options:
What do we think? /cc @bbatsov There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
BTW this is a double error, as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely true, but in this case |
||
# | ||
# # good | ||
# (1..4).reduce(0) do |acc, el| | ||
# acc + el * 2 | ||
# end | ||
# | ||
# # good, returns the accumulator instead of the index | ||
# %w(a b c).reduce({}) do |acc, letter| | ||
# acc[letter] = true | ||
# acc | ||
# end | ||
# | ||
# # good, at least one branch returns the accumulator | ||
# values.reduce(nil) do |result, value| | ||
# break result if something? | ||
# value | ||
# end | ||
# | ||
# # ignored as the return value cannot be determined | ||
# enum.reduce do |acc, el| | ||
# x + y | ||
# end | ||
Comment on lines
+51
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bad example, as it could be determined that this is an error. |
||
class UnmodifiedReduceAccumulator < Base | ||
MSG = 'Ensure the accumulator `%<accum>s` will be modified by `%<method>s`.' | ||
MSG_INDEX = 'Do not return an element of the accumulator in `%<method>s`.' | ||
|
||
def_node_matcher :reduce_with_block?, <<~PATTERN | ||
(block (send _recv {:reduce :inject} !sym) ...) | ||
PATTERN | ||
|
||
def_node_matcher :accumulator_index?, <<~PATTERN | ||
(send (lvar %1) {:[] :[]=} ...) | ||
PATTERN | ||
|
||
def_node_matcher :lvar_used?, <<~PATTERN | ||
{ | ||
(lvar %1) | ||
(lvasgn %1 ...) | ||
(send (lvar %1) :<< ...) | ||
(dstr (begin (lvar %1))) | ||
(%RuboCop::AST::Node::SHORTHAND_ASSIGNMENTS (lvasgn %1)) | ||
} | ||
PATTERN | ||
|
||
def_node_search :expression_values, <<~PATTERN | ||
`{ | ||
marcandre marked this conversation as resolved.
Show resolved
Hide resolved
|
||
(%RuboCop::AST::Node::VARIABLES $_) | ||
(%RuboCop::AST::Node::EQUALS_ASSIGNMENTS $_ ...) | ||
(send (%RuboCop::AST::Node::VARIABLES $_) :<< ...) | ||
(send nil? $_) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This mistakes the name of a method with the name of a variable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually needed, because I'm looking for identifiers in general, not specifically variables. This is valid code and should not be flagged since we can't determine what (1..4).reduce(0) do |acc, el|
method + el
end There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm confused as to why there's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (BTW, please do not "resolve conversation" when there's no agreement) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right there shouldn't be! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry about that! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok, if you remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call, I've fixed that now too. (btw I've been taking all your notes and applying them locally for the purpose of a new fix PR) |
||
(dstr (begin {(send nil? $_) (%RuboCop::AST::Node::VARIABLES $_)})) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here (first part of |
||
(%RuboCop::AST::Node::SHORTHAND_ASSIGNMENTS (%RuboCop::AST::Node::EQUALS_ASSIGNMENTS $_) ...) | ||
} | ||
PATTERN | ||
Comment on lines
+77
to
+86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @marcandre I feel like you probably have a simpler way to do this 😁 |
||
|
||
def on_block(node) | ||
return unless reduce_with_block?(node) | ||
|
||
check_return_values(node) | ||
end | ||
|
||
private | ||
|
||
# Return values in a block are either the value given to next, | ||
# the last line of a multiline block, or the only line of the block | ||
def return_values(block_body_node) | ||
nodes = [block_body_node.begin_type? ? block_body_node.child_nodes.last : block_body_node] | ||
|
||
block_body_node.each_descendant(:next, :break) do |n| | ||
# Ignore `next`/`break` inside an inner block | ||
next if n.each_ancestor(:block).first != block_body_node.parent | ||
next unless n.first_argument | ||
|
||
nodes << n.first_argument | ||
end | ||
|
||
nodes | ||
end | ||
|
||
def check_return_values(block_node) | ||
return_values = return_values(block_node.body) | ||
accumulator_name = block_arg_name(block_node, 0) | ||
element_name = block_arg_name(block_node, 1) | ||
message_opts = { method: block_node.method_name, accum: accumulator_name } | ||
|
||
if (node = returned_accumulator_index(return_values, accumulator_name)) | ||
add_offense(node, message: format(MSG_INDEX, message_opts)) | ||
elsif !returns_accumulator_anywhere?(return_values, accumulator_name) | ||
return_values.each do |return_val| | ||
unless acceptable_return?(return_val, element_name) | ||
add_offense(return_val, message: format(MSG, message_opts)) | ||
end | ||
end | ||
end | ||
end | ||
|
||
def block_arg_name(node, index) | ||
node.arguments[index].node_parts[0] | ||
end | ||
|
||
# Look for an index of the accumulator being returned | ||
# This is always an offense, in order to try to catch potential exceptions | ||
# due to type mismatches | ||
def returned_accumulator_index(return_values, accumulator_name) | ||
return_values.detect { |val| accumulator_index?(val, accumulator_name) } | ||
end | ||
|
||
# If the accumulator is used in any return value, the node is acceptable since | ||
# the accumulator has a chance to change each iteration | ||
def returns_accumulator_anywhere?(return_values, accumulator_name) | ||
return_values.any? { |node| lvar_used?(node, accumulator_name) } | ||
end | ||
|
||
# Determine if a return value is acceptable for the purposes of this cop | ||
# If it is an expression containing the accumulator, it is acceptable | ||
# Otherwise, it is only unacceptable if it contains the iterated element, since we | ||
# otherwise do not have enough information to prevent false positives. | ||
def acceptable_return?(return_val, element_name) | ||
vars = expression_values(return_val).uniq | ||
return true if vars.none? || (vars - [element_name]).any? | ||
|
||
false | ||
end | ||
|
||
# Exclude `begin` nodes inside a `dstr` from being collected by `return_values` | ||
def allowed_type?(parent_node) | ||
!parent_node.dstr_type? | ||
end | ||
end | ||
end | ||
end | ||
end |
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.
Bad example,
acc
is not used at all. There are already cops that deal with this, and Ruby too.Even if
acc
was used, this could still lead to false negatives: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.
Do you mean
Lint/UnusedBlockArgument
here? While that's true it doesn't really convey the same thing that this cop does. (I don't see any other offenses for the example, so I think it's valid).I think you meant false positives (false negatives are inherent here on purpose), but you're right that there is a class of false positives here that I did not consider. I'm looking at this now, thanks!
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.
specs only check for the current cop, you can't rely on that.
Yes, sorry 😅
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.
@marcandre no I meant I ran all of rubocop on the example code haha, it raised
Lint/UnusedBlockArgument
and this cop. 😁