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

--only and ambiguous cop name #9235

Closed
zverok opened this issue Dec 15, 2020 · 4 comments · Fixed by #9241
Closed

--only and ambiguous cop name #9235

zverok opened this issue Dec 15, 2020 · 4 comments · Fixed by #9241
Labels

Comments

@zverok
Copy link
Contributor

zverok commented Dec 15, 2020

Versions: rubocop 1.6.1, rubocop-rspec 2.0.1
This file:

RSpec.describe 'something' do
  let(:field_X) { 1 }
end

...produces (beside others) this offense:

spec/foo_spec.rb:2:7: C: RSpec/VariableName: Use snake_case for variable names.
  let(:field_X) { 1 }
      ^^^^^^^^

...but, when I am trying to use --only RSpec/VariableName (actually, on a large project, checking each cop's impact):

$ rubocop --only RSpec/VariableName spec/
--only option: RSpec/VariableName has the wrong namespace - should be Naming
Inspecting 1 file
.

1 file inspected, no offenses detected

(Notice the warning about namespace)

Shouldn't be this way, I believe :)

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 15, 2020

@dvandersluis can you look into this?

@bbatsov bbatsov added the bug label Dec 15, 2020
@dvandersluis
Copy link
Member

Sure I'll take a look tomorrow (sorry didn't get a notification about this earlier for some reason).

@dvandersluis
Copy link
Member

dvandersluis commented Dec 16, 2020

So some context:

$ rubocop --only RSpec/VariableName spec/ --debug

--only option: RSpec/VariableName has the wrong namespace - should be Naming
For /Users/daniel/RubymineProjects/rubocop: configuration from /Users/daniel/RubymineProjects/rubocop/.rubocop.yml
configuration from /Users/daniel/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/rubocop-performance-1.9.1/config/default.yml
configuration from /Users/daniel/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/rubocop-performance-1.9.1/config/default.yml
Default configuration from /Users/daniel/RubymineProjects/rubocop/config/default.yml
configuration from /Users/daniel/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/rubocop-rspec-2.0.1/config/default.yml
configuration from /Users/daniel/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/rubocop-rspec-2.0.1/config/default.yml
Inheriting configuration from /Users/daniel/RubymineProjects/rubocop/.rubocop_todo.yml
Inspecting 555 files

What's happening here is:

  1. --only triggers Cop::Registry.qualified_cop_name to map the cop names given on the command line to actual cops: https://github.com/rubocop-hq/rubocop/blob/2a4f895dd7eb29246e18f61cb4e3f493cc2005d8/lib/rubocop/options.rb#L100
  2. qualified_cop_name tries to match the name against the known registry. If a badge is not found, it accepts it (crucial here). https://github.com/rubocop-hq/rubocop/blob/7989d3ec1f65b9d2a73589cc956d6858742a47d5/lib/rubocop/cop/registry.rb#L106-L114
  3. If there is a badge found it resolves it. This is where we're failing. https://github.com/rubocop-hq/rubocop/blob/7989d3ec1f65b9d2a73589cc956d6858742a47d5/lib/rubocop/cop/registry.rb#L270-L278

As you can see from the output above, this check happens before the rubocop-rspec configuration is injected (error message is on line 1 but rubocop-rspec is not loaded until line 6). Therefore, when the Registry tries to find a qualified cop name, it can only find Naming/VariableName (because RSpec/VariableName has not yet been loaded). Then the error message is shown because there's a department mismatch.

So in this case, the error is due to there being a name conflict between cops in different libraries. --only RSpec/Focus for example would work fine, because there is no core RuboCop cop named Focus, so step 2 above will accept the missing badge. Since there is a conflict here, a badge is found, hence the error.

I think the best solution here would be to resolve requires earlier so that the registry is full, I'm going to look into how we can achieve that.

dvandersluis added a commit to dvandersluis/rubocop that referenced this issue Dec 16, 2020
…ly qualify cops added by require.

Previously, cop names passed to `--only` or `--except` were qualified immediately, but because the configuration is not yet loaded when `Options.parse` is called, it could not properly resolve cops that are added by an extension or a local `require`. This was generally not a problem because unresolved cops were accepted, but in the case of a cop that shares a name with a default cop, this resulted in raising a warning that the department name was wrong, and in fact loading the wrong cop.

Instead, qualifying cop names is now done lazily, so that they are only qualified right before they are needed. This ensures that the full registry is present and external cops can be properly detected.
bbatsov pushed a commit that referenced this issue Dec 17, 2020
…ify cops added by require.

Previously, cop names passed to `--only` or `--except` were qualified immediately, but because the configuration is not yet loaded when `Options.parse` is called, it could not properly resolve cops that are added by an extension or a local `require`. This was generally not a problem because unresolved cops were accepted, but in the case of a cop that shares a name with a default cop, this resulted in raising a warning that the department name was wrong, and in fact loading the wrong cop.

Instead, qualifying cop names is now done lazily, so that they are only qualified right before they are needed. This ensures that the full registry is present and external cops can be properly detected.
@bquorning
Copy link
Contributor

FYI @pirj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants