Skip to content

Commit

Permalink
[Fix rubocop#10024] Fix an incorrect auto-correct for `Style/Redundan…
Browse files Browse the repository at this point in the history
…tSelfAssignmentBranch`

Fixes rubocop#10024.

This PR fixes an incorrect auto-correct for `Style/RedundantSelfAssignmentBranch`
when using multiline `if` / `else` conditional assignment.
  • Loading branch information
koic committed Aug 19, 2021
1 parent 85219e5 commit 96c7559
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 71 deletions.
@@ -0,0 +1 @@
* [#10024](https://github.com/rubocop/rubocop/issues/10024): Fix an incorrect auto-correct for `Style/RedundantSelfAssignmentBranch` when using multiline `if` / `else` conditional assignment. ([@koic][])
44 changes: 20 additions & 24 deletions lib/rubocop/cop/style/redundant_self_assignment_branch.rb
Expand Up @@ -35,11 +35,11 @@ class RedundantSelfAssignmentBranch < Base

def on_lvasgn(node)
variable, expression = *node
return unless expression&.if_type?
return unless expression.ternary? || expression.else?
return unless use_if_and_else_branch?(expression)

if_branch = expression.if_branch
else_branch = expression.else_branch
return if inconvertible_to_modifier?(if_branch, else_branch)

if self_assign?(variable, if_branch)
register_offense(expression, if_branch, else_branch, 'unless')
Expand All @@ -50,35 +50,31 @@ def on_lvasgn(node)

private

def self_assign?(variable, branch)
variable.to_s == branch&.source
def use_if_and_else_branch?(expression)
return false unless expression&.if_type?

!expression.ternary? || !expression.else?
end

def register_offense(if_node, offense_branch, opposite_branch, keyword)
add_offense(offense_branch) do |corrector|
if if_node.ternary?
replacement = "#{opposite_branch.source} #{keyword} #{if_node.condition.source}"
corrector.replace(if_node, replacement)
else
if_node_loc = if_node.loc
def inconvertible_to_modifier?(if_branch, else_branch)
multiple_statements?(if_branch) || multiple_statements?(else_branch) ||
else_branch.respond_to?(:elsif?) && else_branch.elsif?
end

range = range_by_whole_lines(offense_branch.source_range, include_final_newline: true)
corrector.remove(range)
range = range_by_whole_lines(if_node_loc.else, include_final_newline: true)
corrector.remove(range)
def multiple_statements?(branch)
branch && branch.children.compact.count > 1
end

autocorrect_if_condition(corrector, if_node, if_node_loc, keyword)
end
end
def self_assign?(variable, branch)
variable.to_s == branch&.source
end

def autocorrect_if_condition(corrector, if_node, if_node_loc, keyword)
else_branch = if_node.else_branch
def register_offense(if_node, offense_branch, opposite_branch, keyword)
add_offense(offense_branch) do |corrector|
assignment_value = opposite_branch ? opposite_branch.source : 'nil'
replacement = "#{assignment_value} #{keyword} #{if_node.condition.source}"

if else_branch.respond_to?(:elsif?) && else_branch.elsif?
corrector.replace(if_node.condition, else_branch.condition.source)
else
corrector.replace(if_node_loc.keyword, keyword)
corrector.replace(if_node, replacement)
end
end
end
Expand Down
59 changes: 12 additions & 47 deletions spec/rubocop/cop/style/redundant_self_assignment_branch_spec.rb
Expand Up @@ -34,9 +34,7 @@
RUBY

expect_correction(<<~RUBY)
foo = if condition
bar
end
foo = bar if condition
RUBY
end

Expand All @@ -51,48 +49,30 @@
RUBY

expect_correction(<<~RUBY)
foo = unless condition
bar
end
foo = bar unless condition
RUBY
end

it 'registers and corrects an offense when self-assigning redundant else branch and multiline if branch' do
expect_offense(<<~RUBY)
it 'does not register an offense when self-assigning redundant else branch and multiline if branch' do
expect_no_offenses(<<~RUBY)
foo = if condition
bar
baz
else
foo
^^^ Remove the self-assignment branch.
end
RUBY

expect_correction(<<~RUBY)
foo = if condition
bar
baz
end
RUBY
end

it 'registers and corrects an offense when self-assigning redundant else branch and multiline else branch' do
expect_offense(<<~RUBY)
it 'does not register an offense when self-assigning redundant else branch and multiline else branch' do
expect_no_offenses(<<~RUBY)
foo = if condition
foo
^^^ Remove the self-assignment branch.
else
bar
baz
end
RUBY

expect_correction(<<~RUBY)
foo = unless condition
bar
baz
end
RUBY
end

it 'registers and corrects an offense when self-assigning redundant else branch and empty if branch' do
Expand All @@ -105,8 +85,7 @@
RUBY

expect_correction(<<~RUBY)
foo = if condition
end
foo = nil if condition
RUBY
end

Expand All @@ -120,8 +99,7 @@
RUBY

expect_correction(<<~RUBY)
foo = unless condition
end
foo = nil unless condition
RUBY
end

Expand Down Expand Up @@ -170,30 +148,19 @@
RUBY
end

it 'registers and corrects an offense when using `elsif` and self-assigning the value of `then` branch' do
expect_offense(<<~RUBY)
it 'does not register an offense when using `elsif` and self-assigning the value of `then` branch' do
expect_no_offenses(<<~RUBY)
foo = if condition
foo
^^^ Remove the self-assignment branch.
elsif another_condtion
bar
else
baz
end
RUBY

expect_correction(<<~RUBY)
foo = if another_condtion
bar
else
baz
end
RUBY
end

# It may be possible to extend it to register an offense in future.
# auto-correction test patterns should be considered and implemented.
it 'registers and corrects an offense when using `elsif` and self-assigning the value of `elsif` branch' do
it 'does not register an offense when using `elsif` and self-assigning the value of `elsif` branch' do
expect_no_offenses(<<~RUBY)
foo = if condition
bar
Expand All @@ -205,9 +172,7 @@
RUBY
end

# It may be possible to extend it to register an offense in future.
# auto-correction test patterns should be considered and implemented.
it 'registers and corrects an offense when using `elsif` and self-assigning the value of `else` branch' do
it 'does not register an offense when using `elsif` and self-assigning the value of `else` branch' do
expect_no_offenses(<<~RUBY)
foo = if condition
bar
Expand Down

0 comments on commit 96c7559

Please sign in to comment.