Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add autocorrection to Lint/EmptyConditionalBody #10862

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/change_add_autocorrection_to.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#10862](https://github.com/rubocop/rubocop/pull/10862): Add autocorrection to `Lint/EmptyConditionalBody`. ([@dvandersluis][])
1 change: 1 addition & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1755,6 +1755,7 @@ Lint/EmptyConditionalBody:
Enabled: true
AllowComments: true
VersionAdded: '0.89'
VersionChanged: '<<next>>'

Lint/EmptyEnsure:
Description: 'Checks for empty ensure block.'
Expand Down
61 changes: 60 additions & 1 deletion lib/rubocop/cop/lint/empty_conditional_body.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ module RuboCop
module Cop
module Lint
# Checks for the presence of `if`, `elsif` and `unless` branches without a body.
#
# NOTE: empty `else` branches are handled by `Style/EmptyElse`.
#
# @example
# # bad
# if condition
Expand Down Expand Up @@ -53,15 +56,71 @@ module Lint
# end
#
class EmptyConditionalBody < Base
extend AutoCorrector
include CommentsHelp
include RangeHelp

MSG = 'Avoid `%<keyword>s` branches without a body.'

def on_if(node)
return if node.body
return if cop_config['AllowComments'] && contains_comments?(node)

add_offense(node, message: format(MSG, keyword: node.keyword))
add_offense(node, message: format(MSG, keyword: node.keyword)) do |corrector|
autocorrect(corrector, node)
end
end

private

def autocorrect(corrector, node)
remove_comments(corrector, node)
remove_empty_branch(corrector, node)
correct_other_branches(corrector, node)
end

def remove_comments(corrector, node)
comments_in_range(node).each do |comment|
range = range_by_whole_lines(comment.loc.expression, include_final_newline: true)
corrector.remove(range)
end
end

def remove_empty_branch(corrector, node)
corrector.remove(deletion_range(branch_range(node)))
end

def correct_other_branches(corrector, node)
return unless (node.if? || node.unless?) && node.else_branch

if node.else_branch.if_type?
# Replace an orphaned `elsif` with `if`
corrector.replace(node.else_branch.loc.keyword, 'if')
else
# Flip orphaned `else`
corrector.replace(node.loc.else, "#{node.inverse_keyword} #{node.condition.source}")
end
end

def branch_range(node)
if node.loc.else
node.source_range.with(end_pos: node.loc.else.begin_pos - 1)
else
node.source_range
end
end

def deletion_range(range)
# Collect a range between the start of the `if` node and the next relevant node,
# including final new line.
# Based on `RangeHelp#range_by_whole_lines` but allows the `if` to not start
# on the first column.
buffer = @processed_source.buffer

last_line = buffer.source_line(range.last_line)
end_offset = last_line.length - range.last_column + 1

range.adjust(end_pos: end_offset).intersect(buffer.source_range)
end
end
end
Expand Down
6 changes: 5 additions & 1 deletion lib/rubocop/cop/mixin/comments_help.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,14 @@ def source_range_with_comment(node)
end

def contains_comments?(node)
comments_in_range(node).any?
end

def comments_in_range(node)
start_line = node.source_range.line
end_line = find_end_line(node)

processed_source.each_comment_in_lines(start_line...end_line).any?
processed_source.each_comment_in_lines(start_line...end_line)
end

private
Expand Down
104 changes: 104 additions & 0 deletions spec/rubocop/cop/lint/empty_conditional_body_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
^^^^^^^^^^^^ Avoid `if` branches without a body.
end
RUBY

expect_correction('')
end

it 'does not register an offense for missing `if` body with a comment' do
Expand All @@ -27,6 +29,64 @@
^^^^^^^^^^^^^^^^^^^^^ Avoid `elsif` branches without a body.
end
RUBY

expect_correction(<<~RUBY)
if condition
do_something
end
RUBY
end

it 'registers an offense for missing `if` body with `else`' do
expect_offense(<<~RUBY)
if condition
^^^^^^^^^^^^ Avoid `if` branches without a body.
else
do_something
end
RUBY

expect_correction(<<~RUBY)
unless condition
do_something
end
RUBY
end

it 'registers an offense for missing `unless` body with `else`' do
expect_offense(<<~RUBY)
unless condition
^^^^^^^^^^^^^^^^ Avoid `unless` branches without a body.
else
do_something
end
RUBY

expect_correction(<<~RUBY)
if condition
do_something
end
RUBY
end

it 'registers an offense for missing `if` body with `elsif`' do
expect_offense(<<~RUBY)
if condition
^^^^^^^^^^^^ Avoid `if` branches without a body.
elsif other_condition
do_something
elsif another_condition
do_something_else
end
RUBY

expect_correction(<<~RUBY)
if other_condition
do_something
elsif another_condition
do_something_else
end
RUBY
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dvandersluis this correction doesn't have the same semantics. To get the same meaning, it should be:

if !condition && other_condition
  do_something
elsif !condition && another_condition
  do_something_else
end

I don't know if this autocorrection was considered safe before, but it definitely isn't safe or correct now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I noticed this after it was merged, and it's been changed to SafeAutoCorrect: false. I haven't had a chance to look at fixing the autocorrection yet so feel free to create a PR!

end

it 'does not register an offense for missing `elsif` body with a comment' do
Expand All @@ -49,6 +109,14 @@
# noop
end
RUBY

expect_correction(<<~RUBY)
if condition
do_something
else
# noop
end
RUBY
end

it 'does not register an offense for missing `elsif` body with an inline comment' do
Expand All @@ -72,6 +140,14 @@
^^^^^^^^^ Avoid `elsif` branches without a body.
end
RUBY

expect_correction(<<~RUBY)
if foo
do_foo
elsif bar
do_bar
end
RUBY
end

it 'registers an offense for missing `unless` body' do
Expand All @@ -80,6 +156,8 @@
^^^^^^^^^^^^^^^^ Avoid `unless` branches without a body.
end
RUBY

expect_correction('')
end

it 'does not register an offense for missing `unless` body with a comment' do
Expand All @@ -90,6 +168,22 @@
RUBY
end

it 'autocorrects properly when the if is assigned to a variable' do
expect_offense(<<~RUBY)
x = if foo
^^^^^^ Avoid `if` branches without a body.
elsif bar
5
end
RUBY

expect_correction(<<~RUBY)
x = if bar
5
end
RUBY
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same, this should be:

x = if !foo && bar
  5
end

end

context 'when AllowComments is false' do
let(:cop_config) { { 'AllowComments' => false } }

Expand All @@ -100,6 +194,8 @@
# noop
end
RUBY

expect_correction('')
end

it 'registers an offense for missing `elsif` body with a comment' do
Expand All @@ -111,6 +207,12 @@
# noop
end
RUBY

expect_correction(<<~RUBY)
if condition
do_something
end
RUBY
end

it 'registers an offense for missing `unless` body with a comment' do
Expand All @@ -120,6 +222,8 @@
# noop
end
RUBY

expect_correction('')
end
end
end