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.

Sign up for GitHub

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

Jump to bottom

Fix issues with Lint/UnmodifiedReduceAccumulator #8949

Merged
merged 1 commit into from Oct 28, 2020
Merged

Fix issues with Lint/UnmodifiedReduceAccumulator #8949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion docs/modules/ROOT/pages/cops_lint.adoc
View file
Open in desktop
Expand Up @@ -4314,6 +4314,12 @@ end
acc + el * 2
end

# good, element is returned but modified using the accumulator
values.reduce do |acc, el|
el << acc
el
end

# good, returns the accumulator instead of the index
%w(a b c).reduce({}) do |acc, letter|
acc[letter] = true
Expand All @@ -4328,7 +4334,8 @@ end

# ignored as the return value cannot be determined
enum.reduce do |acc, el|
x + y
x = foo(acc, el)
bar(x)
end
----

Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop.rb
View file
Open in desktop
Expand Up @@ -307,7 +307,6 @@
require_relative 'rubocop/cop/lint/percent_symbol_array'
require_relative 'rubocop/cop/lint/raise_exception'
require_relative 'rubocop/cop/lint/rand_one'
require_relative 'rubocop/cop/lint/reduce_accumulator'
require_relative 'rubocop/cop/lint/redundant_cop_disable_directive'
require_relative 'rubocop/cop/lint/redundant_cop_enable_directive'
require_relative 'rubocop/cop/lint/redundant_require_statement'
Expand Down Expand Up @@ -339,6 +338,7 @@
require_relative 'rubocop/cop/lint/trailing_comma_in_attribute_declaration'
require_relative 'rubocop/cop/lint/underscore_prefixed_variable_name'
require_relative 'rubocop/cop/lint/unified_integer'
require_relative 'rubocop/cop/lint/unmodified_reduce_accumulator'
require_relative 'rubocop/cop/lint/unreachable_code'
require_relative 'rubocop/cop/lint/unreachable_loop'
require_relative 'rubocop/cop/lint/unused_block_argument'
Expand Down
33 changes: 27 additions & 6 deletions lib/rubocop/cop/lint/reduce_accumulator.rb → ...cop/lint/unmodified_reduce_accumulator.rb
View file
Open in desktop
Expand Up @@ -36,6 +36,12 @@ module Lint
# acc + el * 2
# end
#
# # good, element is returned but modified using the accumulator
# values.reduce do |acc, el|
# el << acc
# el
# end
#
# # good, returns the accumulator instead of the index
# %w(a b c).reduce({}) do |acc, letter|
# acc[letter] = true
Expand All @@ -50,20 +56,30 @@ module Lint
#
# # ignored as the return value cannot be determined
# enum.reduce do |acc, el|
# x + y
# x = foo(acc, el)
# bar(x)
# end
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) ...)
(block (send _recv {:reduce :inject} ...) (args arg+) ...)
PATTERN

def_node_matcher :accumulator_index?, <<~PATTERN
(send (lvar %1) {:[] :[]=} ...)
PATTERN

def_node_search :element_modified?, <<~PATTERN
{
(send _receiver !{:[] :[]=} <`(lvar %1) `_ ...>) # method(el, ...)
(send (lvar %1) _message <{ivar gvar cvar lvar send} ...>) # el.method(...)
(lvasgn %1 _) # el = ...
(%RuboCop::AST::Node::SHORTHAND_ASSIGNMENTS (lvasgn %1) ... _) # el += ...
}
PATTERN

def_node_matcher :lvar_used?, <<~PATTERN
{
(lvar %1)
Expand All @@ -75,12 +91,12 @@ class UnmodifiedReduceAccumulator < Base
PATTERN

def_node_search :expression_values, <<~PATTERN
`{
{
(%RuboCop::AST::Node::VARIABLES $_)
(%RuboCop::AST::Node::EQUALS_ASSIGNMENTS $_ ...)
(send (%RuboCop::AST::Node::VARIABLES $_) :<< ...)
(send nil? $_)
(dstr (begin {(send nil? $_) (%RuboCop::AST::Node::VARIABLES $_)}))
$(send _ _)
(dstr (begin {(%RuboCop::AST::Node::VARIABLES $_)}))
(%RuboCop::AST::Node::SHORTHAND_ASSIGNMENTS (%RuboCop::AST::Node::EQUALS_ASSIGNMENTS $_) ...)
}
PATTERN
Expand Down Expand Up @@ -117,7 +133,7 @@ def check_return_values(block_node)

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)
elsif potential_offense?(return_values, block_node.body, element_name, accumulator_name)
return_values.each do |return_val|
unless acceptable_return?(return_val, element_name)
add_offense(return_val, message: format(MSG, message_opts))
Expand All @@ -137,6 +153,11 @@ def returned_accumulator_index(return_values, accumulator_name)
return_values.detect { |val| accumulator_index?(val, accumulator_name) }
end

def potential_offense?(return_values, block_body, element_name, accumulator_name)
!(element_modified?(block_body, element_name) ||
returns_accumulator_anywhere?(return_values, 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)
Expand Down