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

Style/LambdaCall skips subsequent checks after finding a violation #9225

Closed
sswander opened this issue Dec 14, 2020 · 2 comments
Closed

Style/LambdaCall skips subsequent checks after finding a violation #9225

sswander opened this issue Dec 14, 2020 · 2 comments

Comments

@sswander
Copy link
Contributor

My colleague @kelvinngsl and I might have found an unexpected behaviour in Style/LambdaCall. It might also be related to Style/LambdaCall's implementation of ConfigurableEnforcedStyle mixin.

With EnforcedStyle: braces, when the first blk.call is found, it is reported as an offense, but all subsequent code that violates the braces code style got skipped.

With EnforcedStyle: call, when the first blk.() is found, it is reported as an offense, but all subsequent code that violates the braces code style got skipped.

I have included example and steps to reproduce in the section below.

Some finding that might be helpful:

  • Looking at the source code, I think this is related to the use of opposite_style_detected in lib/rubocop/cop/style/lambda_call.rb. Once an opposite style is detected, the ConfigurableEnforcedStyle mixin turned the config into { 'Enabled' => false }, thus ignoring any subsequent violations.

If this is indeed an issue, I am willing to give fixing it a try. However I'm not really sure where to start, so some pointer would be appreciated 🙇



Expected behavior

RuboCop should detect all blk.call violations when EnforcedStyle: braces. Conversely, RuboCop should detect all blk.() violations when EnforcedStyle: call.

Actual behavior

It seems that once RuboCop found the first violation, the rest of the violations get ignored, so not all of the violations end up getting reported.

Steps to reproduce the problem

The following example illustrates an exaggeration of the unexpected behaviour by adding an inline disable. The root cause should be as reported above, which is RuboCop ignoring subsequent violations.

The following example uses EnforcedStyle: braces. However, the same behaviour also happens with EnforcedStyle: call and when the codebase uses the opposite pattern.

rubocop.yml:

Style/LambdaCall:
  EnforcedStyle: braces

test.rb:

def test1(&blk)
  blk.()
end

def test2(&blk)
  blk.call # rubocop:disable Style/LambdaCall
end

def test3(&blk)
  blk.call
end

def test4(&blk)
  blk.call
end

RuboCop result:

$ bundle exec rubocop test.rb

Inspecting 1 file
.

1 file inspected, no offenses detected

RuboCop version

$ bundle exec rubocop -V
1.5.2 (using Parser 2.7.2.0, rubocop-ast 1.3.0, running on ruby 2.6.6 x86_64-darwin18)
@fatkodima
Copy link
Contributor

@sswander You should move opposite_style_detected into add_offense
https://github.com/rubocop-hq/rubocop/blob/b01dcfc27db16010707e8e2d4c5627612c6e21b7/lib/rubocop/cop/style/lambda_call.rb#L30-L36

as

if offense?(node)
  add_offense(node) do |corrector|
    opposite_style_detected
    autocorrect(corrector, node)
  end
else
  correct_style_detected
end

and add a test case with producing 2 violations at the same time.

@sswander
Copy link
Contributor Author

@fatkodima nice, thanks for the pointer! I'll give this a try 👍

sswander added a commit to sswander/rubocop that referenced this issue Dec 18, 2020
- The Cop used to ignore all violations after encountering an opposite
  style because during the first encounter, we set no_acceptable_style!
  and in the next iterations we early returned nil for
  opposite_style_detected check. This caused the Cop to never got
  around to adding the offense
- This fixed the behaviour by letting the Cop add the offense first
  before checking for opposite_style_detected
- Specs updated to illustrate the multiple opposite style scenario
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

2 participants