Refactor redundant cop disable directive

lib/rubocop/cop/lint/redundant_cop_disable_directive.rb

@@ -41,12 +41,9 @@ def initialize(config = nil, options = nil, offenses = nil)
         def on_new_investigation
           return unless offenses_to_check
 
-          cop_disabled_line_ranges = processed_source.disabled_line_ranges
-
           redundant_cops = Hash.new { |h, k| h[k] = Set.new }
 
-          each_redundant_disable(cop_disabled_line_ranges,
-                                 offenses_to_check) do |comment, redundant_cop|
+          each_redundant_disable do |comment, redundant_cop|
             redundant_cops[comment].add(redundant_cop)
           end
 
@@ -56,6 +53,14 @@ def on_new_investigation
 
         private
 
+        def cop_disabled_line_ranges
+          processed_source.disabled_line_ranges
+        end
+
+        def disabled_ranges
+          cop_disabled_line_ranges[COP_NAME] || [0..0]
+        end
+
         def previous_line_blank?(range)
           processed_source.buffer.source_line(range.line - 1).blank?
         end
@@ -94,32 +99,32 @@ def directive_range_in_list(range, ranges)
           range_with_surrounding_space(range: range, side: :right, newlines: false)
         end
 
-        def each_redundant_disable(cop_disabled_line_ranges, offenses,
-                                   &block)
-          disabled_ranges = cop_disabled_line_ranges[COP_NAME] || [0..0]
-
+        def each_redundant_disable(&block)
           cop_disabled_line_ranges.each do |cop, line_ranges|
-            each_already_disabled(line_ranges, disabled_ranges) { |comment| yield comment, cop }
-
-            each_line_range(line_ranges, disabled_ranges, offenses, cop, &block)
+            each_already_disabled(cop, line_ranges, &block)
+            each_line_range(cop, line_ranges, &block)
           end
         end
 
-        def each_line_range(line_ranges, disabled_ranges, offenses,
-                            cop)
+        def each_line_range(cop, line_ranges)
           line_ranges.each_with_index do |line_range, ix|
+            next if ignore_offense?(line_range)
+
             comment = processed_source.comment_at_line(line_range.begin)
-            next if ignore_offense?(disabled_ranges, line_range)
+            redundant_cop = if all_disabled?(comment)
+                              find_redundant_all(line_range, line_ranges[ix + 1])
+                            else
+                              find_redundant(cop, line_range)
+                            end
 
-            redundant_cop = find_redundant(comment, offenses, cop, line_range, line_ranges[ix + 1])
             yield comment, redundant_cop if redundant_cop
           end
         end
 
-        def each_already_disabled(line_ranges, disabled_ranges)
+        def each_already_disabled(cop, line_ranges)
           line_ranges.each_cons(2) do |previous_range, range|
-            next if ignore_offense?(disabled_ranges, range)
-            next if previous_range.end != range.begin
+            next if ignore_offense?(range)
+            next unless followed_ranges?(previous_range, range)
 
             # If a cop is disabled in a range that begins on the same line as
             # the end of the previous range, it means that the cop was
@@ -130,42 +135,45 @@ def each_already_disabled(line_ranges, disabled_ranges)
             # Comments disabling all cops don't count since it's reasonable
             # to disable a few select cops first and then all cops further
             # down in the code.
-            yield comment if comment && !all_disabled?(comment)
+            yield comment, cop if comment && !all_disabled?(comment)
           end
         end
 
-        # rubocop:todo Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
-        def find_redundant(comment, offenses, cop, line_range, next_line_range)
-          if all_disabled?(comment)
-            # If there's a disable all comment followed by a comment
-            # specifically disabling `cop`, we don't report the `all`
-            # comment. If the disable all comment is truly redundant, we will
-            # detect that when examining the comments of another cop, and we
-            # get the full line range for the disable all.
-            if (next_line_range.nil? || line_range.last != next_line_range.first) &&
-               offenses.none? { |o| line_range.cover?(o.line) }
-              'all'
-            end
-          else
-            cop_offenses = offenses.select { |o| o.cop_name == cop }
-            cop if cop_offenses.none? { |o| line_range.cover?(o.line) }
-          end
+        def find_redundant(cop, range)
+          cop_offenses = offenses_to_check.select { |offense| offense.cop_name == cop }
+          cop if range_with_offense?(range, cop_offenses)
+        end
+
+        def find_redundant_all(range, next_range)
+          # If there's a disable all comment followed by a comment
+          # specifically disabling `cop`, we don't report the `all`
+          # comment. If the disable all comment is truly redundant, we will
+          # detect that when examining the comments of another cop, and we
+          # get the full line range for the disable all.
+          has_no_next_range = next_range.nil? || !followed_ranges?(range, next_range)
+          'all' if has_no_next_range && range_with_offense?(range)
+        end
+
+        def followed_ranges?(range, next_range)
+          range.end == next_range.begin
+        end
+
+        def range_with_offense?(range, offenses = offenses_to_check)
+          offenses.none? { |offense| range.cover?(offense.line) }
         end
