Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Metrics/AbcSize needs love #8037

Merged
merged 1 commit into from Jul 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion config/default.yml
Expand Up @@ -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.'
Expand Down
2 changes: 1 addition & 1 deletion docs/modules/ROOT/pages/cops_metrics.adoc
Expand Up @@ -27,7 +27,7 @@ and https://en.wikipedia.org/wiki/ABC_Software_Metric.
| Array

| Max
| `15`
| `17`
| Integer
|===

Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Expand Up @@ -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'
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/comment_config.rb
Expand Up @@ -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|
Expand Down
2 changes: 1 addition & 1 deletion lib/rubocop/config_loader.rb
Expand Up @@ -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)
Expand Down
4 changes: 1 addition & 3 deletions lib/rubocop/config_validator.rb
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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') }

Expand Down
2 changes: 0 additions & 2 deletions lib/rubocop/cop/layout/rescue_ensure_alignment.rb
Expand Up @@ -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
Expand All @@ -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.

Expand Down
4 changes: 2 additions & 2 deletions lib/rubocop/cop/lint/redundant_cop_disable_directive.rb
Expand Up @@ -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
Expand All @@ -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)
Expand Down
53 changes: 48 additions & 5 deletions lib/rubocop/cop/metrics/utils/abc_size_calculator.rb
Expand Up @@ -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)
Expand All @@ -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

Expand All @@ -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?
marcandre marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
37 changes: 37 additions & 0 deletions 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
2 changes: 1 addition & 1 deletion lib/rubocop/cop/mixin/array_min_size.rb
Expand Up @@ -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

Expand Down
9 changes: 7 additions & 2 deletions lib/rubocop/cop/mixin/method_complexity.rb
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
2 changes: 0 additions & 2 deletions lib/rubocop/cop/style/case_like_if.rb
Expand Up @@ -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
Expand All @@ -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)
Expand Down
2 changes: 0 additions & 2 deletions lib/rubocop/cop/style/each_with_object.rb
Expand Up @@ -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')
Expand All @@ -60,7 +59,6 @@ def autocorrect(node)
end
end
end
# rubocop:enable Metrics/AbcSize

private

Expand Down
2 changes: 0 additions & 2 deletions lib/rubocop/cop/variable_force.rb
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 0 additions & 2 deletions lib/rubocop/cops_documentation_generator.rb
Expand Up @@ -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)
Expand All @@ -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|
Expand Down
12 changes: 6 additions & 6 deletions spec/rubocop/cop/metrics/abc_size_spec.rb
Expand Up @@ -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]
marcandre marked this conversation as resolved.
Show resolved Hide resolved
x[0] = 1
end
RUBY
Expand All @@ -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
Expand All @@ -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
Expand Down