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

[Fix #9244] When a cop defined in an extension is explicitly enabled, ensure that it remains enabled. #9254

Merged
merged 1 commit into from Dec 22, 2020

Conversation

dvandersluis
Copy link
Member

@dvandersluis dvandersluis commented Dec 18, 2020

With DisabledByDefault: true in a config, Enabled: false is automatically added to every key in the default configuration.

In normal operation, the configuration from any config files (including via inheritence) is then merged into that default configuration, so that if a cop is explicitly enabled it overrides the default disable (in ConfigLoaderResolver#merge_with_default). This works even if inherit_from or inherit_gem are specified.

However, extensions inject their configuration in when they are required, and the injection code that extensions use also calls merge_with_default. In that case, the extension's configuration gets merged into rubocop's default, which then becomes the default configuration (note, depending on how many extensions are required by the user, this process may happen multiple times). When that configuration gets the local config merged into it with DisabledByDefault: true, if that extension's configuration had a department config key, it would get Enabled: false set on it. Then, when the specific cop is checked for if it's enabled, the department value would override the local value, and the cop would be disabled.

In order to fix this, cops with Enabled: true in their configuration after all the merging happens are always considered to be enabled, and the department is not checked. To facilitate this, when config files are being merged (either through inheritence or through merge_with_default), if a department is disabled any previously defined cops that are Enabled are flipped to be disabled (as expected, since configurations are meant to override other configurations they inherit from).


Example:

# .rubocop.yml
require:
  - rubocop-rspec
  
AllCops:
  DisabledByDefault: true

RSpec/Focus:
  Enabled: true

Before:

$ rubocop test_spec.rb
Inspecting 1 file
.

1 file inspected, no offenses detected

After:

$ rubocop test_spec.rb
Inspecting 1 file
C

Offenses:

test_spec.rb:31:3: C: RSpec/Focus: Focused spec found.
  fit 'does something' do
  ^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected

Fixes #9244.


Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

@dvandersluis
Copy link
Member Author

This bug is really an artifact of the lack of plugin system, which I have some ideas about how we can get into place. However, given that we need to keep supporting the existing injection system for backwards compatibility, this needed to be fixed regardless.

Comment on lines -602 to +617
# Pending in default config, department disabled in grandparent.
expect(enabled?('Lint/StructNewOverride')).to be(false)
# Pending in grandparent config, department disabled in parent.
expect(enabled?('Naming/FileName')).to be(false)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous test was for a cop that wasn't actually pending anymore, so I updated it to not rely on the default config not changing.

@dvandersluis dvandersluis force-pushed the issue/9244 branch 3 times, most recently from 58b76d2 to 8c76bc3 Compare December 21, 2020 20:20
…nabled, ensure that it remains enabled.

With `DisabledByDefault: true` in a config, `Enabled: false` is automatically added to every key in the default configuration. In normal operation, the configuration from any config files (including via inheritence) is then merged into that default configuration, so that if a cop is explicitly enabled it overrides the default disable (in `ConfigLoaderResolver#merge_with_default`). This works even if `inherit_from` or `inherit_gem` are specified.

However, extensions inject their configuration in when they are required, and the injection code that extensions use also calls `merge_with_default`. In that case, the **extension's** configuration gets merged into rubocop's default, which then becomes the default configuration. When that configuration gets the local config merged into it with `DisabledByDefault: true`, if that extension's configuration had a department config key, it would get `Enabled: false` set on it. Then, when the specific cop is checked for if it's enabled, the department value would override the local value, and the cop would be disabled.

In order to fix this, cops with `Enabled: true` in their configuration after all the merging happens are always considered to be enabled, and the department is not checked. To facilitate this, when config files are being merged (either through inheritence or through `merge_with_default`), if a department is disabled any previously defined cops that are Enabled are flipped to be disabled (as expected, since configurations are meant to override other configurations they inherit from).
@dvandersluis
Copy link
Member Author

I expected the department calculation change in #9258 to affect this. Now that it's merged, I've updated to ensure that nested departments work as well. /cc @mvz @bbatsov

@bbatsov
Copy link
Collaborator

bbatsov commented Dec 22, 2020

This bug is really an artifact of the lack of plugin system, which I have some ideas about how we can get into place.

You're totally right. That's one of the major items we kept discussing for years and never really got to doing. It's nice you're thinking in that direction.

@bbatsov bbatsov merged commit 4b7aa96 into rubocop:master Dec 22, 2020
@dvandersluis dvandersluis deleted the issue/9244 branch January 18, 2021 20:43
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

Successfully merging this pull request may close these issues.

Cannot enable rubocop-rspec derived cops on DisabledByDefault
2 participants