-        # rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
 
         def all_disabled?(comment)
-          /rubocop\s*:\s*(?:disable|todo)\s+all\b/.match?(comment.text)
+          DirectiveComment.new(comment).disabled_all?
         end
 
-        def ignore_offense?(disabled_ranges, line_range)
+        def ignore_offense?(line_range)
           disabled_ranges.any? do |range|
             range.cover?(line_range.min) && range.cover?(line_range.max)
           end
         end
 
         def directive_count(comment)
-          _, cops_string = DirectiveComment.new(comment).match_captures
-          cops_string.split(/,\s*/).size
+          DirectiveComment.new(comment).cop_names.size
         end
 
         def add_offenses(redundant_cops)
@@ -180,12 +188,9 @@ def add_offenses(redundant_cops)
 
         def add_offense_for_entire_comment(comment, cops)
           location = comment.loc.expression
-          cop_list = cops.sort.map { |c| describe(c) }
+          cop_names = cops.sort.map { |c| describe(c) }.join(', ')
 
-          add_offense(
-            location,
-            message: "Unnecessary disabling of #{cop_list.join(', ')}."
-          ) do |corrector|
+          add_offense(location, message: message(cop_names)) do |corrector|
             range = comment_range_with_surrounding_space(location)
             corrector.remove(range)
           end
@@ -197,10 +202,8 @@ def add_offense_for_some_cops(comment, cops)
           ranges = cop_ranges.map { |_, r| r }
 
           cop_ranges.each do |cop, range|
-            add_offense(
-              range,
-              message: "Unnecessary disabling of #{describe(cop)}."
-            ) do |corrector|
+            cop_name = describe(cop)
+            add_offense(range, message: message(cop_name)) do |corrector|
               range = directive_range_in_list(range, ranges)
               corrector.remove(range)
             end
@@ -230,18 +233,15 @@ def trailing_range?(ranges, range)
         end
 
         def describe(cop)
-          if cop == 'all'
-            'all cops'
-          elsif all_cop_names.include?(cop)
-            "`#{cop}`"
-          else
-            similar = NameSimilarity.find_similar_name(cop, all_cop_names)
-            if similar
-              "`#{cop}` (did you mean `#{similar}`?)"
-            else
-              "`#{cop}` (unknown cop)"
-            end
-          end
+          return 'all cops' if cop == 'all'
+          return "`#{cop}`" if all_cop_names.include?(cop)
+
+          similar = NameSimilarity.find_similar_name(cop, all_cop_names)
+          similar ? "`#{cop}` (did you mean `#{similar}`?)" : "`#{cop}` (unknown cop)"
+        end
+
+        def message(cop_names)
+          "Unnecessary disabling of #{cop_names}."
         end
 
         def all_cop_names

lib/rubocop/directive_comment.rb

@@ -64,6 +64,11 @@ def enabled_all?
       !disabled? && all_cops?
     end
 
+    # Checks if this directive disables all cops
+    def disabled_all?
+      disabled? && all_cops?
+    end
+
     # Checks if all cops specified in this directive
     def all_cops?
       cops == 'all'

spec/rubocop/directive_comment_spec.rb

@@ -214,4 +214,32 @@
       end
     end
   end
+
+  describe '#disabled_all?' do
+    subject { directive_comment.disabled_all? }
+
+    context 'when enabled all cops' do
+      let(:text) { 'def foo # rubocop:enable all' }
+
+      it { is_expected.to eq false }
+    end
+
+    context 'when enabled specific cops' do
+      let(:text) { '# rubocop:enable Foo/Bar' }
+
+      it { is_expected.to eq false }
+    end
+
+    context 'when disabled all cops' do
+      let(:text) { '# rubocop:disable all' }
+
+      it { is_expected.to eq true }
+    end
+
+    context 'when disabled specific cops' do
+      let(:text) { '# rubocop:disable Foo/Bar' }
+
+      it { is_expected.to eq false }
+    end
+  end
 end