Skip to content

Commit d173f99

Browse files
koicbbatsov
authored andcommitted
[Fix #11040] Fix a false positive for Style/IfUnlessModifier
Fixes #11040. This PR fixes a false positive for `Style/IfUnlessModifier` when `defined?`'s argument value is undefined.
1 parent ff7c004 commit d173f99

File tree

3 files changed

+110
-0
lines changed

3 files changed

+110
-0
lines changed
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
* [#11040](https://github.com/rubocop/rubocop/issues/11040): Fix a false positive for `Style/IfUnlessModifier` when `defined?`'s argument value is undefined. ([@koic][])

lib/rubocop/cop/style/if_unless_modifier.rb

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,20 @@ module Style
1111
# cop. The tab size is configured in the `IndentationWidth` of the
1212
# `Layout/IndentationStyle` cop.
1313
#
14+
# NOTE: It is allowed when `defined?` argument has an undefined value,
15+
# because using the modifier form causes the following incompatibility:
16+
#
17+
# [source,ruby]
18+
# ----
19+
# unless defined?(undefined_foo)
20+
# undefined_foo = 'default_value'
21+
# end
22+
# undefined_foo # => 'default_value'
23+
#
24+
# undefined_bar = 'default_value' unless defined?(undefined_bar)
25+
# undefined_bar # => nil
26+
# ----
27+
#
1428
# @example
1529
# # bad
1630
# if condition
@@ -51,6 +65,8 @@ def self.autocorrect_incompatible_with
5165
end
5266

5367
def on_if(node)
68+
return if defined_nodes(node).any? { |n| defined_argument_is_undefined?(node, n) }
69+
5470
msg = if single_line_as_modifier?(node) && !named_capture_in_condition?(node)
5571
MSG_USE_MODIFIER
5672
elsif too_long_due_to_modifier?(node)
@@ -65,6 +81,24 @@ def on_if(node)
6581

6682
private
6783

84+
def defined_nodes(node)
85+
if node.condition.defined_type?
86+
[node.condition]
87+
else
88+
node.condition.each_descendant.select(&:defined_type?)
89+
end
90+
end
91+
92+
def defined_argument_is_undefined?(if_node, defined_node)
93+
defined_argument = defined_node.first_argument
94+
return false unless defined_argument.lvar_type? || defined_argument.send_type?
95+
96+
if_node.left_siblings.none? do |sibling|
97+
sibling.respond_to?(:lvasgn_type?) && sibling.lvasgn_type? &&
98+
sibling.name == defined_argument.node_parts[0]
99+
end
100+
end
101+
68102
def autocorrect(corrector, node)
69103
replacement = if node.modifier_form?
70104
last_argument = node.if_branch.last_argument if node.if_branch.send_type?

spec/rubocop/cop/style/if_unless_modifier_spec.rb

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,6 +371,81 @@ def f
371371
end
372372
end
373373

374+
context 'using `defined?` in the condtion' do
375+
it 'registers for argument value is defined' do
376+
expect_offense(<<~RUBY)
377+
value = :custom
378+
379+
unless defined?(value)
380+
^^^^^^ Favor modifier `unless` usage when having a single-line body. Another good alternative is the usage of control flow `&&`/`||`.
381+
value = :default
382+
end
383+
RUBY
384+
385+
expect_correction(<<~RUBY)
386+
value = :custom
387+
388+
value = :default unless defined?(value)
389+
RUBY
390+
end
391+
392+
it 'registers for argument value is `yield`' do
393+
expect_offense(<<~RUBY)
394+
unless defined?(yield)
395+
^^^^^^ Favor modifier `unless` usage when having a single-line body. Another good alternative is the usage of control flow `&&`/`||`.
396+
value = :default
397+
end
398+
RUBY
399+
400+
expect_correction(<<~RUBY)
401+
value = :default unless defined?(yield)
402+
RUBY
403+
end
404+
405+
it 'registers for argument value is `super`' do
406+
expect_offense(<<~RUBY)
407+
unless defined?(super)
408+
^^^^^^ Favor modifier `unless` usage when having a single-line body. Another good alternative is the usage of control flow `&&`/`||`.
409+
value = :default
410+
end
411+
RUBY
412+
413+
expect_correction(<<~RUBY)
414+
value = :default unless defined?(super)
415+
RUBY
416+
end
417+
418+
it 'accepts `defined?` when argument value is undefined' do
419+
expect_no_offenses(<<~RUBY)
420+
other_value = do_something
421+
422+
unless defined?(value)
423+
value = :default
424+
end
425+
RUBY
426+
end
427+
428+
it 'accepts `defined?` when argument value is undefined and the first condition' do
429+
expect_no_offenses(<<~RUBY)
430+
other_value = do_something
431+
432+
unless defined?(value) && condition
433+
value = :default
434+
end
435+
RUBY
436+
end
437+
438+
it 'accepts `defined?` when argument value is undefined and the last condition' do
439+
expect_no_offenses(<<~RUBY)
440+
other_value = do_something
441+
442+
unless condition && defined?(value)
443+
value = :default
444+
end
445+
RUBY
446+
end
447+
end
448+
374449
context 'with implicit match conditional' do
375450
let(:body) { 'b' * 36 }
376451

0 commit comments

Comments
 (0)