Skip to content

Commit

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

This PR fixes a false positive for `Style/IfUnlessModifier`
when using one-line pattern matching as a `if` condition.
  • Loading branch information
koic authored and bbatsov committed Mar 23, 2023
1 parent 0102e57 commit 7962934
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 12 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#11723](https://github.com/rubocop/rubocop/issues/11723): Fix a false positive for `Style/IfUnlessModifier` when using one-line pattern matching as a `if` condition. ([@koic][])
50 changes: 38 additions & 12 deletions lib/rubocop/cop/style/if_unless_modifier.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,18 @@ module Style
# cop. The tab size is configured in the `IndentationWidth` of the
# `Layout/IndentationStyle` cop.
#
# One-line pattern matching is always allowed. To ensure that there are few cases
# where the match variable is not used, and to prevent oversights. The variable `x`
# becomes undefined and raises `NameError` when the following example is changed to
# the modifier form:
#
# [source,ruby]
# ----
# if [42] in [x]
# x # `x` is undefined when using modifier form.
# end
# ----
#
# NOTE: It is allowed when `defined?` argument has an undefined value,
# because using the modifier form causes the following incompatibility:
#
Expand Down Expand Up @@ -66,14 +78,10 @@ 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)
MSG_USE_NORMAL
end
return unless msg
condition = node.condition
return if defined_nodes(condition).any? { |n| defined_argument_is_undefined?(node, n) } ||
pattern_matching_nodes(condition).any?
return unless (msg = message(node))

add_offense(node.loc.keyword, message: format(msg, keyword: node.keyword)) do |corrector|
autocorrect(corrector, node)
Expand All @@ -82,11 +90,11 @@ def on_if(node)

private

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

Expand All @@ -100,6 +108,24 @@ def defined_argument_is_undefined?(if_node, defined_node)
end
end

def pattern_matching_nodes(condition)
if condition.match_pattern_type? || condition.match_pattern_p_type?
[condition]
else
condition.each_descendant.select do |node|
node.match_pattern_type? || node.match_pattern_p_type?
end
end
end

def message(node)
if single_line_as_modifier?(node) && !named_capture_in_condition?(node)
MSG_USE_MODIFIER
elsif too_long_due_to_modifier?(node)
MSG_USE_NORMAL
end
end

def autocorrect(corrector, node)
replacement = if node.modifier_form?
replacement_for_modifier_form(corrector, node)
Expand Down
44 changes: 44 additions & 0 deletions spec/rubocop/cop/style/if_unless_modifier_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,50 @@ def f
RUBY
end

shared_examples 'one-line pattern matching' do
it 'does not register an offense when using match var in body' do
expect_no_offenses(<<~RUBY)
if [42] in [x]
x
end
RUBY
end

it 'does not register an offense when using some match var in body' do
expect_no_offenses(<<~RUBY)
if { x: 1, y: 2 } in { x:, y: }
a && y
end
RUBY
end

it 'does not register an offense when not using match var in body' do
expect_no_offenses(<<~RUBY)
if [42] in [x]
y
end
RUBY
end

it 'does not register an offense when not using any match var in body' do
expect_no_offenses(<<~RUBY)
if { x: 1, y: 2 } in { x:, y: }
a && b
end
RUBY
end
end

# The node type for one-line `in` pattern matching in Ruby 2.7 is `match_pattern`.
context 'using `match_pattern` as a one-line pattern matching', :ruby27 do
include_examples 'one-line pattern matching'
end

# The node type for one-line `in` pattern matching in Ruby 3.0 is `match_pattern_p`.
context 'using `match_pattern_p` as a one-line pattern matching', :ruby30 do
include_examples 'one-line pattern matching'
end

context 'multiline `if` that fits on one line and using hash value omission syntax', :ruby31 do
it 'registers an offense' do
expect_offense(<<~RUBY)
Expand Down

0 comments on commit 7962934

Please sign in to comment.