From 0ad22b6950e2afaa433c36535a1907f1b07c40fd Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Mon, 26 Oct 2020 15:59:00 -0400 Subject: [PATCH] Fixes to `Lint/UnModifiedReduceAccumulator`. - Moved file to correct filename - Made examples more descriptive - Fixed not apply when reduce called with no argument or no block arguments - Fixed false positives when the element is modified by another value --- docs/modules/ROOT/pages/cops_lint.adoc | 9 +- lib/rubocop.rb | 2 +- ...or.rb => unmodified_reduce_accumulator.rb} | 33 ++- .../unmodified_reduce_accumulator_spec.rb | 264 ++++++++++++++++-- 4 files changed, 280 insertions(+), 28 deletions(-) rename lib/rubocop/cop/lint/{reduce_accumulator.rb => unmodified_reduce_accumulator.rb} (83%) diff --git a/docs/modules/ROOT/pages/cops_lint.adoc b/docs/modules/ROOT/pages/cops_lint.adoc index d252df494e1..0ffd987d3ad 100644 --- a/docs/modules/ROOT/pages/cops_lint.adoc +++ b/docs/modules/ROOT/pages/cops_lint.adoc @@ -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 @@ -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 ---- diff --git a/lib/rubocop.rb b/lib/rubocop.rb index daddae89079..a9a2c0f8d8a 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -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' @@ -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' diff --git a/lib/rubocop/cop/lint/reduce_accumulator.rb b/lib/rubocop/cop/lint/unmodified_reduce_accumulator.rb similarity index 83% rename from lib/rubocop/cop/lint/reduce_accumulator.rb rename to lib/rubocop/cop/lint/unmodified_reduce_accumulator.rb index 9e7513071b8..445e8db930a 100644 --- a/lib/rubocop/cop/lint/reduce_accumulator.rb +++ b/lib/rubocop/cop/lint/unmodified_reduce_accumulator.rb @@ -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 @@ -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 `%s` will be modified by `%s`.' MSG_INDEX = 'Do not return an element of the accumulator in `%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) @@ -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 @@ -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)) @@ -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) diff --git a/spec/rubocop/cop/lint/unmodified_reduce_accumulator_spec.rb b/spec/rubocop/cop/lint/unmodified_reduce_accumulator_spec.rb index aef0769b27f..a5ca3f3e87e 100644 --- a/spec/rubocop/cop/lint/unmodified_reduce_accumulator_spec.rb +++ b/spec/rubocop/cop/lint/unmodified_reduce_accumulator_spec.rb @@ -4,6 +4,14 @@ subject(:cop) { described_class.new } shared_examples 'reduce/inject' do |method| + it "does not affect #{method} called with no block args" do + expect_no_offenses(<<~RUBY) + values.#{method} do + do_something + end + RUBY + end + it "does not affect #{method} called without a block" do expect_no_offenses(<<~RUBY) values.#{method}(:+) @@ -21,8 +29,27 @@ end it 'registers an offense when returning the element' do + aggregate_failures do + expect_offense(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + el + ^^ Ensure the accumulator `acc` will be modified by `#{method}`. + end + RUBY + + expect_offense(<<~RUBY) + values.#{method}({}) do |acc, el| + acc[el] = true + el + ^^ Ensure the accumulator `acc` will be modified by `#{method}`. + end + RUBY + end + end + + it 'registers an offense when called with no argument' do expect_offense(<<~RUBY) - (1..4).#{method}(0) do |acc, el| + (1..4).#{method} do |acc, el| el ^^ Ensure the accumulator `acc` will be modified by `#{method}`. end @@ -62,19 +89,41 @@ end it 'does not register an offense when op-assigning the accumulator' do - expect_no_offenses(<<~RUBY) - (1..4).#{method}(0) do |acc, el| - acc += el - end - RUBY + aggregate_failures do + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + acc += 5 + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + acc += el + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + el += acc + end + RUBY + end end it 'does not register an offense when or-assigning the accumulator' do - expect_no_offenses(<<~RUBY) - (1..4).#{method}(0) do |acc, el| - acc ||= el - end - RUBY + aggregate_failures do + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + acc ||= el + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + el ||= acc + end + RUBY + end end it 'does not register an offense when returning the accumulator in a boolean statement' do @@ -94,17 +143,162 @@ end it 'does not register an offense when and-assigning the accumulator' do - expect_no_offenses(<<~RUBY) - (1..4).#{method}(0) do |acc, el| - acc &&= el - end - RUBY + aggregate_failures do + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + acc &&= el + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + el &&= acc + end + RUBY + end end it 'does not register an offense when shovelling the accumulator' do - expect_no_offenses(<<~RUBY) - (1..4).#{method}(0) do |acc, el| + aggregate_failures do + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + acc << el + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + el << acc + end + RUBY + end + end + + it 'does not register an offense when mutating the element with the accumulator' do + aggregate_failures do + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + el.method!(acc, foo) + el + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + method!(acc, foo, el) + el + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + el << acc + el + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + el << acc.foo + el + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + el += acc + el + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + el += acc.foo + el + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + el &&= acc + el + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + el &&= acc.foo + el + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + el = acc + el + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + el = acc.foo + el + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + x = acc + el << x + el + end + RUBY + end + end + + it 'does not register an offense when mutating the element with the another value' do + aggregate_failures do + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + el.method!(foo) + el + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + method!(foo, el) + el + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + x = acc + el << x + el + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method} do |acc, el| + el = x = acc + el + end + RUBY + end + end + + it 'registers an offense when mutating the accumulator with the element but not returning it' do + expect_offense(<<~RUBY) + values.#{method} do |acc, el| + acc = el + acc += el acc << el + acc &&= el + acc.method!(el) + el + ^^ Ensure the accumulator `acc` will be modified by `#{method}`. end RUBY end @@ -236,6 +430,12 @@ end RUBY + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + self.x + el + end + RUBY + expect_no_offenses(<<~RUBY) (1..4).#{method}(0) do |acc, el| x + y @@ -282,12 +482,36 @@ RUBY expect_no_offenses(<<~RUBY) - enum.inject do |acc, elem| - x = [*acc, elem] + enum.#{method} do |acc, el| + x = [*acc, el] x << 42 if foo x end RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + Foo.el + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + foo.bar.baz.el + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + self.el + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + "\#{self.el}\#{el}" + end + RUBY end end