diff --git a/CHANGELOG.md b/CHANGELOG.md index 588b834a949..7a210d618e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ * [#8578](https://github.com/rubocop-hq/rubocop/pull/8578): Add `:restore_registry` context and `stub_cop_class` helper class. ([@marcandre][]) * [#8579](https://github.com/rubocop-hq/rubocop/pull/8579): Add `Cop.documentation_url`. ([@marcandre][]) * [#8510](https://github.com/rubocop-hq/rubocop/pull/8510): Add `RegexpNode#each_capture` and `parsed_tree`. ([@marcandre][]) +* [#8365](https://github.com/rubocop-hq/rubocop/pull/8365): Cops defining `on_send` can be optimized by defining the constant `RESTRICT_ON_SEND` with a list of acceptable method names. ([@marcandre][]) ### Bug fixes diff --git a/docs/modules/ROOT/pages/development.adoc b/docs/modules/ROOT/pages/development.adoc index 790d7291c99..fc103207bc7 100644 --- a/docs/modules/ROOT/pages/development.adoc +++ b/docs/modules/ROOT/pages/development.adoc @@ -210,6 +210,9 @@ def on_send(node) end ---- +The `on_send` callback is the most used and can be optimized by restricting the acceptable +method names with a constant `RESTRICT_ON_SEND`. + And the final cop code will look like something like this: [source,ruby] @@ -232,6 +235,8 @@ module RuboCop (send (send $(...) :empty?) :!) PATTERN + RESTRICT_ON_SEND = [:!].freeze # optimization: don't call `on_send` unless + # the method name is in this list def on_send(node) return unless not_empty_call?(node) diff --git a/lib/rubocop/cop/base.rb b/lib/rubocop/cop/base.rb index c8525373377..888e328421f 100644 --- a/lib/rubocop/cop/base.rb +++ b/lib/rubocop/cop/base.rb @@ -46,6 +46,9 @@ class Base # rubocop:disable Metrics/ClassLength # Consider creation API private InvestigationReport = Struct.new(:cop, :processed_source, :offenses, :corrector) + # List of methods names to restrict calls for `on_send` / `on_csend` + RESTRICT_ON_SEND = Set[].freeze + # List of cops that should not try to autocorrect at the same # time as this cop # @@ -287,6 +290,10 @@ def currently_disabled_lines @currently_disabled_lines ||= Set.new end + private_class_method def self.restrict_on_send + @restrict_on_send ||= self::RESTRICT_ON_SEND.to_set.freeze + end + # Called before any investigation def begin_investigation(processed_source) @current_offenses = [] diff --git a/lib/rubocop/cop/commissioner.rb b/lib/rubocop/cop/commissioner.rb index e154292aa30..d6c6a3dfc65 100644 --- a/lib/rubocop/cop/commissioner.rb +++ b/lib/rubocop/cop/commissioner.rb @@ -7,6 +7,9 @@ module Cop class Commissioner include RuboCop::AST::Traversal + RESTRICTED_CALLBACKS = %i[on_send on_csend].freeze + private_constant :RESTRICTED_CALLBACKS + # How a Commissioner returns the results of the investigation # as a list of Cop::InvestigationReport and any errors caught # during the investigation. @@ -43,6 +46,7 @@ def initialize(cops, forces = [], options = {}) @forces = forces @options = options @callbacks = Hash.new { |h, k| h[k] = cops_callbacks_for(k) } + @restricted_map = {} reset end @@ -57,10 +61,17 @@ def initialize(cops, forces = [], options = {}) method_name = :"on_#{node_type}" next unless method_defined?(method_name) - define_method(method_name) do |node| - trigger_responding_cops(method_name, node) - super(node) unless NO_CHILD_NODES.include?(node_type) + if RESTRICTED_CALLBACKS.include?(method_name) + trigger_restricted = "trigger_restricted_cops(:on_#{node_type}, node)" end + + class_eval(<<~RUBY, __FILE__, __LINE__ + 1) + def on_#{node_type}(node) + trigger_responding_cops(:on_#{node_type}, node) + #{trigger_restricted} + #{'super(node)' unless NO_CHILD_NODES.include?(node_type)} + end + RUBY end # @return [InvestigationReport] @@ -95,9 +106,35 @@ def reset end def cops_callbacks_for(callback) - @cops.select do |cop| + callbacks = @cops.select do |cop| cop.respond_to?(callback) end + if RESTRICTED_CALLBACKS.include?(callback) + @restricted_map[callback] = restricted_map(callbacks) + end + callbacks + end + + def trigger_restricted_cops(event, node) + name = node.method_name + @restricted_map.fetch(event)[name]&.each do |cop| + with_cop_error_handling(cop, node) do + cop.send(event, node) + end + end + end + + # Note: mutates `callbacks` in place + def restricted_map(callbacks) + map = {} + callbacks.select! do |cop| + restrictions = cop.class.send :restrict_on_send + restrictions.each do |name| + (map[name] ||= []) << cop + end + restrictions.empty? + end + map end def invoke(callback, cops, *args) diff --git a/spec/rubocop/cop/commissioner_spec.rb b/spec/rubocop/cop/commissioner_spec.rb index 3434c457bc2..9c584ad2f96 100644 --- a/spec/rubocop/cop/commissioner_spec.rb +++ b/spec/rubocop/cop/commissioner_spec.rb @@ -7,15 +7,18 @@ end let(:report) { commissioner.investigate(processed_source) } - let(:cop_offenses) { [] } - let(:cop_report) do - RuboCop::Cop::Base::InvestigationReport - .new(nil, processed_source, cop_offenses, nil) + let(:cop_class) do + stub_const('Fake::FakeCop', Class.new(RuboCop::Cop::Base) do + def on_int(node); end + alias_method :on_def, :on_int + alias_method :on_send, :on_int + alias_method :on_csend, :on_int + end) end let(:cop) do - # rubocop:disable RSpec/VerifiedDoubles - double(RuboCop::Cop::Base, complete_investigation: cop_report).as_null_object - # rubocop:enable RSpec/VerifiedDoubles + cop_class.new.tap do |c| + allow(c).to receive(:complete_investigation).and_return(cop_report) + end end let(:cops) { [cop] } let(:options) { {} } @@ -28,6 +31,15 @@ def method end RUBY let(:processed_source) { parse_source(source, 'file.rb') } + let(:cop_offenses) { [] } + let(:cop_report) do + RuboCop::Cop::Base::InvestigationReport + .new(nil, processed_source, cop_offenses, nil) + end + + around do |example| + RuboCop::Cop::Registry.with_temporary_global { example.run } + end context 'when a cop reports offenses' do let(:cop_offenses) { [Object.new] } @@ -42,6 +54,40 @@ def method offenses end + context 'traverses the AST with on_send / on_csend' do + let(:source) { 'foo; var = bar; var&.baz' } + + context 'for unrestricted cops' do + it 'calls on_send all method calls' do + expect(cop).to receive(:on_send).twice + expect(cop).to receive(:on_csend).once + offenses + end + end + + context 'for a restricted cop' do + before { stub_const("#{cop_class}::RESTRICT_ON_SEND", restrictions) } + + let(:restrictions) { [:bar] } + + it 'calls on_send for the right method calls' do + expect(cop).to receive(:on_send).once + expect(cop).not_to receive(:on_csend) + offenses + end + + context 'on both csend and send' do + let(:restrictions) { %i[bar baz] } + + it 'calls on_send for the right method calls' do + expect(cop).to receive(:on_send).once + expect(cop).to receive(:on_csend).once + offenses + end + end + end + end + it 'stores all errors raised by the cops' do allow(cop).to receive(:on_int) { raise RuntimeError }