From 8075c39aa935e0d6352ebfe8deb823be0acf4b08 Mon Sep 17 00:00:00 2001 From: Vitalii Elenhaupt Date: Wed, 31 Jan 2018 19:46:44 +0200 Subject: [PATCH] Unneeded disable directive --- spec/ameba/inline_comments_spec.cr | 17 ++++ .../rule/unneded_disable_directive_spec.cr | 80 +++++++++++++++++++ src/ameba/inline_comments.cr | 54 +++++++++---- src/ameba/rule/unneeded_disable_directive.cr | 50 ++++++++++++ src/ameba/tokenizer.cr | 10 +++ 5 files changed, 197 insertions(+), 14 deletions(-) create mode 100644 spec/ameba/rule/unneded_disable_directive_spec.cr create mode 100644 src/ameba/rule/unneeded_disable_directive.cr diff --git a/spec/ameba/inline_comments_spec.cr b/spec/ameba/inline_comments_spec.cr index 14193267..e0bd843e 100644 --- a/spec/ameba/inline_comments_spec.cr +++ b/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 diff --git a/spec/ameba/rule/unneded_disable_directive_spec.cr b/spec/ameba/rule/unneded_disable_directive_spec.cr new file mode 100644 index 00000000..c83e061f --- /dev/null +++ b/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 diff --git a/src/ameba/inline_comments.cr b/src/ameba/inline_comments.cr index 9eaa22ab..7c090d95 100644 --- a/src/ameba/inline_comments.cr +++ b/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 diff --git a/src/ameba/rule/unneeded_disable_directive.cr b/src/ameba/rule/unneeded_disable_directive.cr new file mode 100644 index 00000000..46aadf11 --- /dev/null +++ b/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 diff --git a/src/ameba/tokenizer.cr b/src/ameba/tokenizer.cr index 445c64ee..f5bfb346 100644 --- a/src/ameba/tokenizer.cr +++ b/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. # # ```