Unneeded disable directive

spec/ameba/inline_comments_spec.cr

@@ -83,5 +83,22 @@ module Ameba
       s.error(NamedRule.new, 3, 12, "")
       s.should_not be_valid
     end
+
+    it "does not disable if that is a commented out directive" do
+      s = Source.new %Q(
+        # # ameba:disable #{NamedRule.name}
+        Time.epoch(1483859302)
+      )
+      s.error(NamedRule.new, 3, 12, "")
+      s.should_not be_valid
+    end
+
+    it "does not disable if that is an inline commented out directive" do
+      s = Source.new %Q(
+        a = 1 # Disable it: # ameba:disable #{NamedRule.name}
+      )
+      s.error(NamedRule.new, 2, 12, "")
+      s.should_not be_valid
+    end
   end
 end

spec/ameba/rule/unneded_disable_directive_spec.cr

@@ -0,0 +1,80 @@
+require "../../spec_helper"
+
+module Ameba::Rule
+  describe UnneededDisableDirective do
+    subject = UnneededDisableDirective.new
+
+    it "passes if there are no comments" do
+      s = Source.new %(
+        a = 1
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "passes if there is disable directive" do
+      s = Source.new %(
+        a = 1 # my super var
+      )
+      subject.catch(s).should be_valid
+    end
+
+    it "passes if there is disable directive and it is needed" do
+      s = Source.new %Q(
+        a = 1 # ameba:disable #{NamedRule.name}
+      )
+      s.error NamedRule.new, 2, 1, "Alarm!", :disabled
+      subject.catch(s).should be_valid
+    end
+
+    it "ignores commented out disable directive" do
+      s = Source.new %Q(
+        # # ameba:disable #{NamedRule.name}
+        a = 1
+      )
+      s.error NamedRule.new, 3, 1, "Alarm!", :disabled
+      subject.catch(s).should be_valid
+    end
+
+    it "failes if there is unneeded directive" do
+      s = Source.new %Q(
+        # ameba:disable #{NamedRule.name}
+        a = 1
+      )
+      subject.catch(s).should_not be_valid
+      s.errors.first.message.should eq(
+        "Unnecessary disabling of #{NamedRule.name}"
+      )
+    end
+
+    it "fails if there is inline unneeded directive" do
+      s = Source.new %Q(a = 1 # ameba:disable #{NamedRule.name})
+      subject.catch(s).should_not be_valid
+      s.errors.first.message.should eq(
+        "Unnecessary disabling of #{NamedRule.name}"
+      )
+    end
+
+    it "detects mixed inline directives" do
+      s = Source.new %Q(
+        # ameba:disable Rule1, Rule2
+        a = 1 # ameba:disable Rule3
+      ), "source.cr"
+      subject.catch(s).should_not be_valid
+      s.errors.size.should eq 2
+      s.errors.first.message.should contain "Rule1, Rule2"
+      s.errors.last.message.should contain "Rule3"
+    end
+
+    it "reports error, location and message" do
+      s = Source.new %Q(
+        # ameba:disable Rule1, Rule2
+        a = 1
+      ), "source.cr"
+      subject.catch(s).should_not be_valid
+      error = s.errors.first
+      error.rule.should_not be_nil
+      error.location.to_s.should eq "source.cr:2:9"
+      error.message.should eq "Unnecessary disabling of Rule1, Rule2"
+    end
+  end
+end

src/ameba/inline_comments.cr

@@ -1,5 +1,5 @@
 module Ameba
-  # A module that represents inline comments parsing and processing logic.
+  # A module that utilizes inline comments parsing and processing logic.
   module InlineComments
     COMMENT_DIRECTIVE_REGEX = Regex.new "# ameba : (\\w+) ([\\w, ]+)".gsub(" ", "\\s*")
 
@@ -42,22 +42,48 @@ module Ameba
           line_disabled?(prev_line, rule))
     end
 
-    private def comment?(line)
-      line.lstrip.starts_with? '#'
-    end
-
-    private def line_disabled?(line, rule)
-      return false unless inline_comment = parse_inline_comment(line)
-      inline_comment[:action] == "disable" && inline_comment[:rules].includes?(rule)
-    end
-
-    private def parse_inline_comment(line)
-      if comment = COMMENT_DIRECTIVE_REGEX.match(line)
+    # Parses inline comment directive. Returns a tuple that consists of
+    # an action and parsed rules if directive found, nil otherwise.
+    #
+    # ```
+    # line = "# ameba:disable Rule1, Rule2"
+    # directive = parse_inline_directive(line)
+    # directive[:action] # => "disable"
+    # directive[:rules]  # => ["Rule1", "Rule2"]
+    # ```
+    #
+    # It ignores the directive if it is commented out.
+    #
+    # ```
+    # line = "# # ameba:disable Rule1, Rule2"
+    # parse_inline_directive(line) # => nil
+    # ```
+    #
+    def parse_inline_directive(line)
+      if directive = COMMENT_DIRECTIVE_REGEX.match(line)
+        return if commented_out?(line.gsub directive[0], "")
         {
-          action: comment[1],
-          rules:  comment[2].split(/[\s,]/, remove_empty: true),
+          action: directive[1],
+          rules:  directive[2].split(/[\s,]/, remove_empty: true),
         }
       end
     end
+
+    private def comment?(line)
+      return true if line.lstrip.starts_with? '#'
+    end
+
+    private def line_disabled?(line, rule)
+      return false unless directive = parse_inline_directive(line)
+      directive[:action] == "disable" && directive[:rules].includes?(rule)
+    end
+
+    private def commented_out?(line)
+      commented? = false
+
+      lexer = Crystal::Lexer.new(line).tap { |l| l.comments_enabled = true }
+      Tokenizer.new(lexer).run { |t| commented? = true if t.type == :COMMENT }
+      commented?
+    end
   end
 end

src/ameba/rule/unneeded_disable_directive.cr

@@ -0,0 +1,50 @@
+module Ameba::Rule
+  # A rule that reports unneeded disable directives.
+  # For example, this is considered invalid:
+  #
+  # ```
+  # # ameba:disable PredicateName
+  # def comment?
+  #   do_something
+  # end
+  # ```
+  #
+  # as the predicate name is correct and comment directive does not
+  # have any effect, the snippet should be written as the following:
+  #
+  # ```
+  # def comment?
+  #   do_something
+  # end
+  # ```
+  #
+  struct UnneededDisableDirective < Base
+    properties do
+      description = "Reports unneeded disable directives in comments"
+    end
+
+    def test(source)
+      Tokenizer.new(source).run do |token|
+        next unless token.type == :COMMENT
+        next unless directive = source.parse_inline_directive(token.value.to_s)
+        next unless names = unneeded_disables(source, directive, token.location)
+        next unless names.any?
+
+        source.error self, token.location,
+          "Unnecessary disabling of #{names.join(", ")}"
+      end
+    end
+
+    private def unneeded_disables(source, directive, location)
+      return unless directive[:action] == "disable"
+
+      directive[:rules].reject do |rule_name|
+        any = source.errors.any? do |error|
+          error.rule.name == rule_name &&
+            error.disabled? &&
+            error.location.try(&.line_number) == location.line_number
+        end
+      end
+    end
+  end
+end

src/ameba/tokenizer.cr

@@ -27,6 +27,16 @@ module Ameba
       @lexer.filename = source.path
     end
 
+    # Instantiates Tokenizer using a `lexer`.
+    #
+    # ```
+    # lexer = Crystal::Lexer.new(code)
+    # Ameba::Tokenizer.new(lexer)
+    # ```
+    #
+    def initialize(@lexer : Crystal::Lexer)
+    end
+
     # Runs the tokenizer and yields each token as a block argument.
     #
     # ```