diff --git a/CHANGELOG.md b/CHANGELOG.md index 8153795cece..97fb3e8a5fc 100644 --- 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 index 87aabea051e..83abf88b87f 100644 --- 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 index 321c9495d98..35cb579a4db 100644 --- 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 index 0ae870d0f71..f3ca96c7ec1 100644 --- 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 index b05413a134d..309efb6ed24 100644 --- 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 index b04bd49f962..b548388078a 100644 --- 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 index ed45529d411..c87046ffef7 100644 --- 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 index 4932d7d5433..e02359db9d4 100644 --- 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 index 872da67c3c5..0e122a38e48 100644 --- 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 index e28e1249416..75eb867ebc0 100644 --- 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 new file mode 100644 index 00000000000..d9c26020284 --- /dev/null +++ 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 index 1d1e26704f9..c34c61beb43 100644 --- 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 index 580b7d7e1a0..b4feca3bf97 100644 --- 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 index 6e35350a497..8b18a2d27d8 100644 --- 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 index 25a85770838..fefbaa8486e 100644 --- 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 index 2f3a69421b9..44a6f6982a6 100644 --- 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 index b5ab4ab13a6..d7629d139e5 100644 --- 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 index 51503afe399..67823753a16 100644 --- 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 diff --git a/spec/rubocop/cop/metrics/utils/abc_size_calculator_spec.rb b/spec/rubocop/cop/metrics/utils/abc_size_calculator_spec.rb index e54754c8802..788eb2e50ba 100644 --- a/spec/rubocop/cop/metrics/utils/abc_size_calculator_spec.rb +++ b/spec/rubocop/cop/metrics/utils/abc_size_calculator_spec.rb @@ -16,6 +16,28 @@ def method_name it { is_expected.to eq '<0, 3, 0>' } end + context 'with ||=' do + let(:source) { <<~RUBY } + def method_name + x = nil + x ||= 1 + end + RUBY + + it { is_expected.to eq '<2, 0, 1>' } + end + + context 'with &&=' do + let(:source) { <<~RUBY } + def method_name + x = nil + x &&= 1 + end + RUBY + + it { is_expected.to eq '<2, 0, 1>' } + end + context 'assignment with ternary operator' do let(:source) { <<~RUBY } def method_name @@ -27,6 +49,99 @@ def method_name it { is_expected.to eq '<2, 6, 2>' } end + context 'with a block' do + let(:source) { <<~RUBY } + def method_name + x = foo # <1, 1, 0> + bar do # <1, 2, 0> (+1 for bar, 0 for non-empty block) + y = baz # <2, 3, 0> + end + end + RUBY + + it { is_expected.to eq '<2, 3, 0>' } + end + + context 'same but with 7 arguments' do + let(:source) { <<~RUBY } + def method_name + x = foo + bar do |a, (b, c), d = 42, *e, f: 42, **g| + y = baz + end + end + RUBY + + it { is_expected.to eq '<9, 3, 0>' } + end + + context 'with unused assignments' do + let(:source) { <<~RUBY } + def method_name + _, _ignored, foo, = [1, 2, 3] # <1, 0, 0> + bar do |_, (only_real_assignment, _unused), *, **| # <+1, 1, 0> + only_real_assignment + end + end + RUBY + + it { is_expected.to eq '<2, 1, 0>' } + end + + context 'with a known iterating block' do + let(:source) { <<~RUBY } + def method_name + x = foo # <1, 1, 0> + x.each do # <1, 2, 1> (+1 B for each, +1 C iterating block) + y = baz # <2, 3, 1> + end + x.map(&:to_s) # <2, 4, 2> (+1 B for map, +1 C iterating block) + end + RUBY + + it { is_expected.to eq '<2, 4, 2>' } + end + + context 'method with arguments' do + let(:source) { <<~RUBY } + def method_name(a = 0, *b, c: 42, **d) + end + RUBY + + it { is_expected.to eq '<4, 0, 0>' } + end + + context 'with .foo =' do + let(:source) { <<~RUBY } + def method_name + foo.bar = 42 + end + RUBY + + it { is_expected.to eq '<1, 2, 0>' } + end + + context 'with []=' do + let(:source) { <<~RUBY } + def method_name + x = {} + x[:hello] = 'world' + end + RUBY + + it { is_expected.to eq '<2, 1, 0>' } + end + + context 'multiple assignment' do + let(:source) { <<~RUBY } + def method_name + a, b, c = d + end + RUBY + + it { is_expected.to eq '<3, 1, 0>' } + end + context 'if and arithmetic operations' do let(:source) { <<~RUBY } def method_name @@ -67,6 +182,33 @@ def method_name it { is_expected.to eq '<2, 9, 5>' } end + context 'with &.foo' do + let(:source) { <<~RUBY } + def method_name + method&.foo + method&.foo + end + RUBY + + it { is_expected.to eq '<0, 4, 2>' } + + context 'with repeated lvar receivers' do + let(:source) { <<~RUBY } + def foo + var = other = 1 # 2, 0, 0 + var&.do_something # +1, +1 + var&.dont_count_this_as_condition # +1, +0 + var = 2 # +1 + var&.start_counting_again # +1, +1 + var&.dont_count_this_as_condition_either # +1, +0 + other&.do_something # +1, +1 + end + RUBY + + it { is_expected.to eq '<3, 5, 3>' } + end + end + context 'elsif vs else if' do context 'elsif' do let(:source) { <<~RUBY } @@ -102,5 +244,27 @@ def method_name it { is_expected.to eq '<0, 5, 4>' } end end + + context 'with a for' do + let(:source) { <<~RUBY } + def method_name + for x in 0..5 # 2, 0, 1 + puts x # +1 + end + end + RUBY + + it { is_expected.to eq '<2, 1, 1>' } + end + + context 'with a yield' do + let(:source) { <<~RUBY } + def method_name + yield 42 + end + RUBY + + it { is_expected.to eq '<0, 1, 0>' } + end end end