Skip to content

Commit

Permalink
[Fix #9731] Fix two autocorrection issues for `Style/NegatedIfElseCon…
Browse files Browse the repository at this point in the history
…dition`.

Previously, when swapping the `if` and `else` branches, if one of the branches only had a single statement, the branch would end up not being spaced out properly.

Additionally, when there is a node with a comment inside the `if` that has an identical node with a comment *before* the `if` statement, `CommentsHelp.begin_pos_with_comment` returns an incorrect range, which was causing a clobbering error. I fixed this by creating ranges of the entire body of the if and else branches, so that they can just be swapped without relying on `ProcessedSource#ast_with_comments`.
  • Loading branch information
dvandersluis committed May 2, 2021
1 parent 93a325f commit 3ce630f
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 9 deletions.
1 change: 1 addition & 0 deletions changelog/fix_fix_two_autocorrection_issues_for.md
@@ -0,0 +1 @@
* [#9731](https://github.com/rubocop/rubocop/issues/9731): Fix two autocorrection issues for `Style/NegatedIfElseCondition`. ([@dvandersluis][])
26 changes: 17 additions & 9 deletions lib/rubocop/cop/style/negated_if_else_condition.rb
Expand Up @@ -29,7 +29,6 @@ module Style
#
class NegatedIfElseCondition < Base
include RangeHelp
include CommentsHelp
extend AutoCorrector

MSG = 'Invert the negated condition and swap the %<type>s branches.'
Expand Down Expand Up @@ -98,21 +97,30 @@ def swap_branches(corrector, node)
if node.if_branch.nil?
corrector.remove(range_by_whole_lines(node.loc.else, include_final_newline: true))
else
if_range = node_with_comments(node.if_branch)
else_range = node_with_comments(node.else_branch)
if_range = if_range(node)
else_range = else_range(node)

corrector.replace(if_range, else_range.source)
corrector.replace(else_range, if_range.source)
end
end

def node_with_comments(node)
first_statement = node.begin_type? ? node.children[0] : node
return node if processed_source.ast_with_comments[first_statement].empty?
# Collect the entire if branch, including whitespace and comments
def if_range(node)
if node.ternary?
node.if_branch
else
range_between(node.condition.loc.expression.end_pos, node.loc.else.begin_pos)
end
end

begin_pos = source_range_with_comment(first_statement).begin_pos
end_pos = node.source_range.end_pos
Parser::Source::Range.new(buffer, begin_pos, end_pos)
# Collect the entire else branch, including whitespace and comments
def else_range(node)
if node.ternary?
node.else_branch
else
range_between(node.loc.else.end_pos, node.loc.end.begin_pos)
end
end
end
end
Expand Down
124 changes: 124 additions & 0 deletions spec/rubocop/cop/style/negated_if_else_condition_spec.rb
Expand Up @@ -50,6 +50,21 @@
RUBY
end

it 'registers an offens and corrects a multiline ternary' do
expect_offense(<<~RUBY)
!x ?
^^^^ Invert the negated condition and swap the ternary branches.
do_something :
do_something_else # comment
RUBY

expect_correction(<<~RUBY)
x ?
do_something_else :
do_something # comment
RUBY
end

shared_examples 'negation method' do |method, inverted_method|
it "registers an offense and corrects when negating condition with `#{method}` for `if-else`" do
expect_offense(<<~RUBY, method: method)
Expand Down Expand Up @@ -129,6 +144,23 @@
RUBY
end

it 'does not register an offense when the `else` branch is empty' do
expect_no_offenses(<<~RUBY)
if !condition.nil?
foo = 42
else
end
RUBY
end

it 'does not register an offense when both branches are empty' do
expect_no_offenses(<<~RUBY)
if !condition.nil?
else
end
RUBY
end

it 'moves comments to correct branches during autocorrect' do
expect_offense(<<~RUBY)
if !condition.nil?
Expand Down Expand Up @@ -187,6 +219,98 @@
RUBY
end

it 'works with comments when one branch is a begin and the other is not' do
expect_offense(<<~RUBY)
if !condition
^^^^^^^^^^^^^ Invert the negated condition and swap the if-else branches.
# comment
do_a
do_b
else
do_c
end
RUBY

expect_correction(<<~RUBY)
if condition
do_c
else
# comment
do_a
do_b
end
RUBY
end

it 'works with comments when neither branch is a begin node' do
expect_offense(<<~RUBY)
if !condition
^^^^^^^^^^^^^ Invert the negated condition and swap the if-else branches.
# comment
do_b
else
do_c
end
RUBY

expect_correction(<<~RUBY)
if condition
do_c
else
# comment
do_b
end
RUBY
end

it 'works with duplicate nodes' do
expect_offense(<<~RUBY)
# outer comment
do_a
if !condition
^^^^^^^^^^^^^ Invert the negated condition and swap the if-else branches.
# comment
do_a
else
do_c
end
RUBY

expect_correction(<<~RUBY)
# outer comment
do_a
if condition
do_c
else
# comment
do_a
end
RUBY
end

it 'correctly moves comments at the end of branches' do
expect_offense(<<~RUBY)
if !condition
^^^^^^^^^^^^^ Invert the negated condition and swap the if-else branches.
do_a
# comment
else
do_c
end
RUBY

expect_correction(<<~RUBY)
if condition
do_c
else
do_a
# comment
end
RUBY
end

it 'does not register an offense when negating condition for `if-elsif`' do
expect_no_offenses(<<~RUBY)
if !x
Expand Down

0 comments on commit 3ce630f

Please sign in to comment.