From 345be1d50c6f44b8ae1a880f490d5faa1f368945 Mon Sep 17 00:00:00 2001 From: Marc-Andre Lafortune Date: Wed, 11 Nov 2020 16:44:42 -0500 Subject: [PATCH] [Fixes #8564] `Metrics/AbcSize`: Add optional discount for repeated "attributes" --- ...etricsabcsize_add_optional_discount_for.md | 1 + config/default.yml | 3 +- lib/rubocop.rb | 1 + lib/rubocop/cop/metrics/abc_size.rb | 26 +++- .../cop/metrics/utils/abc_size_calculator.rb | 5 +- .../utils/repeated_attribute_discount.rb | 146 ++++++++++++++++++ spec/rubocop/cop/metrics/abc_size_spec.rb | 30 ++++ .../metrics/utils/abc_size_calculator_spec.rb | 73 ++++++++- 8 files changed, 280 insertions(+), 5 deletions(-) create mode 100644 changelog/new_metricsabcsize_add_optional_discount_for.md create mode 100644 lib/rubocop/cop/metrics/utils/repeated_attribute_discount.rb diff --git a/changelog/new_metricsabcsize_add_optional_discount_for.md b/changelog/new_metricsabcsize_add_optional_discount_for.md new file mode 100644 index 000000000000..b96397cfc002 --- /dev/null +++ b/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][]) diff --git a/config/default.yml b/config/default.yml index be8f9571f9e5..e7e4e82178d3 100644 --- a/config/default.yml +++ b/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: '<>' # The ABC size is a calculated magnitude, so this number can be an Integer or # a Float. IgnoredMethods: [] + CountRepeatedAttributes: true Max: 17 Metrics/BlockLength: diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 5e5db495eb4d..639ca51e68ab 100644 --- a/lib/rubocop.rb +++ b/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' diff --git a/lib/rubocop/cop/metrics/abc_size.rb b/lib/rubocop/cop/metrics/abc_size.rb index d3d6901e81d8..0fe4eb384f06 100644 --- a/lib/rubocop/cop/metrics/abc_size.rb +++ b/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 diff --git a/lib/rubocop/cop/metrics/utils/abc_size_calculator.rb b/lib/rubocop/cop/metrics/utils/abc_size_calculator.rb index 9dc9d8bd5bab..7664e100b616 100644 --- a/lib/rubocop/cop/metrics/utils/abc_size_calculator.rb +++ b/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 diff --git a/lib/rubocop/cop/metrics/utils/repeated_attribute_discount.rb b/lib/rubocop/cop/metrics/utils/repeated_attribute_discount.rb new file mode 100644 index 000000000000..d53011b77b69 --- /dev/null +++ b/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 diff --git a/spec/rubocop/cop/metrics/abc_size_spec.rb b/spec/rubocop/cop/metrics/abc_size_spec.rb index b1b82cdd1b3f..ae897001f345 100644 --- a/spec/rubocop/cop/metrics/abc_size_spec.rb +++ b/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 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 b69453f7d136..4ecbe67381d5 100644 --- a/spec/rubocop/cop/metrics/utils/abc_size_calculator_spec.rb +++ b/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