[Fix #8372] Fix Lint/RedundantCopEnableDirective autocorrection not…

… removing empty `# rubocop:enable` comments. (#8745)

Added `RuboCop::DirectiveComment` to encapsulate some logic about determining whether a rubocop directive comment should be removed (if the cops in it are all redundant).

Co-authored-by: Bozhidar Batsov <[email protected]>

CHANGELOG.md

@@ -15,6 +15,7 @@
 * [#8750](https://github.com/rubocop-hq/rubocop/pull/8750): Fix an incorrect auto-correct for `Style/MultilineWhenThen` when line break for multiple condidate values of `when` statement. ([@koic][])
 * [#8754](https://github.com/rubocop-hq/rubocop/pull/8754): Fix an error for `Style/RandomWithOffset` when using a range with non-integer bounds. ([@eugeneius][])
 * [#8756](https://github.com/rubocop-hq/rubocop/issues/8756): Fix an infinite loop error for `Layout/EmptyLinesAroundAccessModifier` with `Layout/EmptyLinesAroundBlockBody` when using access modifier with block argument. ([@koic][])
+* [#8372](https://github.com/rubocop-hq/rubocop/issues/8372): Fix `Lint/RedundantCopEnableDirective` autocorrection to not leave orphaned empty `# rubocop:enable` comments. ([@dvandersluis][])
 * [#8372](https://github.com/rubocop-hq/rubocop/issues/8372): Fix `Lint/RedundantCopDisableDirective` autocorrection. ([@dvandersluis][])
 * [#8764](https://github.com/rubocop-hq/rubocop/issues/8764): Fix `Layout/CaseIndentation` not showing the cop name in output messages. ([@dvandersluis][])
 * [#8771](https://github.com/rubocop-hq/rubocop/issues/8771): Fix an error for `Style/OneLineConditional` when using `if-then-elsif-then-end`. ([@koic][])

lib/rubocop.rb

@@ -614,6 +614,7 @@
 require_relative 'rubocop/config_store'
 require_relative 'rubocop/config_validator'
 require_relative 'rubocop/target_finder'
+require_relative 'rubocop/directive_comment'
 require_relative 'rubocop/comment_config'
 require_relative 'rubocop/magic_comment'
 require_relative 'rubocop/result_cache'

lib/rubocop/comment_config.rb

@@ -41,12 +41,15 @@ def cop_disabled_line_ranges
     end
 
     def extra_enabled_comments
-      extra_enabled_comments_with_names([], {})
+      extra_enabled_comments_with_names(
+        extras: Hash.new { |h, k| h[k] = [] },
+        names: Hash.new(0)
+      )
     end
 
     private
 
-    def extra_enabled_comments_with_names(extras, names)
+    def extra_enabled_comments_with_names(extras:, names:)
       each_directive do |comment, cop_names, disabled|
         next unless comment_only_line?(comment.loc.expression.line)
 
@@ -190,18 +193,19 @@ def handle_enable_all(names, extras, comment)
         enabled_cops += 1
       end
 
-      extras << [comment, 'all'] if enabled_cops.zero?
+      extras[comment] << 'all' if enabled_cops.zero?
     end
 
+    # Collect cops that have been disabled or enabled by name in a directive comment
+    # so that `Lint/RedundantCopEnableDirective` can register offenses correctly.
     def handle_switch(cop_names, names, disabled, extras, comment)
       cop_names.each do |name|
-        names[name] ||= 0
         if disabled
           names[name] += 1
         elsif (names[name]).positive?
           names[name] -= 1
         else
-          extras << [comment, name]
+          extras[comment] << name
         end
       end
     end

lib/rubocop/cop/lint/redundant_cop_enable_directive.rb

@@ -45,18 +45,28 @@ def on_new_investigation
           return if processed_source.blank?
 
           offenses = processed_source.comment_config.extra_enabled_comments
-          offenses.each do |comment, name|
+          offenses.each { |comment, cop_names| register_offense(comment, cop_names) }
+        end
+
+        private
+
+        def register_offense(comment, cop_names)
+          directive = DirectiveComment.new(comment)
+
+          cop_names.each do |name|
             add_offense(
               range_of_offense(comment, name),
               message: format(MSG, cop: all_or_name(name))
             ) do |corrector|
-              corrector.remove(range_with_comma(comment, name))
+              if directive.match?(cop_names)
+                corrector.remove(range_with_surrounding_space(range: directive.range, side: :right))
+              else
+                corrector.remove(range_with_comma(comment, name))
+              end
             end
           end
         end
 
-        private
-
         def range_of_offense(comment, name)
           start_pos = comment_start(comment) + cop_name_indention(comment, name)
           range_between(start_pos, start_pos + name.size)

lib/rubocop/directive_comment.rb

@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+module RuboCop
+  # This class wraps the `Parser::Source::Comment` object that represents a
+  # special `rubocop:disable` and `rubocop:enable` comment and exposes what
+  # cops it contains.
+  class DirectiveComment
+    attr_reader :comment
+
+    def initialize(comment)
+      @comment = comment
+    end
+
+    # Return all the cops specified in the directive
+    def cops
+      match = comment.text.match(CommentConfig::COMMENT_DIRECTIVE_REGEXP)
+      return unless match
+
+      cops_string = match.captures[1]
+      cops_string.split(/,\s*/).uniq.sort
+    end
+
+    # Checks if this directive contains all the given cop names
+    def match?(cop_names)
+      cops == cop_names.uniq.sort
+    end
+
+    def range
+      comment.location.expression
+    end
+  end
+end

spec/rubocop/cli/cli_autocorrect_spec.rb

@@ -677,13 +677,12 @@ def func
       C:  2:  1: [Corrected] Layout/EmptyLineAfterMagicComment: Add an empty line after magic comments.
       C:  3:  1: Style/Documentation: Missing top-level class documentation comment.
       W:  4:  3: [Corrected] Lint/RedundantCopDisableDirective: Unnecessary disabling of Metrics/MethodLength.
-      C:  5:  1: [Corrected] Layout/EmptyLinesAroundMethodBody: Extra empty line detected at method body beginning.
-      C:  5:  1: [Corrected] Layout/TrailingWhitespace: Trailing whitespace detected.
+      C:  5:  3: [Corrected] Layout/IndentationWidth: Use 2 (not 6) spaces for indentation.
       W:  5: 22: [Corrected] Lint/RedundantCopEnableDirective: Unnecessary enabling of Metrics/MethodLength.
       W:  7: 54: [Corrected] Lint/RedundantCopDisableDirective: Unnecessary disabling of Style/For.
       W:  9:  5: [Corrected] Lint/RedundantCopDisableDirective: Unnecessary disabling of Style/ClassVars.
 
-      1 file inspected, 9 offenses detected, 8 offenses corrected
+      1 file inspected, 8 offenses detected, 7 offenses corrected
     RESULT
     corrected = <<~RUBY
       # frozen_string_literal: true

spec/rubocop/cop/lint/redundant_cop_enable_directive_spec.rb

@@ -12,7 +12,6 @@
 
     expect_correction(<<~RUBY)
       foo
-
     RUBY
   end
 
@@ -43,7 +42,6 @@
 
     expect_correction(<<~RUBY)
       foo
-      # rubocop:enable
       bar
     RUBY
   end
@@ -85,7 +83,6 @@
 
       bar
 
-
       bar
     RUBY
   end
@@ -100,7 +97,6 @@
 
       expect_correction(<<~RUBY)
         foo
-
       RUBY
     end
 
@@ -179,4 +175,42 @@
       RUBY
     end
   end
+
+  context 'when all cops are unnecessarily enabled' do
+    context 'on the same line' do
+      it 'registers an offense and corrects' do
+        expect_offense(<<~RUBY)
+          foo
+          # rubocop:enable Layout/LineLength, Metrics/AbcSize, Lint/Debugger
+                                                               ^^^^^^^^^^^^^ Unnecessary enabling of Lint/Debugger.
+                                              ^^^^^^^^^^^^^^^ Unnecessary enabling of Metrics/AbcSize.
+                           ^^^^^^^^^^^^^^^^^ Unnecessary enabling of Layout/LineLength.
+        RUBY
+
+        expect_correction(<<~RUBY)
+          foo
+        RUBY
+      end
+    end
+
+    context 'on separate lines' do
+      it 'registers an offense and corrects when there is extra white space' do
+        expect_offense(<<~RUBY)
+          foo
+          # rubocop:enable Metrics/ModuleSize
+                           ^^^^^^^^^^^^^^^^^^ Unnecessary enabling of Metrics/ModuleSize.
+          # rubocop:enable Layout/LineLength, Metrics/ClassSize
+                           ^^^^^^^^^^^^^^^^^ Unnecessary enabling of Layout/LineLength.
+                                              ^^^^^^^^^^^^^^^^^ Unnecessary enabling of Metrics/ClassSize.
+          # rubocop:enable Metrics/AbcSize, Lint/Debugger
+                                            ^^^^^^^^^^^^^ Unnecessary enabling of Lint/Debugger.
+                           ^^^^^^^^^^^^^^^ Unnecessary enabling of Metrics/AbcSize.
+        RUBY
+
+        expect_correction(<<~RUBY)
+          foo
+        RUBY
+      end
+    end
+  end
 end

spec/rubocop/directive_comment_spec.rb

@@ -0,0 +1,99 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::DirectiveComment do
+  subject(:directive_comment) { described_class.new(comment) }
+
+  let(:comment) { instance_double(Parser::Source::Comment, text: text) }
+  let(:comment_cop_names) { 'all' }
+  let(:text) { "#rubocop:enable #{comment_cop_names}" }
+
+  describe '#cops' do
+    subject(:cops) { directive_comment.cops }
+
+    context 'all' do
+      let(:comment_cop_names) { 'all' }
+
+      it 'returns [all]' do
+        expect(cops).to eq(%w[all])
+      end
+    end
+
+    context 'single cop' do
+      let(:comment_cop_names) { 'Metrics/AbcSize' }
+
+      it 'returns [Metrics/AbcSize]' do
+        expect(cops).to eq(%w[Metrics/AbcSize])
+      end
+    end
+
+    context 'single cop duplicated' do
+      let(:comment_cop_names) { 'Metrics/AbcSize,Metrics/AbcSize' }
+
+      it 'returns [Metrics/AbcSize]' do
+        expect(cops).to eq(%w[Metrics/AbcSize])
+      end
+    end
+
+    context 'multiple cops' do
+      let(:comment_cop_names) { 'Style/Not, Metrics/AbcSize' }
+
+      it 'returns the cops in alphabetical order' do
+        expect(cops).to eq(%w[Metrics/AbcSize Style/Not])
+      end
+    end
+  end
+
+  describe '#match?' do
+    subject(:match) { directive_comment.match?(cop_names) }
+
+    let(:comment_cop_names) { 'Metrics/AbcSize, Metrics/PerceivedComplexity, Style/Not' }
+
+    context 'no comment_cop_names' do
+      let(:cop_names) { [] }
+
+      it 'returns false' do
+        expect(match).to eq(false)
+      end
+    end
+
+    context 'same cop names as in the comment' do
+      let(:cop_names) { %w[Metrics/AbcSize Metrics/PerceivedComplexity Style/Not] }
+
+      it 'returns true' do
+        expect(match).to eq(true)
+      end
+    end
+
+    context 'same cop names as in the comment in a different order' do
+      let(:cop_names) { %w[Style/Not Metrics/AbcSize Metrics/PerceivedComplexity] }
+
+      it 'returns true' do
+        expect(match).to eq(true)
+      end
+    end
+
+    context 'subset of names' do
+      let(:cop_names) { %w[Metrics/AbcSize Style/Not] }
+
+      it 'returns false' do
+        expect(match).to eq(false)
+      end
+    end
+
+    context 'superset of names' do
+      let(:cop_names) { %w[Lint/Void Metrics/AbcSize Metrics/PerceivedComplexity Style/Not] }
+
+      it 'returns false' do
+        expect(match).to eq(false)
+      end
+    end
+
+    context 'duplicate names' do
+      let(:cop_names) { %w[Metrics/AbcSize Metrics/AbcSize Metrics/PerceivedComplexity Style/Not] }
+
+      it 'returns true' do
+        expect(match).to eq(true)
+      end
+    end
+  end
+end