From 38df07a4fedea07f2415c389bb36343d782b42b7 Mon Sep 17 00:00:00 2001 From: Marc-Andre Lafortune Date: Fri, 12 Jun 2020 16:10:23 -0400 Subject: [PATCH 1/3] These defaults may change --- spec/rubocop/cli/cli_disable_uncorrectable_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/rubocop/cli/cli_disable_uncorrectable_spec.rb b/spec/rubocop/cli/cli_disable_uncorrectable_spec.rb index 53bfae6d945..9146d83d23e 100644 --- a/spec/rubocop/cli/cli_disable_uncorrectable_spec.rb +++ b/spec/rubocop/cli/cli_disable_uncorrectable_spec.rb @@ -152,6 +152,12 @@ def choose_move(who_to_move) end end RUBY + create_file('.rubocop.yml', <<~YAML) + Metrics/AbcSize: + Max: 15 + Metrics/CyclomaticComplexity: + Max: 6 + YAML expect(exit_code).to eq(0) expect($stderr.string).to eq('') expect($stdout.string).to eq(<<~OUTPUT) From 2b7d41a5099f216daeb19e9ae09c642b2cc9eb97 Mon Sep 17 00:00:00 2001 From: Marc-Andre Lafortune Date: Thu, 18 Jun 2020 13:24:41 -0400 Subject: [PATCH 2/3] Refactor EmptyCaseCondition#on_case --- lib/rubocop/cop/style/empty_case_condition.rb | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/lib/rubocop/cop/style/empty_case_condition.rb b/lib/rubocop/cop/style/empty_case_condition.rb index 2ca4ae4755c..d9d3f4c3eda 100644 --- a/lib/rubocop/cop/style/empty_case_condition.rb +++ b/lib/rubocop/cop/style/empty_case_condition.rb @@ -43,13 +43,15 @@ class EmptyCaseCondition < Cop def on_case(case_node) return if case_node.condition - return if case_node.when_branches.any? do |when_branch| - when_branch.each_descendant.any?(&:return_type?) - end - if (else_branch = case_node.else_branch) - return if else_branch.return_type? || - else_branch.each_descendant.any?(&:return_type?) + branch_bodies = [ + *case_node.when_branches.map(&:body), + case_node.else_branch + ].compact + + return if branch_bodies.any? do |body| + body.return_type? || + body.each_descendant.any?(&:return_type?) end add_offense(case_node, location: :keyword) From e3e7f6ff1fc5a6be237f9727d4bdecdf59285ec7 Mon Sep 17 00:00:00 2001 From: Marc-Andre Lafortune Date: Tue, 26 May 2020 05:40:18 -0400 Subject: [PATCH 3/3] Fix Metrics/CyclomaticComplexity for ||=, &&=, &. and iterating blocks --- CHANGELOG.md | 4 ++ config/default.yml | 2 +- docs/modules/ROOT/pages/cops_metrics.adoc | 18 ++++- lib/rubocop.rb | 1 + lib/rubocop/cop/layout/hash_alignment.rb | 2 +- .../lint/redundant_cop_disable_directive.rb | 2 + .../cop/metrics/cyclomatic_complexity.rb | 38 ++++++++++- .../cop/metrics/utils/abc_size_calculator.rb | 2 +- .../cop/metrics/utils/iterating_block.rb | 61 +++++++++++++++++ .../mixin/multiline_expression_indentation.rb | 2 +- lib/rubocop/cop/naming/file_name.rb | 2 - lib/rubocop/cop/style/block_delimiters.rb | 2 - .../cop/style/conditional_assignment.rb | 2 +- .../style/identical_conditional_branches.rb | 2 +- lib/rubocop/cop/style/if_inside_else.rb | 2 +- lib/rubocop/cop/style/sample.rb | 2 +- lib/rubocop/cop/style/semicolon.rb | 2 +- .../cop/metrics/cyclomatic_complexity_spec.rb | 65 +++++++++++++++++++ 18 files changed, 194 insertions(+), 17 deletions(-) create mode 100644 lib/rubocop/cop/metrics/utils/iterating_block.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index df291092cac..a43ea874338 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,10 @@ * [#8146](https://github.com/rubocop-hq/rubocop/pull/8146): Use UTC in RuboCop todo file generation. ([@mauro-oto][]) +### Changes + +* [#8149](https://github.com/rubocop-hq/rubocop/pull/8149): Cop "Metrics/CyclotomicComplexicy" now counts `&.`, `||=`, `&&=` and blocks known to iterate. Default bumped from 6 to 7. ([@marcandre][]) + ## 0.85.1 (2020-06-07) ### Bug fixes diff --git a/config/default.yml b/config/default.yml index eca00467bb1..62525f1e022 100644 --- a/config/default.yml +++ b/config/default.yml @@ -1925,7 +1925,7 @@ Metrics/CyclomaticComplexity: VersionAdded: '0.25' VersionChanged: '0.81' IgnoredMethods: [] - Max: 6 + Max: 7 Metrics/MethodLength: Description: 'Avoid methods longer than 10 lines of code.' diff --git a/docs/modules/ROOT/pages/cops_metrics.adoc b/docs/modules/ROOT/pages/cops_metrics.adoc index a0508d38d00..f2bf669f980 100644 --- a/docs/modules/ROOT/pages/cops_metrics.adoc +++ b/docs/modules/ROOT/pages/cops_metrics.adoc @@ -166,6 +166,22 @@ else branch does not, since it doesn't add a decision point. The && operator (or keyword and) can be converted to a nested if statement, and ||/or is shorthand for a sequence of ifs, so they also add one. Loops can be said to have an exit condition, so they add one. +Blocks that are calls to builtin iteration methods +(e.g. `ary.map{...}) also add one, otherse are ignored. + + def each_child_node(*types) # count begins: 1 + unless block_given? # unless: +1 + return to_enum(__method__, *types) + + children.each do |child| # each{}: +1 + next unless child.is_a?(Node) # unless: +1 + + yield child if types.empty? || # if: +1, ||: +1 + types.include?(child.type) + end + + self + end # total: 6 === Configurable attributes @@ -177,7 +193,7 @@ Loops can be said to have an exit condition, so they add one. | Array | Max -| `6` +| `7` | Integer |=== diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 82ec73b7dd6..1c1ba69e389 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -321,6 +321,7 @@ require_relative 'rubocop/cop/lint/useless_setter_call' require_relative 'rubocop/cop/lint/void' +require_relative 'rubocop/cop/metrics/utils/iterating_block' require_relative 'rubocop/cop/metrics/cyclomatic_complexity' # relies on cyclomatic_complexity require_relative 'rubocop/cop/metrics/utils/abc_size_calculator' diff --git a/lib/rubocop/cop/layout/hash_alignment.rb b/lib/rubocop/cop/layout/hash_alignment.rb index c675a54e516..c565117f7bd 100644 --- a/lib/rubocop/cop/layout/hash_alignment.rb +++ b/lib/rubocop/cop/layout/hash_alignment.rb @@ -200,7 +200,7 @@ def on_send(node) alias on_super on_send alias on_yield on_send - def on_hash(node) + def on_hash(node) # rubocop:todo Metrics/CyclomaticComplexity return if ignored_node?(node) return if node.pairs.empty? || node.single_line? diff --git a/lib/rubocop/cop/lint/redundant_cop_disable_directive.rb b/lib/rubocop/cop/lint/redundant_cop_disable_directive.rb index 5fcebab55c9..70f60a60494 100644 --- a/lib/rubocop/cop/lint/redundant_cop_disable_directive.rb +++ b/lib/rubocop/cop/lint/redundant_cop_disable_directive.rb @@ -135,6 +135,7 @@ def each_already_disabled(line_ranges, disabled_ranges, comments) end end + # rubocop:todo 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 @@ -152,6 +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 def all_disabled?(comment) /rubocop\s*:\s*(?:disable|todo)\s+all\b/.match?(comment.text) diff --git a/lib/rubocop/cop/metrics/cyclomatic_complexity.rb b/lib/rubocop/cop/metrics/cyclomatic_complexity.rb index 3c53a93d3af..b406d06fa08 100644 --- a/lib/rubocop/cop/metrics/cyclomatic_complexity.rb +++ b/lib/rubocop/cop/metrics/cyclomatic_complexity.rb @@ -13,19 +13,51 @@ module Metrics # operator (or keyword and) can be converted to a nested if statement, # and ||/or is shorthand for a sequence of ifs, so they also add one. # Loops can be said to have an exit condition, so they add one. + # Blocks that are calls to builtin iteration methods + # (e.g. `ary.map{...}) also add one, otherse are ignored. + # + # def each_child_node(*types) # count begins: 1 + # unless block_given? # unless: +1 + # return to_enum(__method__, *types) + # + # children.each do |child| # each{}: +1 + # next unless child.is_a?(Node) # unless: +1 + # + # yield child if types.empty? || # if: +1, ||: +1 + # types.include?(child.type) + # end + # + # self + # end # total: 6 class CyclomaticComplexity < Cop include MethodComplexity + include Utils::IteratingBlock MSG = 'Cyclomatic complexity for %s is too high. ' \ '[%d/%d]' - COUNTED_NODES = %i[if while until for - rescue when and or].freeze + COUNTED_NODES = %i[if while until for csend block block_pass + rescue when and or or_asgn and_asgn].freeze private - def complexity_score_for(_node) + def complexity_score_for(node) + return 0 if iterating_block?(node) == false + 1 end + + def block_method(node) + case node.type + when :block + node.method_name + when :block_pass + node.parent.method_name + end + end + + def count_block?(block) + KNOWN_ITERATING_METHODS.include? block.method_name + end end end end diff --git a/lib/rubocop/cop/metrics/utils/abc_size_calculator.rb b/lib/rubocop/cop/metrics/utils/abc_size_calculator.rb index 92e8c47eba2..e28e1249416 100644 --- a/lib/rubocop/cop/metrics/utils/abc_size_calculator.rb +++ b/lib/rubocop/cop/metrics/utils/abc_size_calculator.rb @@ -19,7 +19,7 @@ class AbcSizeCalculator # > Condition -- a logical/Boolean test, == != <= >= < > else case # > default try catch ? and unary conditionals. # > http://c2.com/cgi/wiki?AbcMetric - CONDITION_NODES = CyclomaticComplexity::COUNTED_NODES.freeze + CONDITION_NODES = (CyclomaticComplexity::COUNTED_NODES - %i[block block_pass]).freeze def self.calculate(node) new(node).calculate diff --git a/lib/rubocop/cop/metrics/utils/iterating_block.rb b/lib/rubocop/cop/metrics/utils/iterating_block.rb new file mode 100644 index 00000000000..ebf40ba98c8 --- /dev/null +++ b/lib/rubocop/cop/metrics/utils/iterating_block.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Metrics + module Utils + # Used to identify iterating blocks like `.map{}` and `.map(&:...)` + module IteratingBlock + enumerable = %i[ + all? any? chain chunk chunk_while collect collect_concat count cycle + detect drop drop_while each each_cons each_entry each_slice + each_with_index each_with_object entries filter filter_map find + find_all find_index flat_map grep grep_v group_by inject lazy map + max max_by min min_by minmax minmax_by none? one? partition reduce + reject reverse_each select slice_after slice_before slice_when sort + sort_by sum take take_while tally to_h uniq zip + ] + + enumerator = %i[with_index with_object] + + array = %i[ + bsearch bsearch_index collect! combination d_permutation delete_if + each_index keep_if map! permutation product reject! repeat + repeated_combination select! sort sort! sort_by sort_by + ] + + hash = %i[ + each_key each_pair each_value fetch fetch_values has_key? merge + merge! transform_keys transform_keys! transform_values + transform_values! + ] + + KNOWN_ITERATING_METHODS = (Set.new(enumerable) + enumerator + array + hash).freeze + + # Returns the name of the method called with a block + # if node is a block node, or a block-pass node. + def block_method_name(node) + case node.type + when :block + node.method_name + when :block_pass + node.parent.method_name + end + end + + # Returns true iff name is a known iterating type (e.g. :each, :transform_values) + def iterating_method?(name) + KNOWN_ITERATING_METHODS.include? name + end + + # Returns nil if node is neither a block node or a block-pass node. + # Otherwise returns true/false if method call is a known iterating call + def iterating_block?(node) + name = block_method_name(node) + name && iterating_method?(name) + end + end + end + end + end +end diff --git a/lib/rubocop/cop/mixin/multiline_expression_indentation.rb b/lib/rubocop/cop/mixin/multiline_expression_indentation.rb index f774aa427cb..837c0baa143 100644 --- a/lib/rubocop/cop/mixin/multiline_expression_indentation.rb +++ b/lib/rubocop/cop/mixin/multiline_expression_indentation.rb @@ -152,7 +152,7 @@ def indented_keyword_expression(node) expression end - def argument_in_method_call(node, kind) + def argument_in_method_call(node, kind) # rubocop:todo Metrics/CyclomaticComplexity node.each_ancestor(:send, :block).find do |a| # If the node is inside a block, it makes no difference if that block # is an argument in a method call. It doesn't count. diff --git a/lib/rubocop/cop/naming/file_name.rb b/lib/rubocop/cop/naming/file_name.rb index 273f70682ce..fb72ef738cb 100644 --- a/lib/rubocop/cop/naming/file_name.rb +++ b/lib/rubocop/cop/naming/file_name.rb @@ -126,7 +126,6 @@ def filename_good?(basename) basename.match?(regex || SNAKE_CASE) end - # rubocop:disable Metrics/CyclomaticComplexity def find_class_or_module(node, namespace) return nil unless node @@ -145,7 +144,6 @@ def find_class_or_module(node, namespace) nil end - # rubocop:enable Metrics/CyclomaticComplexity def match_namespace(node, namespace, expected) match_partial = partial_matcher!(expected) diff --git a/lib/rubocop/cop/style/block_delimiters.rb b/lib/rubocop/cop/style/block_delimiters.rb index e7b701e3569..69c42640de6 100644 --- a/lib/rubocop/cop/style/block_delimiters.rb +++ b/lib/rubocop/cop/style/block_delimiters.rb @@ -252,7 +252,6 @@ def whitespace_after?(range, length = 1) /\s/.match?(range.source_buffer.source[range.begin_pos + length, 1]) end - # rubocop:disable Metrics/CyclomaticComplexity def get_blocks(node, &block) case node.type when :block @@ -270,7 +269,6 @@ def get_blocks(node, &block) node.each_child_node { |child| get_blocks(child, &block) } end end - # rubocop:enable Metrics/CyclomaticComplexity def proper_block_style?(node) return special_method_proper_block_style?(node) if special_method?(node.method_name) diff --git a/lib/rubocop/cop/style/conditional_assignment.rb b/lib/rubocop/cop/style/conditional_assignment.rb index 5ba1171c9a8..bf88d1030f6 100644 --- a/lib/rubocop/cop/style/conditional_assignment.rb +++ b/lib/rubocop/cop/style/conditional_assignment.rb @@ -67,7 +67,7 @@ def end_with_eq?(sym) private - def expand_elsif(node, elsif_branches = []) + def expand_elsif(node, elsif_branches = []) # rubocop:todo Metrics/CyclomaticComplexity return [] if node.nil? || !node.if_type? || !node.elsif? elsif_branches << node.if_branch diff --git a/lib/rubocop/cop/style/identical_conditional_branches.rb b/lib/rubocop/cop/style/identical_conditional_branches.rb index 3b45e6791e1..5edee294f57 100644 --- a/lib/rubocop/cop/style/identical_conditional_branches.rb +++ b/lib/rubocop/cop/style/identical_conditional_branches.rb @@ -81,7 +81,7 @@ def on_case(node) private - def check_branches(branches) + def check_branches(branches) # rubocop:todo Metrics/CyclomaticComplexity # return if any branch is empty. An empty branch can be an `if` # without an `else` or a branch that contains only comments. return if branches.any?(&:nil?) diff --git a/lib/rubocop/cop/style/if_inside_else.rb b/lib/rubocop/cop/style/if_inside_else.rb index 1750e860946..419e78a9bdc 100644 --- a/lib/rubocop/cop/style/if_inside_else.rb +++ b/lib/rubocop/cop/style/if_inside_else.rb @@ -61,7 +61,7 @@ module Style class IfInsideElse < Cop MSG = 'Convert `if` nested inside `else` to `elsif`.' - def on_if(node) + def on_if(node) # rubocop:todo Metrics/CyclomaticComplexity return if node.ternary? || node.unless? else_branch = node.else_branch diff --git a/lib/rubocop/cop/style/sample.rb b/lib/rubocop/cop/style/sample.rb index 6709c3fd351..25af4ccc831 100644 --- a/lib/rubocop/cop/style/sample.rb +++ b/lib/rubocop/cop/style/sample.rb @@ -92,7 +92,7 @@ def sample_size_for_two_args(first, second) second.int_type? ? second.to_a.first : :unknown end - def range_size(range_node) + def range_size(range_node) # rubocop:todo Metrics/CyclomaticComplexity vals = range_node.to_a return :unknown unless vals.all?(&:int_type?) diff --git a/lib/rubocop/cop/style/semicolon.rb b/lib/rubocop/cop/style/semicolon.rb index bcfadf61652..1b6b43552cd 100644 --- a/lib/rubocop/cop/style/semicolon.rb +++ b/lib/rubocop/cop/style/semicolon.rb @@ -39,7 +39,7 @@ def investigate(processed_source) check_for_line_terminator_or_opener end - def on_begin(node) + def on_begin(node) # rubocop:todo Metrics/CyclomaticComplexity return if cop_config['AllowAsExpressionSeparator'] exprs = node.children diff --git a/spec/rubocop/cop/metrics/cyclomatic_complexity_spec.rb b/spec/rubocop/cop/metrics/cyclomatic_complexity_spec.rb index 127767d87c4..78a2faae411 100644 --- a/spec/rubocop/cop/metrics/cyclomatic_complexity_spec.rb +++ b/spec/rubocop/cop/metrics/cyclomatic_complexity_spec.rb @@ -150,6 +150,16 @@ def method_name RUBY end + it 'registers an offense for &&=' do + expect_offense(<<~RUBY) + def method_name + ^^^^^^^^^^^^^^^ Cyclomatic complexity for method_name is too high. [2/1] + foo = nil + foo &&= 42 + end + RUBY + end + it 'registers an offense for and' do expect_offense(<<~RUBY) def method_name @@ -168,6 +178,16 @@ def method_name RUBY end + it 'registers an offense for ||=' do + expect_offense(<<~RUBY) + def method_name + ^^^^^^^^^^^^^^^ Cyclomatic complexity for method_name is too high. [2/1] + foo = nil + foo ||= 42 + end + RUBY + end + it 'registers an offense for or' do expect_offense(<<~RUBY) def method_name @@ -189,6 +209,16 @@ def method_name RUBY end + it 'registers an offense for &.' do + expect_offense(<<~RUBY) + def method_name + ^^^^^^^^^^^^^^^ Cyclomatic complexity for method_name is too high. [2/1] + foo = nil + foo&.bar + end + RUBY + end + it 'counts only a single method' do expect_offense(<<~RUBY) def method_name_1 @@ -211,6 +241,41 @@ def method_name_2 end RUBY end + + it 'counts enumerating methods with blocks as +1' do + expect_offense(<<~RUBY) + define_method :method_name do + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Cyclomatic complexity for method_name is too high. [3/1] + (1..4).map do |i| # map: +1 + i * 2 + end.each.with_index { |val, i| puts val, i } # each: +0, with_index: +1 + return treasure.map + end + RUBY + end + + it 'counts enumerating methods with block-pass as +1' do + expect_offense(<<~RUBY) + define_method :method_name do + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Cyclomatic complexity for method_name is too high. [2/1] + [].map(&:to_s) + end + RUBY + end + + it 'does not count blocks in general' do + expect_no_offenses(<<~RUBY) + define_method :method_name do + Struct.new(:foo, :bar) do + String.class_eval do + [42].tap do |answer| + foo { bar } + end + end + end + end + RUBY + end end context 'when method is in list of ignored methods' do