From 96c75599167ca55ae3ea2cd908c440efdadac6ba Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Thu, 19 Aug 2021 02:44:49 +0900 Subject: [PATCH] [Fix #10024] Fix an incorrect auto-correct for `Style/RedundantSelfAssignmentBranch` Fixes #10024. This PR fixes an incorrect auto-correct for `Style/RedundantSelfAssignmentBranch` when using multiline `if` / `else` conditional assignment. --- ...ct_for_redundant_self_assignment_branch.md | 1 + .../style/redundant_self_assignment_branch.rb | 44 +++++++------- .../redundant_self_assignment_branch_spec.rb | 59 ++++--------------- 3 files changed, 33 insertions(+), 71 deletions(-) create mode 100644 changelog/fix_incorrect_autocorrect_for_redundant_self_assignment_branch.md diff --git a/changelog/fix_incorrect_autocorrect_for_redundant_self_assignment_branch.md b/changelog/fix_incorrect_autocorrect_for_redundant_self_assignment_branch.md new file mode 100644 index 00000000000..556d10b6887 --- /dev/null +++ b/changelog/fix_incorrect_autocorrect_for_redundant_self_assignment_branch.md @@ -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][]) diff --git a/lib/rubocop/cop/style/redundant_self_assignment_branch.rb b/lib/rubocop/cop/style/redundant_self_assignment_branch.rb index d8a9dd8c670..a6cfa356360 100644 --- a/lib/rubocop/cop/style/redundant_self_assignment_branch.rb +++ b/lib/rubocop/cop/style/redundant_self_assignment_branch.rb @@ -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') @@ -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 diff --git a/spec/rubocop/cop/style/redundant_self_assignment_branch_spec.rb b/spec/rubocop/cop/style/redundant_self_assignment_branch_spec.rb index bc88cc7050e..846d4fd2f59 100644 --- a/spec/rubocop/cop/style/redundant_self_assignment_branch_spec.rb +++ b/spec/rubocop/cop/style/redundant_self_assignment_branch_spec.rb @@ -34,9 +34,7 @@ RUBY expect_correction(<<~RUBY) - foo = if condition - bar - end + foo = bar if condition RUBY end @@ -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 @@ -105,8 +85,7 @@ RUBY expect_correction(<<~RUBY) - foo = if condition - end + foo = nil if condition RUBY end @@ -120,8 +99,7 @@ RUBY expect_correction(<<~RUBY) - foo = unless condition - end + foo = nil unless condition RUBY end @@ -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 @@ -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