[Fix #8859] Add new Lint/UnmodifiedReduceAccumulator cop

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
 

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

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]

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
 
 |===

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'

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 `%<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
+          `{
+            (%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