[Fixes #8564] Metrics/AbcSize: Add optional discount for repeated "attributes"

Author: marcandre

changelog/new_metricsabcsize_add_optional_discount_for.md

@@ -0,0 +1 @@
+* [#8564](https://github.com/rubocop-hq/rubocop/issues/8564): `Metrics/AbcSize`: Add optional discount for repeated "attributes". ([@marcandre][])

config/default.yml

@@ -2128,10 +2128,11 @@ Metrics/AbcSize:
     - https://en.wikipedia.org/wiki/ABC_Software_Metric
   Enabled: true
   VersionAdded: '0.27'
-  VersionChanged: '0.81'
+  VersionChanged: '<<next>>'
   # The ABC size is a calculated magnitude, so this number can be an Integer or
   # a Float.
   IgnoredMethods: []
+  CountRepeatedAttributes: true
   Max: 17
 
 Metrics/BlockLength:

lib/rubocop.rb

@@ -89,6 +89,7 @@
 require_relative 'rubocop/cop/mixin/line_length_help'
 require_relative 'rubocop/cop/mixin/match_range'
 require_relative 'rubocop/cop/metrics/utils/repeated_csend_discount'
+require_relative 'rubocop/cop/metrics/utils/repeated_attribute_discount'
 require_relative 'rubocop/cop/mixin/method_complexity'
 require_relative 'rubocop/cop/mixin/method_preference'
 require_relative 'rubocop/cop/mixin/min_body_length'

lib/rubocop/cop/metrics/abc_size.rb

@@ -7,6 +7,27 @@ module Metrics
       # configured maximum. The ABC size is based on assignments, branches
       # (method calls), and conditions. See http://c2.com/cgi/wiki?AbcMetric
       # and https://en.wikipedia.org/wiki/ABC_Software_Metric.
+      #
+      # You can have repeated "attributes" calls count as a single "branch".
+      # For this purpose, attributes are any method with no argument; no attempt
+      # is meant to distinguish actual `attr_reader` from other methods.
+      #
+      # @example CountRepeatedAttributes: false (default is true)
+      #
+      #    # `model` and `current_user`, refenced 3 times each,
+      #    # are each counted as only 1 branch each if
+      #    # `CountRepeatedAttributes` is set to 'false'
+      #
+      #    def search
+      #      @posts = model.active.visible_by(current_user)
+      #                .search(params[:q])
+      #      @posts = model.some_process(@posts, current_user)
+      #      @posts = model.another_process(@posts, current_user)
+      #
+      #      render 'pages/search/page'
+      #    end
+      #
+      # This cop also takes into account `IgnoredMethods` (defaults to `[]`)
       class AbcSize < Base
         include MethodComplexity
 
@@ -16,7 +37,10 @@ class AbcSize < Base
         private
 
         def complexity(node)
-          Utils::AbcSizeCalculator.calculate(node)
+          Utils::AbcSizeCalculator.calculate(
+            node,
+            discount_repeated_attributes: !cop_config['CountRepeatedAttributes']
+          )
         end
       end
     end

lib/rubocop/cop/metrics/utils/abc_size_calculator.rb

@@ -13,6 +13,7 @@ module Utils
         class AbcSizeCalculator
           include IteratingBlock
           include RepeatedCsendDiscount
+          prepend RepeatedAttributeDiscount
 
           # > Branch -- an explicit forward program branch out of scope -- a
           # > function call, class method call ..
@@ -24,8 +25,8 @@ class AbcSizeCalculator
           # > http://c2.com/cgi/wiki?AbcMetric
           CONDITION_NODES = CyclomaticComplexity::COUNTED_NODES.freeze
 
-          def self.calculate(node)
-            new(node).calculate
+          def self.calculate(node, discount_repeated_attributes: false)
+            new(node, discount_repeated_attributes: discount_repeated_attributes).calculate
           end
 
           # TODO: move to rubocop-ast

lib/rubocop/cop/metrics/utils/repeated_attribute_discount.rb

@@ -0,0 +1,146 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Metrics
+      module Utils
+        # @api private
+        #
+        # Identifies repetitions `{c}send` calls with no arguments:
+        #
+        #   foo.bar
+        #   foo.bar # => repeated
+        #   foo.bar.baz.qux # => inner send repeated
+        #   foo.bar.baz.other # => both inner send repeated
+        #   foo.bar(2) # => not repeated
+        #
+        # It also invalidates sequences if a receiver is reassigned:
+        #
+        #   xx.foo.bar
+        #   xx.foo.baz      # => inner send repeated
+        #   self.xx = any   # => invalidates everything so far
+        #   xx.foo.baz      # => no repetition
+        #   self.xx.foo.baz # => all repeated
+        #
+        module RepeatedAttributeDiscount
+          extend NodePattern::Macros
+          include RuboCop::AST::Sexp
+
+          # Plug into the calculator
+          def initialize(node, discount_repeated_attributes: false)
+            super(node)
+            return unless discount_repeated_attributes
+
+            self_attributes = {} # Share hash for `(send nil? :foo)` and `(send (self) :foo)`
+            @known_attributes = {
+              s(:self) => self_attributes,
+              nil => self_attributes
+            }
+            # example after running `obj = foo.bar; obj.baz.qux`
+            # { nil => {foo: {bar: {}}},
+            #   s(self) => same hash ^,
+            #   s(:lvar, :obj) => {baz: {qux: {}}}
+            # }
+          end
+
+          def discount_repeated_attributes?
+            defined?(@known_attributes)
+          end
+
+          def evaluate_branch_nodes(node)
+            return if discount_repeated_attributes? && discount_repeated_attribute?(node)
+
+            super
+          end
+
+          def calculate_node(node)
+            update_repeated_attribute(node) if discount_repeated_attributes?
+            super
+          end
+
+          private
+
+          def_node_matcher :attribute_call?, <<~PATTERN
+            ( {csend send} _receiver _method # and no parameters
+            )
+          PATTERN
+
+          def discount_repeated_attribute?(send_node)
+            return false unless attribute_call?(send_node)
+
+            repeated = true
+            find_attributes(send_node) do |hash, lookup|
+              return false if hash.nil?
+
+              repeated = false
+              hash[lookup] = {}
+            end
+
+            repeated
+          end
+
+          def update_repeated_attribute(node)
+            return unless (receiver, method = setter_to_getter(node))
+
+            calls = find_attributes(receiver) { return }
+            if method # e.g. `self.foo = 42`
+              calls.delete(method)
+            else      # e.g. `var = 42`
+              calls.clear
+            end
+          end
+
+          def_node_matcher :root_node?, <<~PATTERN
+            { nil? | self               # e.g. receiver of `my_method` or `self.my_attr`
+            | lvar | ivar | cvar | gvar # e.g. receiver of `var.my_method`
+            | const }                   # e.g. receiver of `MyConst.foo.bar`
+          PATTERN
+
+          # Returns the "known_attributes" for the `node` by walking the receiver tree
+          # If at any step the subdirectory does not exist, it is yielded with the
+          # associated key (method_name)
+          # If the node is not a series of `(c)send` calls with no arguments,
+          # then `nil` is yielded
+          def find_attributes(node, &block)
+            if attribute_call?(node)
+              calls = find_attributes(node.receiver, &block)
+              value = node.method_name
+            elsif root_node?(node)
+              calls = @known_attributes
+              value = node
+            else
+              return yield nil
+            end
+
+            calls.fetch(value) do
+              yield [calls, value]
+            end
+          end
+
+          VAR_SETTER_TO_GETTER = {
+            lvasgn: :lvar,
+            ivasgn: :ivar,
+            cvasgn: :cvar,
+            gvasgn: :gvar
+          }.freeze
+
+          # @returns `[receiver, method | nil]` for the given setter `node`
+          # or `nil` if it is not a setter.
+          def setter_to_getter(node)
+            if (type = VAR_SETTER_TO_GETTER[node.type])
+              # (lvasgn :my_var (int 42)) => [(lvar my_var), nil]
+              [s(type, node.children.first), nil]
+            elsif node.shorthand_asgn? # (or-asgn (send _receiver :foo) _value)
+              # (or-asgn (send _receiver :foo) _value) => [_receiver, :foo]
+              node.children.first.children
+            elsif node.respond_to?(:setter_method?) && node.setter_method?
+              # (send _receiver :foo= (int 42) ) => [_receiver, :foo]
+              method_name = node.method_name[0...-1].to_sym
+              [node.receiver, method_name]
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/metrics/abc_size_spec.rb

@@ -134,6 +134,36 @@ def self.foo
         end
       end
     end
+
+    context 'when CountRepeatedAttributes is `false`' do
+      let(:cop_config) { { 'Max' => 0, 'CountRepeatedAttributes' => false } }
+
+      it 'does not count repeated attributes' do
+        expect_offense(<<~RUBY)
+          def foo
+          ^^^^^^^ Assignment Branch Condition size for foo is too high. [<0, 1, 0> 1/0]
+            bar
+            self.bar
+            bar
+          end
+        RUBY
+      end
+    end
+
+    context 'when CountRepeatedAttributes is `true`' do
+      let(:cop_config) { { 'Max' => 0, 'CountRepeatedAttributes' => true } }
+
+      it 'counts repeated attributes' do
+        expect_offense(<<~RUBY)
+          def foo
+          ^^^^^^^ Assignment Branch Condition size for foo is too high. [<0, 3, 0> 3/0]
+            bar
+            self.bar
+            bar
+          end
+        RUBY
+      end
+    end
   end
 
   context 'when Max is 2' do

spec/rubocop/cop/metrics/utils/abc_size_calculator_spec.rb

@@ -2,7 +2,12 @@
 
 RSpec.describe RuboCop::Cop::Metrics::Utils::AbcSizeCalculator do
   describe '#calculate' do
-    subject(:vector) { described_class.calculate(node).last }
+    subject(:vector) do
+      described_class.calculate(node,
+                                discount_repeated_attributes: discount_repeated_attributes).last
+    end
+
+    let(:discount_repeated_attributes) { false }
 
     let(:node) { parse_source(source).ast }
 
@@ -319,5 +324,71 @@ def method_name
 
       it { is_expected.to eq '<0, 1, 0>' }
     end
+
+    context 'when counting repeated calls' do
+      let(:discount_repeated_attributes) { false }
+      let(:source) { <<~RUBY }
+        def method_name(var)
+          var.foo
+          var.foo
+          bar
+          bar
+        end
+      RUBY
+
+      it { is_expected.to eq '<1, 4, 0>' }
+    end
+
+    context 'when discounting repeated calls' do
+      let(:discount_repeated_attributes) { true }
+
+      context 'when root receiver is a var' do
+        let(:source) { <<~RUBY }
+          def method_name(var)  #   1, 0, 0
+            var.foo.bar.baz     #     +3
+            var.foo.bar.qux     #     +1
+            var.foo.bar = 42    #  +1 +1   (partial invalidation)
+            var.foo             #     +0
+            var.foo.bar         #     +1
+            var.foo.bar.baz     #     +1
+            var = 42            #  +1      (complete invalidation)
+            var.foo.bar         #     +2
+          end
+        RUBY
+
+        it { is_expected.to eq '<3, 9, 0>' }
+      end
+
+      context 'when root receiver is self/nil' do
+        let(:source) { <<~RUBY }
+          def method_name       #   0, 0, 0
+            self.foo.bar.baz    #     +3
+            foo.bar.qux         #     +1
+            foo.bar = 42        #  +1 +1    (partial invalidation)
+            foo                 #     +0
+            self.foo.bar        #     +1
+            foo&.bar.baz        #     +1    (C += 0 since `csend` treated as `send`)
+            self.foo ||= 42     #  +1 +1 +1 (complete invalidation)
+            self.foo.bar        #     +2
+          end
+        RUBY
+
+        it { is_expected.to eq '<2, 9, 1>' }
+      end
+
+      context 'when some calls have arguments' do
+        let(:source) { <<~RUBY }
+          def method_name(var)  #   1, 0, 0
+            var.foo(42).bar     #     +2
+            var.foo(42).bar     #     +2
+            var.foo.bar         #     +2
+            var.foo.bar         #     +0
+            var.foo.bar(42)     #     +1
+          end
+        RUBY
+
+        it { is_expected.to eq '<1, 7, 0>' }
+      end
+    end
   end
 end