-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
1 parent
f8aa27f
commit 37e9e5f
Showing
5 changed files
with
326 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
* [#12140](https://github.com/rubocop/rubocop/issues/12140): Add new `Style/CombinableDefined` cop. ([@dvandersluis][]) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
# frozen_string_literal: true | ||
|
||
module RuboCop | ||
module Cop | ||
module Style | ||
# Checks for multiple `defined?` calls joined by `&&` that can be combined | ||
# into a single `defined?`. | ||
# | ||
# When checking that a nested constant or chained method is defined, it is | ||
# not necessary to check each ancestor or component of the chain. | ||
# | ||
# @example | ||
# # bad | ||
# defined?(Foo) && defined?(Foo::Bar) && defined?(Foo::Bar::Baz) | ||
# | ||
# # good | ||
# defined?(Foo::Bar::Baz) | ||
# | ||
# # bad | ||
# defined?(foo) && defined?(foo.bar) && defined?(foo.bar.baz) | ||
# | ||
# # good | ||
# defined?(foo.bar.baz) | ||
class CombinableDefined < Base | ||
extend AutoCorrector | ||
include RangeHelp | ||
|
||
MSG = 'Combine nested `defined?` calls.' | ||
OPERATORS = %w[&& and].freeze | ||
|
||
def on_and(node) | ||
# Only register an offense if all `&&` terms are `defined?` calls | ||
return unless (terms = terms(node)).all?(&:defined_type?) | ||
|
||
calls = defined_calls(terms) | ||
namespaces = namespaces(calls) | ||
|
||
calls.each do |call| | ||
next unless namespaces.any?(call) | ||
|
||
add_offense(node) do |corrector| | ||
remove_term(corrector, call) | ||
end | ||
end | ||
end | ||
|
||
private | ||
|
||
def terms(node) | ||
node.each_descendant.select do |descendant| | ||
descendant.parent.and_type? && !descendant.and_type? | ||
end | ||
end | ||
|
||
def defined_calls(nodes) | ||
nodes.filter_map do |defined_node| | ||
subject = defined_node.first_argument | ||
subject if subject.const_type? || subject.call_type? | ||
end | ||
end | ||
|
||
def namespaces(nodes) | ||
nodes.filter_map do |node| | ||
if node.respond_to?(:namespace) | ||
node.namespace | ||
elsif node.respond_to?(:receiver) | ||
node.receiver | ||
end | ||
end | ||
end | ||
|
||
def remove_term(corrector, term) | ||
term = term.parent until term.parent.and_type? | ||
range = if term == term.parent.children.last | ||
rhs_range_to_remove(term) | ||
else | ||
lhs_range_to_remove(term) | ||
end | ||
|
||
corrector.remove(range) | ||
end | ||
|
||
# If the redundant `defined?` node is the LHS of an `and` node, | ||
# the term as well as the subsequent `&&`/`and` operator will be removed. | ||
def lhs_range_to_remove(term) | ||
source = @processed_source.buffer.source | ||
|
||
pos = term.source_range.end_pos | ||
pos += 1 until source[..pos].end_with?(*OPERATORS) | ||
|
||
range_with_surrounding_space( | ||
range: term.source_range.with(end_pos: pos + 1), | ||
side: :right, | ||
newlines: false | ||
) | ||
end | ||
|
||
# If the redundant `defined?` node is the RHS of an `and` node, | ||
# the term as well as the preceding `&&`/`and` operator will be removed. | ||
def rhs_range_to_remove(term) | ||
source = @processed_source.buffer.source | ||
|
||
pos = term.source_range.begin_pos | ||
pos -= 1 until source[pos, 3].start_with?(*OPERATORS) | ||
|
||
range_with_surrounding_space( | ||
range: term.source_range.with(begin_pos: pos - 1), | ||
side: :right, | ||
newlines: false | ||
) | ||
end | ||
end | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,204 @@ | ||
# frozen_string_literal: true | ||
|
||
RSpec.describe RuboCop::Cop::Style::CombinableDefined, :config do | ||
it 'does not register an offense for a single `defined?`' do | ||
expect_no_offenses(<<~RUBY) | ||
defined?(Foo) | ||
RUBY | ||
end | ||
|
||
%i[&& and].each do |operator| | ||
context "joined by `#{operator}`" do | ||
it 'does not register an offense for two separate `defined?`s' do | ||
expect_no_offenses(<<~RUBY) | ||
defined?(Foo) #{operator} defined?(Bar) | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense for two identical `defined?`s' do | ||
# handled by Lint/BinaryOperatorWithIdenticalOperands | ||
expect_no_offenses(<<~RUBY) | ||
defined?(Foo) #{operator} defined?(Foo) | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense for the same constant with different `cbase`s' do | ||
expect_no_offenses(<<~RUBY) | ||
defined?(Foo) #{operator} defined?(::Foo) | ||
RUBY | ||
end | ||
|
||
it 'registers an offense for two `defined?`s with same nesting' do | ||
expect_offense(<<~RUBY, operator: operator) | ||
defined?(Foo) #{operator} defined?(Foo::Bar) | ||
^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
defined?(Foo::Bar) | ||
RUBY | ||
end | ||
|
||
it 'registers an offense for two `defined?`s with the same nesting in reverse order' do | ||
expect_offense(<<~RUBY, operator: operator) | ||
defined?(Foo::Bar) #{operator} defined?(Foo) | ||
^^^^^^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
defined?(Foo::Bar) | ||
RUBY | ||
end | ||
|
||
it 'registers an offense for two `defined?`s with the same nesting and `cbase`' do | ||
expect_offense(<<~RUBY, operator: operator) | ||
defined?(::Foo) #{operator} defined?(::Foo::Bar) | ||
^^^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
defined?(::Foo::Bar) | ||
RUBY | ||
end | ||
|
||
it 'registers an offense for two `defined?`s with the same deep nesting' do | ||
expect_offense(<<~RUBY, operator: operator) | ||
defined?(Foo::Bar) #{operator} defined?(Foo::Bar::Baz) | ||
^^^^^^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
defined?(Foo::Bar::Baz) | ||
RUBY | ||
end | ||
|
||
it 'registers an offense for three `defined?`s with same nesting' do | ||
expect_offense(<<~RUBY, operator: operator) | ||
defined?(Foo) #{operator} defined?(Foo::Bar) #{operator} defined?(Foo::Bar::Baz) | ||
^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
defined?(Foo::Bar::Baz) | ||
RUBY | ||
end | ||
|
||
it 'registers an offense for three `defined?`s with the same module ancestor' do | ||
expect_offense(<<~RUBY, operator: operator) | ||
defined?(Foo) #{operator} defined?(Foo::Bar) #{operator} defined?(Foo::Baz) | ||
^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
defined?(Foo::Bar) #{operator} defined?(Foo::Baz) | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense for two `defined?`s with same namespace but different nesting' do | ||
expect_no_offenses(<<~RUBY) | ||
defined?(Foo::Bar) #{operator} defined?(Foo::Baz) | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense for two `defined?`s with negation' do | ||
expect_no_offenses(<<~RUBY) | ||
defined?(Foo) #{operator} !defined?(Foo::Bar) | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense for two `defined?` with different `cbase`s' do | ||
expect_no_offenses(<<~RUBY) | ||
defined?(::Foo) #{operator} defined?(Foo::Bar) | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense when skipping a nesting level' do | ||
expect_no_offenses(<<~RUBY) | ||
defined?(Foo) #{operator} defined?(Foo::Bar::Baz) | ||
RUBY | ||
end | ||
|
||
it 'registers an offense when the namespace is not a constant' do | ||
expect_offense(<<~RUBY, operator: operator) | ||
defined?(foo) #{operator} defined?(foo::Bar) | ||
^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
defined?(foo::Bar) | ||
RUBY | ||
end | ||
|
||
it 'registers an offense for method chain with dots' do | ||
expect_offense(<<~RUBY, operator: operator) | ||
defined?(foo) #{operator} defined?(foo.bar) | ||
^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
defined?(foo.bar) | ||
RUBY | ||
end | ||
|
||
it 'registers an offense for method chain with `::`' do | ||
expect_offense(<<~RUBY, operator: operator) | ||
defined?(foo) #{operator} defined?(foo::bar) | ||
^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
defined?(foo::bar) | ||
RUBY | ||
end | ||
|
||
it 'registers an offense for a method chain followed by constant nesting' do | ||
expect_offense(<<~RUBY, operator: operator) | ||
defined?(foo) #{operator} defined?(foo.bar) #{operator} defined?(foo.bar::BAZ) | ||
^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^^^{operator}^^^^^^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
defined?(foo.bar::BAZ) | ||
RUBY | ||
end | ||
|
||
it 'does not register an offense when there is another term between `defined?`s' do | ||
expect_no_offenses(<<~RUBY) | ||
foo #{operator} defined?(Foo) #{operator} bar #{operator} defined?(Foo::Bar) | ||
RUBY | ||
end | ||
end | ||
end | ||
|
||
context 'mixed operators' do | ||
it 'registers an offense and corrects' do | ||
expect_offense(<<~RUBY) | ||
defined?(Foo) && defined?(Foo::Bar) and defined?(Foo::Bar::Baz) | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
RUBY | ||
|
||
expect_correction(<<~RUBY) | ||
defined?(Foo::Bar::Baz) | ||
RUBY | ||
end | ||
|
||
it 'registers an offense and corrects when an operator is retained' do | ||
expect_offense(<<~RUBY) | ||
defined?(Foo) && defined?(Foo::Bar) and defined?(Foo::Baz) | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Combine nested `defined?` calls. | ||
RUBY | ||
|
||
# The deleted operator is the one attached to the term being removed | ||
# (in this case `defined?(Foo)`). | ||
# `Style/AndOr` will subsequently update the operator if necessary. | ||
expect_correction(<<~RUBY) | ||
defined?(Foo::Bar) and defined?(Foo::Baz) | ||
RUBY | ||
end | ||
end | ||
end |