[Fix #9244] When a cop defined in an extension is explicitly enabled,…

… 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).

changelog/fix_when_a_cop_defined_in_an_extension_is.md

@@ -0,0 +1 @@
+* [#9244](https://github.com/rubocop-hq/rubocop/issues/9244): When a cop defined in an extension is explicitly enabled, ensure that it remains enabled. ([@dvandersluis][])

lib/rubocop/config.rb

@@ -50,8 +50,8 @@ def check
       self
     end
 
-    def_delegators :@hash, :[], :[]=, :delete, :each, :key?, :keys, :each_key,
-                   :fetch, :map, :merge, :to_h, :to_hash, :transform_values
+    def_delegators :@hash, :[], :[]=, :delete, :dig, :each, :key?, :keys, :each_key,
+                   :fetch, :map, :merge, :replace, :to_h, :to_hash, :transform_values
     def_delegators :@validator, :validate, :target_ruby_version
 
     def to_s
@@ -281,6 +281,9 @@ def read_rails_version_from_bundler_lock_file
     end
 
     def enable_cop?(qualified_cop_name, cop_options)
+      # If the cop is explicitly enabled, the other checks can be skipped.
+      return true if cop_options['Enabled'] == true
+
       department = department_of(qualified_cop_name)
       cop_enabled = cop_options.fetch('Enabled') do
         !for_all_cops['DisabledByDefault']

lib/rubocop/config_loader_resolver.rb

@@ -20,12 +20,13 @@ def resolve_requires(path, hash)
       end
     end
 
-    # rubocop:disable Metrics/MethodLength
-    def resolve_inheritance(path, hash, file, debug)
+    def resolve_inheritance(path, hash, file, debug) # rubocop:disable Metrics/MethodLength, Metrics/AbcSize
       inherited_files = Array(hash['inherit_from'])
       base_configs(path, inherited_files, file)
         .reverse.each_with_index do |base_config, index|
         override_department_setting_for_cops(base_config, hash)
+        override_enabled_for_disabled_departments(base_config, hash)
+
         base_config.each do |k, v|
           next unless v.is_a?(Hash)
 
@@ -39,7 +40,6 @@ def resolve_inheritance(path, hash, file, debug)
         end
       end
     end
-    # rubocop:enable Metrics/MethodLength
 
     def resolve_inheritance_from_gems(hash)
       gems = hash.delete('inherit_gem')
@@ -75,6 +75,7 @@ def merge_with_default(config, config_file, unset_nil:)
       end
 
       config = handle_disabled_by_default(config, default_configuration) if disabled_by_default
+      override_enabled_for_disabled_departments(default_configuration, config)
 
       opts = { inherit_mode: config['inherit_mode'] || {},
                unset_nil: unset_nil }
@@ -122,10 +123,26 @@ def override_department_setting_for_cops(base_hash, derived_hash)
       end
     end
 
+    # If a cop was previously explicitly enabled, but then superseded by the
+    # department being disabled, disable it.
+    def override_enabled_for_disabled_departments(base_hash, derived_hash)
+      cops_to_disable = derived_hash.each_key.with_object([]) do |key, cops|
+        next unless disabled?(derived_hash, key)
+
+        cops.concat(base_hash.keys.grep(Regexp.new("^#{key}/")))
+      end
+
+      cops_to_disable.each do |cop_name|
+        next unless base_hash.dig(cop_name, 'Enabled') == true
+
+        derived_hash.replace(merge({ cop_name => { 'Enabled' => false } }, derived_hash))
+      end
+    end
+
     private
 
     def disabled?(hash, department)
-      hash[department] && hash[department]['Enabled'] == false
+      hash[department].is_a?(Hash) && hash[department]['Enabled'] == false
     end
 
     def duplicate_setting?(base_hash, derived_hash, key, inherited_file)

spec/rubocop/config_loader_spec.rb

@@ -535,13 +535,18 @@
       end
     end
 
-    context 'when a department is disabled' do
+    context 'when a department is disabled', :restore_registry do
       let(:file_path) { '.rubocop.yml' }
 
       shared_examples 'resolves enabled/disabled for all cops' do |enabled_by_default, disabled_by_default|
+        before { stub_cop_class('RuboCop::Cop::Foo::Bar::Baz') }
+
         it "handles EnabledByDefault: #{enabled_by_default}, " \
            "DisabledByDefault: #{disabled_by_default}" do
           create_file('grandparent_rubocop.yml', <<~YAML)
+            Naming/FileName:
+              Enabled: pending
+
             Metrics/AbcSize:
               Enabled: true
 
@@ -550,6 +555,9 @@
 
             Lint:
               Enabled: false
+
+            Foo/Bar/Baz:
+              Enabled: true
           YAML
           create_file('parent_rubocop.yml', <<~YAML)
             inherit_from: grandparent_rubocop.yml
@@ -559,6 +567,12 @@
 
             Metrics/AbcSize:
               Enabled: false
+
+            Naming:
+              Enabled: false
+
+            Foo/Bar:
+              Enabled: false
           YAML
           create_file(file_path, <<~YAML)
             inherit_from: parent_rubocop.yml
@@ -599,8 +613,8 @@ def enabled?(cop)
           # Enabled in grandparent config, department disabled in parent.
           expect(enabled?('Metrics/PerceivedComplexity')).to be(false)
 
-          # 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)
 
           # Department disabled in child config.
           expect(enabled?('Style/Alias')).to be(false)
@@ -611,6 +625,15 @@ def enabled?(cop)
           # Department disabled in grandparent, cop enabled in child config.
           expect(enabled?('Lint/RaiseException')).to be(true)
 
+          # Cop enabled in grandparent, nested department disabled in parent.
+          expect(enabled?('Foo/Bar/Baz')).to be(false)
+
+          # Cop with similar prefix to disabled nested department.
+          expect(enabled?('Foo/BarBaz')).to eq(!disabled_by_default)
+
+          # Cop enabled in default config, department disabled in grandparent.
+          expect(enabled?('Lint/StructNewOverride')).to be(false)
+
           # Cop enabled in default config, but not mentioned in user config.
           expect(enabled?('Bundler/DuplicatedGem')).to eq(!disabled_by_default)
         end
@@ -1235,6 +1258,98 @@ def cop_enabled?(cop_class)
         end
       end
     end
+
+    context 'when a department is configured without an Enable value specified', :restore_registry do
+      let(:file_path) { '.rubocop.yml' }
+
+      before do
+        create_file('third_party/default.yml', <<~YAML)
+          Custom:
+            Foo: Bar
+        YAML
+
+        stub_cop_class('RuboCop::Cop::Custom::Cop')
+      end
+
+      def cop_enabled?(cop_class)
+        configuration_from_file.for_cop(cop_class).fetch('Enabled')
+      end
+
+      context 'inline' do
+        before do
+          create_file('.rubocop.yml', <<~YAML)
+            AllCops:
+              DisabledByDefault: true
+
+            Custom:
+              Foo: Bar
+
+            Custom/Cop:
+              Enabled: true
+          YAML
+        end
+
+        it 'enables the cop' do
+          expect(cop_enabled?('Custom/Cop')).to be true
+        end
+      end
+
+      context 'via inherit_from' do
+        before do
+          create_file('.rubocop.yml', <<~YAML)
+            inherit_from:
+              - 'third_party/default.yml'
+
+            AllCops:
+              DisabledByDefault: true
+
+            Custom/Cop:
+              Enabled: true
+          YAML
+        end
+
+        it 'enables the cop' do
+          expect(cop_enabled?('Custom/Cop')).to be true
+        end
+      end
+
+      context 'by an extension' do
+        before do
+          create_file('third_party.rb', <<~RUBY)
+            module RuboCop
+              module Custom
+                def self.inject!
+                  path = 'third_party/default.yml'
+
+                  # Injection code currently used in extensions
+                  hash = ConfigLoader.send(:load_yaml_configuration, path)
+                  config = Config.new(hash, path)
+                  config = ConfigLoader.merge_with_default(config, path)
+                  ConfigLoader.instance_variable_set(:@default_configuration, config)
+                end
+              end
+            end
+
+            RuboCop::Custom.inject!
+          RUBY
+
+          create_file('.rubocop.yml', <<~YAML)
+            require:
+              - ./third_party.rb
+
+            AllCops:
+              DisabledByDefault: true
+
+            Custom/Cop:
+              Enabled: true
+          YAML
+        end
+
+        it 'enables the cop' do
+          expect(cop_enabled?('Custom/Cop')).to be true
+        end
+      end
+    end
   end
 
   describe '.load_file', :isolated_environment do

spec/rubocop/config_spec.rb

@@ -774,9 +774,9 @@ def cop_enabled(cop_class)
           }
         end
 
-        it 'still disables the cop' do
+        it 'the cop setting overrides the department' do
           cop_class = RuboCop::Cop::Layout::TrailingWhitespace
-          expect(cop_enabled(cop_class)).to be false
+          expect(cop_enabled(cop_class)).to be true
         end
       end
     end
@@ -790,7 +790,20 @@ def cop_enabled(cop_class)
           }
         end
 
-        it 'still disables the cop' do
+        it 'the cop setting overrides the department' do
+          cop_class = 'Foo/Bar/BazCop'
+          expect(cop_enabled(cop_class)).to be true
+        end
+      end
+
+      context 'and an individual cop is not specified' do
+        let(:hash) do
+          {
+            'Foo/Bar' => { 'Enabled' => false }
+          }
+        end
+
+        it 'the cop setting overrides the department' do
           cop_class = 'Foo/Bar/BazCop'
           expect(cop_enabled(cop_class)).to be false
         end