Skip to content

Commit

Permalink
[Fix #7999] Consider only the comments relevant for the node being ch…
Browse files Browse the repository at this point in the history
…ecked
  • Loading branch information
Darhazer committed Mar 30, 2022
1 parent bd7a65f commit c1788be
Show file tree
Hide file tree
Showing 8 changed files with 89 additions and 5 deletions.
@@ -0,0 +1 @@
* [#10491](https://github.com/rubocop/rubocop/pull/10491): Improve the handling of comments in `Lint/EmptyConditionalBody`, `Lint/EmptyInPattern` and `Lint/EmptyWhen` when `AllowComments` is set to `true`. ([@Darhazer][])
4 changes: 3 additions & 1 deletion lib/rubocop/cop/lint/empty_conditional_body.rb
Expand Up @@ -53,11 +53,13 @@ module Lint
# end
#
class EmptyConditionalBody < Base
include CommentsHelp

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

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

add_offense(node, message: format(MSG, keyword: node.keyword))
end
Expand Down
4 changes: 3 additions & 1 deletion lib/rubocop/cop/lint/empty_in_pattern.rb
Expand Up @@ -44,14 +44,16 @@ module Lint
#
class EmptyInPattern < Base
extend TargetRubyVersion
include CommentsHelp

MSG = 'Avoid `in` branches without a body.'

minimum_target_ruby_version 2.7

def on_case_match(node)
node.in_pattern_branches.each do |branch|
next if branch.body || (cop_config['AllowComments'] && comment_lines?(node))
next if branch.body
next if cop_config['AllowComments'] && contains_comments?(branch)

add_offense(branch)
end
Expand Down
4 changes: 3 additions & 1 deletion lib/rubocop/cop/lint/empty_when.rb
Expand Up @@ -45,12 +45,14 @@ module Lint
# end
#
class EmptyWhen < Base
include CommentsHelp

MSG = 'Avoid `when` branches without a body.'

def on_case(node)
node.each_when do |when_node|
next if when_node.body
next if cop_config['AllowComments'] && comment_lines?(node)
next if cop_config['AllowComments'] && contains_comments?(when_node)

add_offense(when_node)
end
Expand Down
24 changes: 22 additions & 2 deletions lib/rubocop/cop/mixin/comments_help.rb
Expand Up @@ -4,15 +4,20 @@ module RuboCop
module Cop
# Help methods for working with nodes containing comments.
module CommentsHelp
include VisibilityHelp

def source_range_with_comment(node)
begin_pos = begin_pos_with_comment(node)
end_pos = end_position_for(node)

Parser::Source::Range.new(buffer, begin_pos, end_pos)
end

def contains_comments?(node)
start_line = node.source_range.line
end_line = find_end_line(node)

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

private

def end_position_for(node)
Expand All @@ -37,6 +42,21 @@ def start_line_position(node)
def buffer
processed_source.buffer
end

# Returns the end line of a node, which might be a comment and not part of the AST
# End line is considered either the line at which another node starts, or
# the line at which the parent node ends.
def find_end_line(node)
if node.if_type? && node.loc.else
node.loc.else.line
elsif (next_sibling = node.right_sibling)
next_sibling.loc.line
elsif (parent = node.parent)
parent.loc.end.line
else
node.loc.end.line
end
end
end
end
end
23 changes: 23 additions & 0 deletions spec/rubocop/cop/lint/empty_conditional_body_spec.rb
Expand Up @@ -39,6 +39,29 @@
RUBY
end

it 'registers an offense for missing `elsif` body that is not the one with a comment' do
expect_offense(<<~RUBY)
if condition
do_something
elsif other_condition
^^^^^^^^^^^^^^^^^^^^^ Avoid `elsif` branches without a body.
else
# noop
end
RUBY
end

it 'does not register an offense for missing `elsif` body with an inline comment' do
expect_no_offenses(<<~RUBY)
if condition
do_something
elsif other_condition # no op, but avoid going into the else
else
do_other_things
end
RUBY
end

it 'registers an offense for missing `unless` body' do
expect_offense(<<~RUBY)
unless condition
Expand Down
11 changes: 11 additions & 0 deletions spec/rubocop/cop/lint/empty_in_pattern_spec.rb
Expand Up @@ -133,6 +133,17 @@
context 'when `AllowComments: true`', :ruby27 do
let(:cop_config) { { 'AllowComments' => true } }

it 'registers an offense for empty `in` when comment is in another branch' do
expect_offense(<<~RUBY)
case condition
in [a]
^^^^^^ Avoid `in` branches without a body.
in [a, b]
# do nothing
end
RUBY
end

it 'accepts an empty `in` body with a comment' do
expect_no_offenses(<<~RUBY)
case condition
Expand Down
23 changes: 23 additions & 0 deletions spec/rubocop/cop/lint/empty_when_spec.rb
Expand Up @@ -172,6 +172,29 @@
end
RUBY
end

it 'registers an offense for missing when body without a comment' do
expect_offense(<<~RUBY)
case condition
when foo
42 # magic number
when bar
^^^^^^^^ Avoid `when` branches without a body.
when baz # more comments mixed
21 # another magic number
end
RUBY
end

it 'accepts an empty when ... then body with a comment' do
expect_no_offenses(<<~RUBY)
case condition
when foo
do_something
when bar then # do nothing
end
RUBY
end
end

context 'when `AllowComments: false`' do
Expand Down

0 comments on commit c1788be

Please sign in to comment.