Skip to content

Commit

Permalink
[Fix #11040] Fix a false positive for Style/IfUnlessModifier
Browse files Browse the repository at this point in the history
Fixes #11040.

This PR fixes a false positive for `Style/IfUnlessModifier`
when `defined?`'s argument value is undefined.
  • Loading branch information
koic authored and bbatsov committed Feb 28, 2023
1 parent ff7c004 commit d173f99
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#11040](https://github.com/rubocop/rubocop/issues/11040): Fix a false positive for `Style/IfUnlessModifier` when `defined?`'s argument value is undefined. ([@koic][])
34 changes: 34 additions & 0 deletions lib/rubocop/cop/style/if_unless_modifier.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,20 @@ module Style
# cop. The tab size is configured in the `IndentationWidth` of the
# `Layout/IndentationStyle` cop.
#
# NOTE: It is allowed when `defined?` argument has an undefined value,
# because using the modifier form causes the following incompatibility:
#
# [source,ruby]
# ----
# unless defined?(undefined_foo)
# undefined_foo = 'default_value'
# end
# undefined_foo # => 'default_value'
#
# undefined_bar = 'default_value' unless defined?(undefined_bar)
# undefined_bar # => nil
# ----
#
# @example
# # bad
# if condition
Expand Down Expand Up @@ -51,6 +65,8 @@ def self.autocorrect_incompatible_with
end

def on_if(node)
return if defined_nodes(node).any? { |n| defined_argument_is_undefined?(node, n) }

msg = if single_line_as_modifier?(node) && !named_capture_in_condition?(node)
MSG_USE_MODIFIER
elsif too_long_due_to_modifier?(node)
Expand All @@ -65,6 +81,24 @@ def on_if(node)

private

def defined_nodes(node)
if node.condition.defined_type?
[node.condition]
else
node.condition.each_descendant.select(&:defined_type?)
end
end

def defined_argument_is_undefined?(if_node, defined_node)
defined_argument = defined_node.first_argument
return false unless defined_argument.lvar_type? || defined_argument.send_type?

if_node.left_siblings.none? do |sibling|
sibling.respond_to?(:lvasgn_type?) && sibling.lvasgn_type? &&
sibling.name == defined_argument.node_parts[0]
end
end

def autocorrect(corrector, node)
replacement = if node.modifier_form?
last_argument = node.if_branch.last_argument if node.if_branch.send_type?
Expand Down
75 changes: 75 additions & 0 deletions spec/rubocop/cop/style/if_unless_modifier_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,81 @@ def f
end
end

context 'using `defined?` in the condtion' do
it 'registers for argument value is defined' do
expect_offense(<<~RUBY)
value = :custom
unless defined?(value)
^^^^^^ Favor modifier `unless` usage when having a single-line body. Another good alternative is the usage of control flow `&&`/`||`.
value = :default
end
RUBY

expect_correction(<<~RUBY)
value = :custom
value = :default unless defined?(value)
RUBY
end

it 'registers for argument value is `yield`' do
expect_offense(<<~RUBY)
unless defined?(yield)
^^^^^^ Favor modifier `unless` usage when having a single-line body. Another good alternative is the usage of control flow `&&`/`||`.
value = :default
end
RUBY

expect_correction(<<~RUBY)
value = :default unless defined?(yield)
RUBY
end

it 'registers for argument value is `super`' do
expect_offense(<<~RUBY)
unless defined?(super)
^^^^^^ Favor modifier `unless` usage when having a single-line body. Another good alternative is the usage of control flow `&&`/`||`.
value = :default
end
RUBY

expect_correction(<<~RUBY)
value = :default unless defined?(super)
RUBY
end

it 'accepts `defined?` when argument value is undefined' do
expect_no_offenses(<<~RUBY)
other_value = do_something
unless defined?(value)
value = :default
end
RUBY
end

it 'accepts `defined?` when argument value is undefined and the first condition' do
expect_no_offenses(<<~RUBY)
other_value = do_something
unless defined?(value) && condition
value = :default
end
RUBY
end

it 'accepts `defined?` when argument value is undefined and the last condition' do
expect_no_offenses(<<~RUBY)
other_value = do_something
unless condition && defined?(value)
value = :default
end
RUBY
end
end

context 'with implicit match conditional' do
let(:body) { 'b' * 36 }

Expand Down

0 comments on commit d173f99

Please sign in to comment.