Skip to content

Commit

Permalink
[Fix #10325] Enhance Style/RedundantCondition by considering the ca…
Browse files Browse the repository at this point in the history
…se that variable assignments in each branch
  • Loading branch information
nobuyo committed Apr 30, 2022
1 parent 793f145 commit 96d0a99
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 4 deletions.
1 change: 1 addition & 0 deletions changelog/fix_enhance_style_redundantcondition.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#10325](https://github.com/rubocop/rubocop/issues/10325): Enhance `Style/RedundantCondition` by considering the case that variable assignments in each branch. ([@nobuyo][])
42 changes: 38 additions & 4 deletions lib/rubocop/cop/style/redundant_condition.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,12 @@ def range_of_offense(node)
end

def offense?(node)
condition, if_branch, else_branch = *node
_condition, _if_branch, else_branch = *node

return false if use_if_branch?(else_branch) || use_hash_key_assignment?(else_branch)

condition == if_branch && !node.elsif? && (
node.ternary? || !else_branch.instance_of?(AST::Node) || else_branch.single_line?
)
synonymous_condition_and_branch?(node) && !node.elsif? &&
(node.ternary? || !else_branch.instance_of?(AST::Node) || else_branch.single_line?)
end

def use_if_branch?(else_branch)
Expand All @@ -90,11 +89,46 @@ def use_hash_key_assignment?(else_branch)
else_branch&.send_type? && else_branch&.method?(:[]=)
end

def synonymous_condition_and_branch?(node)
condition, if_branch, _else_branch = *node
# e.g.
# if var
# var
# else
# 'foo'
# end
return true if condition == if_branch

# e.g.
# if foo
# @value = foo
# else
# @value = another_value?
# end
branches_have_assignment?(node) && condition == if_branch.expression
end

def branches_have_assignment?(node)
_condition, if_branch, else_branch = *node

return false unless if_branch && else_branch

asgn_type?(if_branch) && (if_branch_variable_name = if_branch.name) &&
asgn_type?(else_branch) && (else_branch_variable_name = else_branch.name) &&
if_branch_variable_name == else_branch_variable_name
end

def asgn_type?(node)
node.lvasgn_type? || node.ivasgn_type? || node.cvasgn_type? || node.gvasgn_type?
end

def else_source(else_branch)
if require_parentheses?(else_branch)
"(#{else_branch.source})"
elsif without_argument_parentheses_method?(else_branch)
"#{else_branch.method_name}(#{else_branch.arguments.map(&:source).join(', ')})"
elsif branches_have_assignment?(else_branch.parent)
else_branch.expression.source
else
else_branch.source
end
Expand Down
54 changes: 54 additions & 0 deletions spec/rubocop/cop/style/redundant_condition_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,60 @@
foo || (1..2)
RUBY
end

it 'registers an offense and corrects when defined inside method and the branches contains assignment' do
expect_offense(<<~RUBY)
def test
if foo
^^^^^^ Use double pipes `||` instead.
@value = foo
else
@value = 'bar'
end
end
RUBY

expect_correction(<<~RUBY)
def test
@value = foo || 'bar'
end
RUBY
end

it 'registers an offense and corrects when the branches contains assignment' do
expect_offense(<<~RUBY)
if foo
^^^^^^ Use double pipes `||` instead.
@value = foo
else
@value = 'bar'
end
RUBY

expect_correction(<<~RUBY)
@value = foo || 'bar'
RUBY
end

it 'does not register offenses when using `nil?` and the branches contains assignment' do
expect_no_offenses(<<~RUBY)
if foo.nil?
@value = foo
else
@value = 'bar'
end
RUBY
end

it 'does not register offenses when the branches contains assignment but target not matched' do
expect_no_offenses(<<~RUBY)
if foo
@foo = foo
else
@baz = 'quux'
end
RUBY
end
end
end

Expand Down

0 comments on commit 96d0a99

Please sign in to comment.