diff --git a/changelog/fix_when_a_cop_defined_in_an_extension_is.md b/changelog/fix_when_a_cop_defined_in_an_extension_is.md new file mode 100644 index 000000000000..0fd43935e193 --- /dev/null +++ b/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][]) diff --git a/lib/rubocop/config.rb b/lib/rubocop/config.rb index d02dcd6956f4..a4ab5b98f5d2 100644 --- a/lib/rubocop/config.rb +++ b/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'] diff --git a/lib/rubocop/config_loader_resolver.rb b/lib/rubocop/config_loader_resolver.rb index 4359669bf8b0..2abeb95f7b77 100644 --- a/lib/rubocop/config_loader_resolver.rb +++ b/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) diff --git a/spec/rubocop/config_loader_spec.rb b/spec/rubocop/config_loader_spec.rb index 050eaa9e012f..093b32683dff 100644 --- a/spec/rubocop/config_loader_spec.rb +++ b/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 diff --git a/spec/rubocop/config_spec.rb b/spec/rubocop/config_spec.rb index b9b541ccdb27..756dd2326c29 100644 --- a/spec/rubocop/config_spec.rb +++ b/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