Skip to content

Commit

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

This PR fixes a false positive `Style/IdenticalConditionalBranches`
when `if`..`else` with identical leading lines and assign to condition value.
  • Loading branch information
koic authored and bbatsov committed Oct 10, 2023
1 parent 6a100e6 commit 86c0e8d
Show file tree
Hide file tree
Showing 3 changed files with 115 additions and 3 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#12249](https://github.com/rubocop/rubocop/issues/12249): Fix a false positive `Style/IdenticalConditionalBranches` when `if`..`else` with identical leading lines and assign to condition value. ([@koic][])
20 changes: 17 additions & 3 deletions lib/rubocop/cop/style/identical_conditional_branches.rb
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ def on_case_match(node)

private

# rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
# rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
def check_branches(node, branches)
# return if any branch is empty. An empty branch can be an `if`
# without an `else` or a branch that contains only comments.
Expand All @@ -149,9 +149,15 @@ def check_branches(node, branches)
branches.any? { |branch| single_child_branch?(branch) }

heads = branches.map { |branch| head(branch) }
check_expressions(node, heads, :before_condition) if duplicated_expressions?(node, heads)

return unless duplicated_expressions?(node, heads)

condition_variable = assignable_condition_value(node)
return if heads.first.assignment? && condition_variable == heads.first.name.to_s

check_expressions(node, heads, :before_condition)
end
# rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
# rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity

def duplicated_expressions?(node, expressions)
unique_expressions = expressions.uniq
Expand All @@ -164,6 +170,14 @@ def duplicated_expressions?(node, expressions)
node.condition.child_nodes.none? { |n| n.source == lhs.source if n.variable? }
end

def assignable_condition_value(node)
if node.condition.call_type?
(receiver = node.condition.receiver) ? receiver.source : node.condition.source
elsif node.condition.variable?
node.condition.source
end
end

# rubocop:disable Metrics/CyclomaticComplexity, Metrics/MethodLength, Metrics/PerceivedComplexity
def check_expressions(node, expressions, insert_position)
return if expressions.any?(&:nil?)
Expand Down
97 changes: 97 additions & 0 deletions spec/rubocop/cop/style/identical_conditional_branches_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,103 @@
end
end

context 'on if..else with identical leading lines and assign to condition value of method call receiver' do
it "doesn't register an offense" do
expect_no_offenses(<<~RUBY)
if x.condition
x = do_something
foo
else
x = do_something
bar
end
RUBY
end
end

context 'on if..else with identical leading lines and assign to condition value of safe navigation call receiver' do
it "doesn't register an offense" do
expect_no_offenses(<<~RUBY)
if x&.condition
x = do_something
foo
else
x = do_something
bar
end
RUBY
end
end

context 'on if..else with identical leading lines and assign to condition value of method call' do
it "doesn't register an offense" do
expect_no_offenses(<<~RUBY)
if x
x = do_something
foo
else
x = do_something
bar
end
RUBY
end
end

context 'on if..else with identical leading lines and assign to condition local variable' do
it "doesn't register an offense" do
expect_no_offenses(<<~RUBY)
x = 42
if x
x = do_something
foo
else
x = do_something
bar
end
RUBY
end
end

context 'on if..else with identical leading lines and assign to condition instance variable' do
it "doesn't register an offense" do
expect_no_offenses(<<~RUBY)
if @x
@x = do_something
foo
else
@x = do_something
bar
end
RUBY
end
end

context 'on if..else with identical trailing lines and assign to condition value' do
it 'registers and corrects an offense' do
expect_offense(<<~RUBY)
if x.condition
foo
x = do_something
^^^^^^^^^^^^^^^^ Move `x = do_something` out of the conditional.
else
bar
x = do_something
^^^^^^^^^^^^^^^^ Move `x = do_something` out of the conditional.
end
RUBY

expect_correction(<<~RUBY)
if x.condition
foo
else
bar
end
x = do_something
RUBY
end
end

context 'on if..else with identical leading lines, single child branch and last node of the parent' do
it "doesn't register an offense" do
expect_no_offenses(<<~RUBY)
Expand Down

0 comments on commit 86c0e8d

Please sign in to comment.