Skip to content

Commit

Permalink
Allow cops to restrict callbacks on_send to specific method names only
Browse files Browse the repository at this point in the history
  • Loading branch information
marcandre authored and mergify[bot] committed Aug 27, 2020
1 parent c09e2c6 commit af336ca
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 11 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -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

Expand Down
5 changes: 5 additions & 0 deletions docs/modules/ROOT/pages/development.adoc
Expand Up @@ -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]
Expand All @@ -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)
Expand Down
7 changes: 7 additions & 0 deletions lib/rubocop/cop/base.rb
Expand Up @@ -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
#
Expand Down Expand Up @@ -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 = []
Expand Down
45 changes: 41 additions & 4 deletions lib/rubocop/cop/commissioner.rb
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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]
Expand Down Expand Up @@ -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)
Expand Down
60 changes: 53 additions & 7 deletions spec/rubocop/cop/commissioner_spec.rb
Expand Up @@ -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) { {} }
Expand All @@ -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] }
Expand All @@ -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 }

Expand Down

0 comments on commit af336ca

Please sign in to comment.