Skip to content

Commit

Permalink
[Fix #12140] Add new Style/CombinableDefined cop.
Browse files Browse the repository at this point in the history
  • Loading branch information
dvandersluis authored and bbatsov committed Oct 31, 2024
1 parent f8aa27f commit 37e9e5f
Show file tree
Hide file tree
Showing 5 changed files with 326 additions and 0 deletions.
1 change: 1 addition & 0 deletions changelog/new_add_new_style_combinable_defined_cop.md
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][])
5 changes: 5 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3553,6 +3553,11 @@ Style/ColonMethodDefinition:
Enabled: true
VersionAdded: '0.52'

Style/CombinableDefined:
Description: 'Checks successive `defined?` calls that can be combined into a single call.'
Enabled: pending
VersionAdded: '<<next>>'

Style/CombinableLoops:
Description: >-
Checks for places where multiple consecutive loops over the same data
Expand Down
1 change: 1 addition & 0 deletions lib/rubocop.rb
Original file line number Diff line number Diff line change
Expand Up @@ -492,6 +492,7 @@
require_relative 'rubocop/cop/style/collection_methods'
require_relative 'rubocop/cop/style/colon_method_call'
require_relative 'rubocop/cop/style/colon_method_definition'
require_relative 'rubocop/cop/style/combinable_defined'
require_relative 'rubocop/cop/style/combinable_loops'
require_relative 'rubocop/cop/style/command_literal'
require_relative 'rubocop/cop/style/comment_annotation'
Expand Down
115 changes: 115 additions & 0 deletions lib/rubocop/cop/style/combinable_defined.rb
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
204 changes: 204 additions & 0 deletions spec/rubocop/cop/style/combinable_defined_spec.rb
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

0 comments on commit 37e9e5f

Please sign in to comment.