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

Lint/EmptyConditionalBody does not allow commented empty body in 2nd elsif #11820

Closed
r7kamura opened this issue Apr 27, 2023 · 1 comment
Closed

Comments

@r7kamura
Copy link
Contributor

Expected behavior

In Lint/EmptyConditionalBody, when AllowComments: true, no offense should be detected from the following code:

if condition1
  do_something1
elsif condition2
  do_something2
elsif condition3
  # noop
end

Actual behavior

Lint/EmptyConditionalBody detects an offense like this:

elsif condition2
  do_something2
elsif condition3
+^^^^^^^^^^^^^^^^ Avoid `elsif` branches without a body.
  # noop
end

Steps to reproduce the problem

This behavior can be reproduced by applying the following patch:

diff --git a/spec/rubocop/cop/lint/empty_conditional_body_spec.rb b/spec/rubocop/cop/lint/empty_conditional_body_spec.rb
index edd5cda13..8ceea38f8 100644
--- a/spec/rubocop/cop/lint/empty_conditional_body_spec.rb
+++ b/spec/rubocop/cop/lint/empty_conditional_body_spec.rb
@@ -75,6 +75,18 @@ RSpec.describe RuboCop::Cop::Lint::EmptyConditionalBody, :config do
     RUBY
   end
 
+  it 'does not register an offense for missing 2nd `elsif` body with a comment' do
+    expect_no_offenses(<<~'RUBY')
+      if condition1
+        do_something1
+      elsif condition2
+        do_something2
+      elsif condition3
+        # noop
+      end
+    RUBY
+  end
+
   it 'registers an offense for missing `elsif` body' do
     expect_offense(<<~RUBY)
       if condition

RuboCop version

$ bundle exec rubocop -V
1.50.2 (using Parser 3.2.2.1, rubocop-ast 1.28.0, running on ruby 3.1.3) [x86_64-linux]
  - rubocop-performance 1.17.1
  - rubocop-rake 0.6.0
  - rubocop-rspec 2.20.0
@r7kamura
Copy link
Contributor Author

I think the reason for this bug is that the parent of the second elsif is the first elsif, and this first elsif does not have location.end, so it fails to compute the line number and consequently does not recognize the comment.

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)
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.
# rubocop:disable Metrics/AbcSize
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 ? parent.loc.end.line : parent.loc.line
else
node.loc.end.line
end
end
# rubocop:enable Metrics/AbcSize
end

@koic koic closed this as completed in 4aa6989 Apr 28, 2023
koic added a commit that referenced this issue Apr 28, 2023
[Fix #11820] Fix `Lint/EmptyConditionalBody` false-positives for commented empty `elsif` body
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant