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.

Sign up for GitHub

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

Jump to bottom

Confusing behavior when using inline comment to disable Lint/EmptyBlock #8971

Closed
mvz opened this issue Oct 30, 2020 · 0 comments · Fixed by #8972
Closed

Confusing behavior when using inline comment to disable Lint/EmptyBlock #8971

mvz opened this issue Oct 30, 2020 · 0 comments · Fixed by #8972
Labels
bug

Comments

@mvz
Copy link
Contributor

mvz commented Oct 30, 2020

I have the following test in one of my projects:

  def test_empty_combinator
    assert_equal nil, combinator {}.call
  end

This triggers Lint/EmptyBlock. If I try to disable this using the regular method of disabling cops in code, the result is surprising. I use a comment like so:

  def test_empty_combinator
    assert_equal nil, combinator {}.call # rubocop:disable Lint/EmptyBlock
  end

Expected behavior

The offense goes away and there is are no more offenses on this line

Actual behavior

RuboCop complains that disabling the cop is not necessary:

For /home/matijs/Projects/iba: configuration from /home/matijs/Projects/iba/.rubocop.yml
configuration from /home/matijs/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rubocop-performance-1.8.1/config/default.yml
configuration from /home/matijs/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rubocop-performance-1.8.1/config/default.yml
Default configuration from /home/matijs/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/rubocop-1.1.0/config/default.yml
Inheriting configuration from /home/matijs/Projects/iba/.rubocop_todo.yml
Inspecting 1 file
Scanning /home/matijs/Projects/iba/test/call_test.rb
For /home/matijs/Projects/iba/test: configuration from /home/matijs/Projects/iba/test/.rubocop.yml
Inheriting configuration from /home/matijs/Projects/iba/.rubocop.yml
Inheriting configuration from /home/matijs/Projects/iba/.rubocop_todo.yml
AllCops/Exclude configuration from /home/matijs/Projects/iba/.rubocop.yml
Inheriting configuration from /home/matijs/Projects/iba/.rubocop_todo.yml
W

Offenses:

test/call_test.rb:8:42: W: Lint/RedundantCopDisableDirective: Unnecessary disabling of Lint/EmptyBlock.
    assert_equal nil, combinator {}.call # rubocop:disable Lint/EmptyBlock
                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense auto-correctable

Analysis

I think what happens is that Lint/EmptyBlock does not register an offense if there is some comment associated with the empty block. So the mere presence of the comment already causes the offense to go away. The RedundantCopDisableDirective cop then kicks in because no EmptyBlock offense is found anymore.

Steps to reproduce the problem

Create a file with the code above and run RuboCop 1.1.0 on it.

RuboCop version

$ [bundle exec] rubocop -V
1.1.0 (using Parser 2.7.2.0, rubocop-ast 1.1.0, running on ruby 2.7.1 x86_64-linux)
  - rubocop-performance 1.8.1
@koic koic added the bug label Oct 30, 2020
@koic koic mentioned this issue Oct 30, 2020
Merged
8 tasks
koic added a commit to koic/rubocop that referenced this issue Oct 30, 2020
@koic
[Fix rubocop#8971] Fix a false alarm for `# rubocop:disable Lint/Empt…
778cb31 
…yBlock`

Fixes rubocop#8971.

This PR fixes the following false alarm for `# rubocop:disable Lint/EmptyBlock`
inline comment with `Lint/RedundantCopDisableDirective`.

```ruby
% cat example.rb
# frozen_string_literal: true

assert_equal nil, combinator {}.call # rubocop:disable Lint/EmptyBlock

% bundle exec rubocop example.rb
Inspecting 1 file
W

Offenses:

example.rb:3:38: W: Lint/RedundantCopDisableDirective: Unnecessary
disabling of Lint/EmptyBlock.
assert_equal nil, combinator {}.call # rubocop:disable Lint/EmptyBlock
                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense auto-correctabl
```

`Lint/EmptyBlock` is `AllowComments: true` by default.
`Lint/RedundantCopDisableDirective` was occurring because
`# rubocop:disable Lint/EmptyBlock` inline comment was treated as
an allowed comment by `Lint/EmptyBlock`.

This PR does not treat `# rubocop:disable` as a comment allowed by `AllowComment`.
bbatsov pushed a commit that referenced this issue Oct 30, 2020
@koic @bbatsov
[Fix #8971] Fix a false alarm for # rubocop:disable Lint/EmptyBlock
5027e4a 
Fixes #8971.

This PR fixes the following false alarm for `# rubocop:disable Lint/EmptyBlock`
inline comment with `Lint/RedundantCopDisableDirective`.

```ruby
% cat example.rb
# frozen_string_literal: true

assert_equal nil, combinator {}.call # rubocop:disable Lint/EmptyBlock

% bundle exec rubocop example.rb
Inspecting 1 file
W

Offenses:

example.rb:3:38: W: Lint/RedundantCopDisableDirective: Unnecessary
disabling of Lint/EmptyBlock.
assert_equal nil, combinator {}.call # rubocop:disable Lint/EmptyBlock
                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense auto-correctabl
```

`Lint/EmptyBlock` is `AllowComments: true` by default.
`Lint/RedundantCopDisableDirective` was occurring because
`# rubocop:disable Lint/EmptyBlock` inline comment was treated as
an allowed comment by `Lint/EmptyBlock`.

This PR does not treat `# rubocop:disable` as a comment allowed by `AllowComment`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Fix #8971] Fix a false alarm for # rubocop:disable Lint/EmptyBlock koic/rubocop
2 participants
@mvz @koic