Many fixes to Metrics/AbcSize. Default bumped to 17

Now counts correctly ||=, &&=, multiple assignments, for, yield, iterating blocks. All &. count as a condition, unless it's on a repeated and unchanged lvar [see #8276]
rubocop · Jul 29, 2020 · 2d26a11 · 2d26a11
1 parent c6fcfca
commit 2d26a11
Show file tree

Hide file tree

Showing 19 changed files with 272 additions and 33 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -30,6 +30,7 @@
 * [#8376](https://github.com/rubocop-hq/rubocop/pull/8376): `Style/MethodMissingSuper` cop is removed in favor of new `Lint/MissingSuper` cop. ([@fatkodima][])
 * [#8350](https://github.com/rubocop-hq/rubocop/pull/8350): Set default max line length to 120 for `Style/MultilineMethodSignature`. ([@koic][])
 * [#8338](https://github.com/rubocop-hq/rubocop/pull/8338): **potentially breaking**. Config#for_department now returns only the config specified for that department; the 'Enabled' attribute is no longer calculated. ([@marcandre][])
+* [#8037](https://github.com/rubocop-hq/rubocop/pull/8037): **(Breaking)** Cop `Metrics/AbcSize` now counts ||=, &&=, multiple assignments, for, yield, iterating blocks. `&.` now count as conditions too (unless repeated on the same variable). Default bumped from 15 to 16.5. ([@marcandre][])
 
 ## 0.88.0 (2020-07-13)
 

diff --git a/config/default.yml b/config/default.yml
@@ -1933,7 +1933,7 @@ Metrics/AbcSize:
   # The ABC size is a calculated magnitude, so this number can be an Integer or
   # a Float.
   IgnoredMethods: []
-  Max: 15
+  Max: 17
 
 Metrics/BlockLength:
   Description: 'Avoid long blocks with many lines.'

diff --git a/docs/modules/ROOT/pages/cops_metrics.adoc b/docs/modules/ROOT/pages/cops_metrics.adoc
@@ -27,7 +27,7 @@ and https://en.wikipedia.org/wiki/ABC_Software_Metric.
 | Array
 
 | Max
-| `15`
+| `17`
 | Integer
 |===
 

diff --git a/lib/rubocop.rb b/lib/rubocop.rb
@@ -81,6 +81,7 @@
 require_relative 'rubocop/cop/mixin/interpolation'
 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/mixin/method_complexity'
 require_relative 'rubocop/cop/mixin/method_preference'
 require_relative 'rubocop/cop/mixin/min_body_length'

diff --git a/lib/rubocop/comment_config.rb b/lib/rubocop/comment_config.rb
@@ -55,7 +55,7 @@ def extra_enabled_comments_with_names(extras, names)
       extras
     end
 
-    def analyze
+    def analyze # rubocop:todo Metrics/AbcSize
       analyses = Hash.new { |hash, key| hash[key] = CopAnalysis.new([], nil) }
 
       each_mentioned_cop do |cop_name, disabled, line, single_line|

diff --git a/lib/rubocop/config_loader.rb b/lib/rubocop/config_loader.rb
@@ -34,7 +34,7 @@ def clear_options
         FileFinder.root_level = nil
       end
 
-      def load_file(file) # rubocop:disable Metrics/AbcSize
+      def load_file(file)
         path = File.absolute_path(file.is_a?(RemoteConfig) ? file.file : file)
 
         hash = load_yaml_configuration(path)

diff --git a/lib/rubocop/config_validator.rb b/lib/rubocop/config_validator.rb
@@ -23,7 +23,6 @@ def initialize(config)
       @target_ruby = TargetRuby.new(config)
     end
 
-    # rubocop:disable Metrics/AbcSize
     def validate
       check_cop_config_value(@config)
       reject_conflicting_safe_settings
@@ -45,7 +44,6 @@ def validate
       validate_syntax_cop
       reject_mutually_exclusive_defaults
     end
-    # rubocop:enable Metrics/AbcSize
 
     def target_ruby_version
       target_ruby.version
@@ -150,7 +148,7 @@ def each_invalid_parameter(cop_name)
       end
     end
 
-    def validate_enforced_styles(valid_cop_names)
+    def validate_enforced_styles(valid_cop_names) # rubocop:todo Metrics/AbcSize
       valid_cop_names.each do |name|
         styles = @config[name].select { |key, _| key.start_with?('Enforced') }
 

diff --git a/lib/rubocop/cop/layout/rescue_ensure_alignment.rb b/lib/rubocop/cop/layout/rescue_ensure_alignment.rb
@@ -96,7 +96,6 @@ def format_message(alignment_node, alignment_loc, kw_loc)
           )
         end
 
-        # rubocop:disable Metrics/AbcSize
         def alignment_source(node, starting_loc)
           ending_loc =
             case node.type
@@ -115,7 +114,6 @@ def alignment_source(node, starting_loc)
 
           range_between(starting_loc.begin_pos, ending_loc.end_pos).source
         end
-        # rubocop:enable Metrics/AbcSize
 
         # We will use ancestor or wrapper with access modifier.
 

diff --git a/lib/rubocop/cop/lint/redundant_cop_disable_directive.rb b/lib/rubocop/cop/lint/redundant_cop_disable_directive.rb
@@ -135,7 +135,7 @@ def each_already_disabled(line_ranges, disabled_ranges, comments)
           end
         end
 
-        # rubocop:todo Metrics/CyclomaticComplexity
+        # rubocop:todo Metrics/AbcSize, Metrics/CyclomaticComplexity
         def find_redundant(comment, offenses, cop, line_range, next_line_range)
           if all_disabled?(comment)
             # If there's a disable all comment followed by a comment
@@ -153,7 +153,7 @@ def find_redundant(comment, offenses, cop, line_range, next_line_range)
             cop if cop_offenses.none? { |o| line_range.cover?(o.line) }
           end
         end
-        # rubocop:enable Metrics/CyclomaticComplexity
+        # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity
 
         def all_disabled?(comment)
           /rubocop\s*:\s*(?:disable|todo)\s+all\b/.match?(comment.text)

diff --git a/lib/rubocop/cop/metrics/utils/abc_size_calculator.rb b/lib/rubocop/cop/metrics/utils/abc_size_calculator.rb
@@ -11,32 +11,39 @@ module Utils
         # We separate the *calculator* from the *cop* so that the calculation,
         # the formula itself, is easier to test.
         class AbcSizeCalculator
+          include IteratingBlock
+          include RepeatedCsendDiscount
+
           # > Branch -- an explicit forward program branch out of scope -- a
           # > function call, class method call ..
           # > http://c2.com/cgi/wiki?AbcMetric
-          BRANCH_NODES = %i[send csend].freeze
+          BRANCH_NODES = %i[send csend yield].freeze
 
           # > Condition -- a logical/Boolean test, == != <= >= < > else case
           # > default try catch ? and unary conditionals.
           # > http://c2.com/cgi/wiki?AbcMetric
-          CONDITION_NODES = (CyclomaticComplexity::COUNTED_NODES - %i[block block_pass]).freeze
+          CONDITION_NODES = CyclomaticComplexity::COUNTED_NODES.freeze
 
           def self.calculate(node)
             new(node).calculate
           end
 
+          # TODO: move to rubocop-ast
+          ARGUMENT_TYPES = %i[arg optarg restarg kwarg kwoptarg kwrestarg blockarg].freeze
+
           def initialize(node)
             @assignment = 0
             @branch = 0
             @condition = 0
             @node = node
+            reset_repeated_csend
           end
 
           def calculate
             @node.each_node do |child|
-              if child.assignment?
-                @assignment += 1
-              elsif branch?(child)
+              @assignment += 1 if assignment?(child)
+
+              if branch?(child)
                 evaluate_branch_nodes(child)
               elsif condition?(child)
                 evaluate_condition_node(child)
@@ -54,6 +61,7 @@ def evaluate_branch_nodes(node)
               @condition += 1
             else
               @branch += 1
+              @condition += 1 if node.csend_type? && !discount_for_repeated_csend?(node)
             end
           end
 
@@ -70,11 +78,46 @@ def else_branch?(node)
 
           private
 
+          def assignment?(node)
+            node.for_type? ||
+              node.op_asgn_type? ||
+              (node.respond_to?(:setter_method?) && node.setter_method?) ||
+              (simple_assignment?(node) && capturing_variable?(node.children.first))
+          end
+
+          def simple_assignment?(node)
+            return false if node.masgn_type?
+
+            if node.equals_asgn?
+              reset_on_lvasgn(node) if node.lvasgn_type?
+              return true
+            end
+
+            argument?(node)
+          end
+
+          def capturing_variable?(name)
+            name && !/^_/.match?(name)
+          end
+
+          # Returns true for nodes which otherwise would be counted
+          # as one too many assignment
+          def assignment_doubled_in_ast?(node)
+            node.masgn_type? || node.or_asgn_type? || node.and_asgn_type?
+          end
+
           def branch?(node)
             BRANCH_NODES.include?(node.type)
           end
 
+          # TODO: move to rubocop-ast
+          def argument?(node)
+            ARGUMENT_TYPES.include?(node.type)
+          end
+
           def condition?(node)
+            return false if iterating_block?(node) == false
+
             CONDITION_NODES.include?(node.type)
           end
         end

diff --git a/lib/rubocop/cop/metrics/utils/repeated_csend_discount.rb b/lib/rubocop/cop/metrics/utils/repeated_csend_discount.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Metrics
+      module Utils
+        # @api private
+        #
+        # Helps to calculate code length for the provided node.
+        module RepeatedCsendDiscount
+          def reset_repeated_csend
+            @repeated_csend = {}
+          end
+
+          def discount_for_repeated_csend?(csend_node)
+            receiver = csend_node.receiver
+
+            return false unless receiver.lvar_type?
+
+            var_name = receiver.children.first
+            seen = @repeated_csend.fetch(var_name) do
+              @repeated_csend[var_name] = csend_node
+              return false
+            end
+
+            !seen.equal?(csend_node)
+          end
+
+          def reset_on_lvasgn(node)
+            var_name = node.children.first
+            @repeated_csend.delete(var_name)
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/rubocop/cop/mixin/array_min_size.rb b/lib/rubocop/cop/mixin/array_min_size.rb
@@ -16,7 +16,7 @@ def min_size_config
         cop_config['MinSize']
       end
 
-      def array_style_detected(style, ary_size)
+      def array_style_detected(style, ary_size) # rubocop:todo Metrics/AbcSize
         cfg = config_to_allow_offenses
         return if cfg['Enabled'] == false
 

diff --git a/lib/rubocop/cop/mixin/method_complexity.rb b/lib/rubocop/cop/mixin/method_complexity.rb
@@ -2,10 +2,13 @@
 
 module RuboCop
   module Cop
+    # @api private
+    #
     # This module handles measurement and reporting of complexity in methods.
     module MethodComplexity
       include ConfigurableMax
       include IgnoredMethods
+      include Metrics::Utils::RepeatedCsendDiscount
       extend NodePattern::Macros
 
       def on_def(node)
@@ -37,6 +40,7 @@ def check_complexity(node, method_name)
         return unless node.body
 
         max = cop_config['Max']
+        reset_repeated_csend
         complexity, abc_vector = complexity(node.body)
 
         return unless complexity > max
@@ -53,8 +57,9 @@ def check_complexity(node, method_name)
       end
 
       def complexity(body)
-        body.each_node(*self.class::COUNTED_NODES).reduce(1) do |score, n|
-          score + complexity_score_for(n)
+        body.each_node(*self.class::COUNTED_NODES).reduce(1) do |score, node|
+          reset_on_lvasgn(node) if node.lvasgn_type?
+          score + complexity_score_for(node)
         end
       end
     end

diff --git a/lib/rubocop/cop/style/case_like_if.rb b/lib/rubocop/cop/style/case_like_if.rb
@@ -151,7 +151,6 @@ def collect_conditions(node, target, conditions)
           conditions << condition if condition
         end
 
-        # rubocop:disable Metrics/AbcSize
         # rubocop:disable Metrics/CyclomaticComplexity
         def condition_from_send_node(node, target)
           case node.method_name
@@ -169,7 +168,6 @@ def condition_from_send_node(node, target)
           end
         end
         # rubocop:enable Metrics/CyclomaticComplexity
-        # rubocop:enable Metrics/AbcSize
 
         def condition_from_binary_op(lhs, rhs, target)
           lhs = deparenthesize(lhs)

diff --git a/lib/rubocop/cop/style/each_with_object.rb b/lib/rubocop/cop/style/each_with_object.rb
@@ -41,7 +41,6 @@ def on_block(node)
           end
         end
 
-        # rubocop:disable Metrics/AbcSize
         def autocorrect(node)
           lambda do |corrector|
             corrector.replace(node.send_node.loc.selector, 'each_with_object')
@@ -60,7 +59,6 @@ def autocorrect(node)
             end
           end
         end
-        # rubocop:enable Metrics/AbcSize
 
         private
 

diff --git a/lib/rubocop/cop/variable_force.rb b/lib/rubocop/cop/variable_force.rb
@@ -197,7 +197,6 @@ def regexp_captured_names(node)
         regexp.named_captures.keys
       end
 
-      # rubocop:disable Metrics/AbcSize
       def process_variable_operator_assignment(node)
         if LOGICAL_OPERATOR_ASSIGNMENT_TYPES.include?(node.type)
           asgn_node, rhs_node = *node
@@ -232,7 +231,6 @@ def process_variable_operator_assignment(node)
 
         skip_children!
       end
-      # rubocop:enable Metrics/AbcSize
 
       def process_variable_multiple_assignment(node)
         lhs_node, rhs_node = *node

diff --git a/lib/rubocop/cops_documentation_generator.rb b/lib/rubocop/cops_documentation_generator.rb
@@ -33,7 +33,6 @@ def cops_of_department(department)
     cops.with_department(department).sort!
   end
 
-  # rubocop:disable Metrics/AbcSize
   def cops_body(cop, description, examples_objects, pars)
     content = h2(cop.cop_name)
     content << required_ruby_version(cop)
@@ -44,7 +43,6 @@ def cops_body(cop, description, examples_objects, pars)
     content << references(cop)
     content
   end
-  # rubocop:enable Metrics/AbcSize
 
   def examples(examples_object)
     examples_object.each_with_object(h3('Examples').dup) do |example, content|

diff --git a/spec/rubocop/cop/metrics/abc_size_spec.rb b/spec/rubocop/cop/metrics/abc_size_spec.rb
@@ -39,7 +39,7 @@ def method_name
     it 'registers an offense for an assignment of an element' do
       expect_offense(<<~RUBY)
         def method_name
-        ^^^^^^^^^^^^^^^ Assignment Branch Condition size for method_name is too high. [<1, 1, 0> 1.41/0]
+        ^^^^^^^^^^^^^^^ Assignment Branch Condition size for method_name is too high. [<1, 2, 0> 2.24/0]
           x[0] = 1
         end
       RUBY
@@ -49,9 +49,9 @@ def method_name
        'scores' do
       expect_offense(<<~RUBY)
         def method_name
-        ^^^^^^^^^^^^^^^ Assignment Branch Condition size for method_name is too high. [<1, 4, 4> 5.74/0]
+        ^^^^^^^^^^^^^^^ Assignment Branch Condition size for method_name is too high. [<3, 4, 5> 7.07/0]
           my_options = Hash.new if 1 == 1 || 2 == 2 # 1, 1, 4
-          my_options.each do |key, value|           # 0, 1, 0
+          my_options.each do |key, value|           # 2, 1, 1
             p key                                   # 0, 1, 0
             p value                                 # 0, 1, 0
           end
@@ -68,10 +68,10 @@ def method_name
       RUBY
     end
 
-    it 'treats safe navigation method calls like regular method calls' do
-      expect_offense(<<~RUBY) # sqrt(0 + 2*2 + 0) => 2
+    it 'treats safe navigation method calls like regular method calls + a condition' do
+      expect_offense(<<~RUBY)
         def method_name
-        ^^^^^^^^^^^^^^^ Assignment Branch Condition size for method_name is too high. [<0, 2, 0> 2/0]
+        ^^^^^^^^^^^^^^^ Assignment Branch Condition size for method_name is too high. [<0, 2, 1> 2.24/0]
           object&.do_something
         end
       RUBY