From d4509a097c4993388e146c904dff4ef91ff390eb Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Wed, 21 Oct 2020 11:44:01 -0400 Subject: [PATCH] [Fix #8859] Add new `Lint/UnmodifiedReduceAccumulator` cop. --- CHANGELOG.md | 1 + config/default.yml | 5 + docs/modules/ROOT/pages/cops.adoc | 1 + docs/modules/ROOT/pages/cops_lint.adoc | 65 ++++ lib/rubocop.rb | 1 + lib/rubocop/cop/lint/reduce_accumulator.rb | 164 +++++++++ .../unmodified_reduce_accumulator_spec.rb | 334 ++++++++++++++++++ 7 files changed, 571 insertions(+) create mode 100644 lib/rubocop/cop/lint/reduce_accumulator.rb create mode 100644 spec/rubocop/cop/lint/unmodified_reduce_accumulator_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 3b05367b437..444d4afbc6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * [#8895](https://github.com/rubocop-hq/rubocop/pull/8895): Add new `Lint/EmptyBlock` cop. ([@fatkodima][]) * [#7549](https://github.com/rubocop-hq/rubocop/issues/7549): Add new `Style/ArgumentsForwarding` cop. ([@koic][]) +* [#8859](https://github.com/rubocop-hq/rubocop/issues/8859): Add new `Lint/UnmodifiedReduceAccumulator` cop. ([@dvandersluis][]) ### Changes diff --git a/config/default.yml b/config/default.yml index c4b197e88a2..f77265b10ae 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1942,6 +1942,11 @@ Lint/UnifiedInteger: Enabled: true VersionAdded: '0.43' +Lint/UnmodifiedReduceAccumulator: + Description: Checks for `reduce` or `inject` blocks that do not update the accumulator each iteration. + Enabled: pending + VersionAdded: '1.1' + Lint/UnreachableCode: Description: 'Unreachable code.' Enabled: true diff --git a/docs/modules/ROOT/pages/cops.adoc b/docs/modules/ROOT/pages/cops.adoc index 962acf8a32a..8661dffb295 100644 --- a/docs/modules/ROOT/pages/cops.adoc +++ b/docs/modules/ROOT/pages/cops.adoc @@ -274,6 +274,7 @@ In the following section you find all available cops: * xref:cops_lint.adoc#linttrailingcommainattributedeclaration[Lint/TrailingCommaInAttributeDeclaration] * xref:cops_lint.adoc#lintunderscoreprefixedvariablename[Lint/UnderscorePrefixedVariableName] * xref:cops_lint.adoc#lintunifiedinteger[Lint/UnifiedInteger] +* xref:cops_lint.adoc#lintunmodifiedreduceaccumulator[Lint/UnmodifiedReduceAccumulator] * xref:cops_lint.adoc#lintunreachablecode[Lint/UnreachableCode] * xref:cops_lint.adoc#lintunreachableloop[Lint/UnreachableLoop] * xref:cops_lint.adoc#lintunusedblockargument[Lint/UnusedBlockArgument] diff --git a/docs/modules/ROOT/pages/cops_lint.adoc b/docs/modules/ROOT/pages/cops_lint.adoc index f7c34a760ca..e2857c5f618 100644 --- a/docs/modules/ROOT/pages/cops_lint.adoc +++ b/docs/modules/ROOT/pages/cops_lint.adoc @@ -4190,6 +4190,71 @@ This cop checks for using Fixnum or Bignum constant. 1.is_a?(Integer) ---- +== Lint/UnmodifiedReduceAccumulator + +|=== +| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged + +| Pending +| Yes +| No +| 1.1 +| - +|=== + +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). + +=== Examples + +[source,ruby] +---- +# 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 + +# 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 +---- + == Lint/UnreachableCode |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index beade3dc188..481d48c164a 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -306,6 +306,7 @@ 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' diff --git a/lib/rubocop/cop/lint/reduce_accumulator.rb b/lib/rubocop/cop/lint/reduce_accumulator.rb new file mode 100644 index 00000000000..9e7513071b8 --- /dev/null +++ b/lib/rubocop/cop/lint/reduce_accumulator.rb @@ -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 + # + # # 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 + 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) ...) + 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 + `{ + (%RuboCop::AST::Node::VARIABLES $_) + (%RuboCop::AST::Node::EQUALS_ASSIGNMENTS $_ ...) + (send (%RuboCop::AST::Node::VARIABLES $_) :<< ...) + (send nil? $_) + (dstr (begin {(send nil? $_) (%RuboCop::AST::Node::VARIABLES $_)})) + (%RuboCop::AST::Node::SHORTHAND_ASSIGNMENTS (%RuboCop::AST::Node::EQUALS_ASSIGNMENTS $_) ...) + } + PATTERN + + 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 diff --git a/spec/rubocop/cop/lint/unmodified_reduce_accumulator_spec.rb b/spec/rubocop/cop/lint/unmodified_reduce_accumulator_spec.rb new file mode 100644 index 00000000000..aef0769b27f --- /dev/null +++ b/spec/rubocop/cop/lint/unmodified_reduce_accumulator_spec.rb @@ -0,0 +1,334 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Lint::UnmodifiedReduceAccumulator do + subject(:cop) { described_class.new } + + shared_examples 'reduce/inject' do |method| + it "does not affect #{method} called without a block" do + expect_no_offenses(<<~RUBY) + values.#{method}(:+) + RUBY + end + + context "given a #{method} block" do + it 'does not register an offense when returning a literal' do + expect_no_offenses(<<~RUBY) + values.reduce(true) do |result, value| + next false if something? + true + end + RUBY + end + + it 'registers an offense when returning the element' do + expect_offense(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + el + ^^ Ensure the accumulator `acc` will be modified by `#{method}`. + end + RUBY + end + + it 'does not register an offense when comparing' do + aggregate_failures do + expect_no_offenses(<<~RUBY) + values.#{method}(false) do |acc, el| + acc == el + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method}(false) do |acc, el| + el == acc + end + RUBY + end + end + + it 'does not register an offense when returning the accumulator' do + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + acc + end + RUBY + end + + it 'does not register an offense when assigning the accumulator' do + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + acc = el + end + RUBY + 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 + 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 + end + + it 'does not register an offense when returning the accumulator in a boolean statement' do + 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 and-assigning the accumulator' do + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + acc &&= el + end + RUBY + end + + it 'does not register an offense when shovelling the accumulator' do + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + acc << el + end + RUBY + end + + it 'does not register an offense with the accumulator in interpolation' do + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + "\#{acc}\#{el}" + end + RUBY + end + + it 'registers an offense with the element in interpolation' do + expect_offense(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + "\#{el}" + ^^^^^^^ Ensure the accumulator `acc` will be modified by `#{method}`. + end + RUBY + end + + it 'does not register an offense with the accumulator in heredoc' do + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + <<~RESULT + \#{acc}\#{el} + RESULT + end + RUBY + end + + it 'registers an offense with the element in heredoc' do + expect_offense(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + <<~RESULT + ^^^^^^^^^ Ensure the accumulator `acc` will be modified by `#{method}`. + \#{el} + RESULT + end + RUBY + end + + it 'does not register an offense when returning the accumulator in an expression' do + 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 a method called on the accumulator' do + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + acc.method + end + RUBY + end + + it 'does not register an offense when returning a method called with the accumulator' do + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + method(acc) + end + RUBY + end + + it 'registers an offense when returning an index setter on the accumulator' do + expect_offense(<<~RUBY) + %w(a b c).#{method}({}) do |acc, letter| + acc[letter] = true + ^^^^^^^^^^^^^^^^^^ Do not return an element of the accumulator in `#{method}`. + end + RUBY + end + + it 'registers an offense when returning an expression with the element' do + expect_offense(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + el + 2 + ^^^^^^ Ensure the accumulator `acc` will be modified by `#{method}`. + end + RUBY + end + + it 'registers an offense for values returned with `next`' do + expect_offense(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + next el if el.even? + ^^ Ensure the accumulator `acc` will be modified by `#{method}`. + acc += 1 + end + RUBY + end + + it 'registers an offense for values returned with `break`' do + expect_offense(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + break el if el.even? + ^^ Ensure the accumulator `acc` will be modified by `#{method}`. + acc += 1 + end + RUBY + end + + it 'registers an offense for every violating return value' do + expect_offense(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + next el if el.even? + ^^ Ensure the accumulator `acc` will be modified by `#{method}`. + el * 2 + ^^^^^^ Ensure the accumulator `acc` will be modified by `#{method}`. + end + RUBY + end + + it 'does not register an offense if the return value cannot be determined' do + aggregate_failures do + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + x + el + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + x + y + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + x = acc + el + x + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + x = acc + el + x ** 2 + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + x = acc + x + el + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + @var + el + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + el + @var + end + RUBY + + expect_no_offenses(<<~RUBY) + (1..4).#{method}(0) do |acc, el| + foo.bar(el) + end + RUBY + + expect_no_offenses(<<~RUBY) + enum.inject do |acc, elem| + x = [*acc, elem] + x << 42 if foo + x + end + RUBY + end + end + + it 'does not look inside inner blocks' do + expect_no_offenses(<<~RUBY) + foo.#{method}(bar) do |acc, el| + values.map do |v| + next el if something? + el + end + end + RUBY + end + + it 'handles break with no value' do + expect_no_offenses(<<~RUBY) + foo.#{method}([]) do |acc, el| + break if something? + acc << el + end + RUBY + end + + it 'allows the element to be the return value if the accumulator is returned in any branch' do + expect_no_offenses(<<~RUBY) + values.#{method}(nil) do |result, value| + break result if something? + value + end + RUBY + + expect_no_offenses(<<~RUBY) + values.#{method}(nil) do |result, value| + next value if something? + result + end + RUBY + end + end + end + + it_behaves_like 'reduce/inject', :reduce + it_behaves_like 'reduce/inject', :inject +end