diff --git a/spec/ameba/inline_comments_spec.cr b/spec/ameba/inline_comments_spec.cr index 3f3b2e182..e49661810 100644 --- a/spec/ameba/inline_comments_spec.cr +++ b/spec/ameba/inline_comments_spec.cr @@ -9,14 +9,14 @@ module Ameba result = subject.match("# ameba:enable Group/RuleName") result = result.should_not be_nil result["action"].should eq "enable" - result["rules"].should eq "Group/RuleName" + result["names"].should eq "Group/RuleName" end it "parses multiple rules" do result = subject.match("# ameba:enable Group/RuleName, OtherRule, Foo/Bar") result = result.should_not be_nil result["action"].should eq "enable" - result["rules"].should eq "Group/RuleName, OtherRule, Foo/Bar" + result["names"].should eq "Group/RuleName, OtherRule, Foo/Bar" end it "fails to parse directives with spaces" do @@ -25,109 +25,154 @@ module Ameba end end - it "disables a rule with a comment directive" do - s = Source.new %Q( - # ameba:disable #{NamedRule.name} - Time.epoch(1483859302) - ) - s.add_issue(NamedRule.new, location: {1, 12}, message: "Error!") - s.should be_valid - end + context "ameba:disable_next_line" do + it "disables a rule with a comment directive" do + s = Source.new %Q( + # ameba:disable_next_line #{NamedRule.name} + Time.epoch(1483859302) + ) + s.add_issue(NamedRule.new, location: {2, 12}, message: "Error!") + s.should be_valid + end - it "disables a rule with a line that ends with a comment directive" do - s = Source.new %Q( - Time.epoch(1483859302) # ameba:disable #{NamedRule.name} - ) - s.add_issue(NamedRule.new, location: {1, 12}, message: "Error!") - s.should be_valid - end + it "disables a rule if multiple rule names are separated by comma" do + s = Source.new %Q( + # ameba:disable_next_line SomeRule, LargeNumbers, #{NamedRule.name}, SomeOtherRule + Time.epoch(1483859302) + ) + s.add_issue(NamedRule.new, location: {2, 12}, message: "") + s.should be_valid + end - it "does not disable a rule of a different name" do - s = Source.new %Q( - # ameba:disable WrongName - Time.epoch(1483859302) - ) - s.add_issue(NamedRule.new, location: {2, 12}, message: "Error!") - s.should_not be_valid - end + it "does not disable if multiple rule names used without required one" do + s = Source.new %( + # ameba:disable_next_line SomeRule, SomeOtherRule LargeNumbers + Time.epoch(1483859302) + ) + s.add_issue(NamedRule.new, location: {2, 12}, message: "") + s.should_not be_valid + end - it "disables a rule if multiple rule names provided" do - s = Source.new %Q( - # ameba:disable SomeRule LargeNumbers #{NamedRule.name} SomeOtherRule - Time.epoch(1483859302) - ) - s.add_issue(NamedRule.new, location: {2, 12}, message: "") - s.should be_valid - end + it "does not disable if comment directive has wrong place" do + s = Source.new %Q( + # ameba:disable_next_line #{NamedRule.name} + # + Time.epoch(1483859302) + ) + s.add_issue(NamedRule.new, location: {3, 12}, message: "") + s.should_not be_valid + end - it "disables a rule if multiple rule names are separated by comma" do - s = Source.new %Q( - # ameba:disable SomeRule, LargeNumbers, #{NamedRule.name}, SomeOtherRule - Time.epoch(1483859302) - ) - s.add_issue(NamedRule.new, location: {2, 12}, message: "") - s.should be_valid - end + it "does not disable if that is not a comment directive" do + s = Source.new %Q( + "ameba:disable_next_line #{NamedRule.name}" + Time.epoch(1483859302) + ) + s.add_issue(NamedRule.new, location: {2, 12}, message: "") + s.should_not be_valid + end - it "does not disable if multiple rule names used without required one" do - s = Source.new %( - # ameba:disable SomeRule, SomeOtherRule LargeNumbers - Time.epoch(1483859302) - ) - s.add_issue(NamedRule.new, location: {2, 12}, message: "") - s.should_not be_valid + it "does not disable if that is a commented out directive" do + s = Source.new %Q( + # # ameba:disable_next_line #{NamedRule.name} + Time.epoch(1483859302) + ) + s.add_issue(NamedRule.new, location: {2, 12}, message: "") + s.should_not be_valid + end end - it "does not disable if comment directive has wrong place" do - s = Source.new %Q( - # ameba:disable #{NamedRule.name} - # - Time.epoch(1483859302) - ) - s.add_issue(NamedRule.new, location: {3, 12}, message: "") - s.should_not be_valid - end + context "ameba:disable_line" do + it "disables a rule with a line that ends with a comment directive" do + s = Source.new %Q( + Time.epoch(1483859302) # ameba:disable_line #{NamedRule.name} + ) + s.add_issue(NamedRule.new, location: {1, 12}, message: "Error!") + s.should be_valid + end - it "does not disable if comment directive added to the wrong line" do - s = Source.new %Q( - if use_epoch? # ameba:disable #{NamedRule.name} - Time.epoch(1483859302) - end - ) - s.add_issue(NamedRule.new, location: {3, 12}, message: "") - s.should_not be_valid - end + it "does not disable a rule of a different name" do + s = Source.new %Q( + Time.epoch(1483859302) # ameba:disable_line WrongName + ) + s.add_issue(NamedRule.new, location: {1, 1}, message: "Error!") + s.should_not be_valid + end - it "does not disable if that is not a comment directive" do - s = Source.new %Q( - "ameba:disable #{NamedRule.name}" - Time.epoch(1483859302) - ) - s.add_issue(NamedRule.new, location: {3, 12}, message: "") - s.should_not be_valid - end + it "disables a rule if multiple rule names provided" do + s = Source.new %Q( + Time.epoch(1483859302) # ameba:disable_line SomeRule LargeNumbers #{NamedRule.name} SomeOtherRule + ) + s.add_issue(NamedRule.new, location: {1, 1}, message: "") + s.should 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.add_issue(NamedRule.new, location: {3, 12}, message: "") - s.should_not be_valid + it "does not disable if comment directive added to the wrong line" do + s = Source.new %Q( + if use_epoch? # ameba:disable_line #{NamedRule.name} + Time.epoch(1483859302) + end + ) + s.add_issue(NamedRule.new, location: {2, 12}, message: "") + 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_line #{NamedRule.name} + ) + s.add_issue(NamedRule.new, location: {1, 1}, message: "") + s.should_not be_valid + end 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.add_issue(NamedRule.new, location: {2, 12}, message: "") - s.should_not be_valid + context "ameba:disable/enable" do + it "disables region below" do + s = Source.new %Q( + # ameba:disable #{NamedRule.name} + # something goes here + # something else goes here + a = 1 + ) + s.add_issue(NamedRule.new, location: {4, 1}, message: "") + s.should be_valid + end + + it "disables the line the region starts at" do + s = Source.new %Q( + a = 1 # ameba:disable #{NamedRule.name} + ) + s.add_issue(NamedRule.new, location: {1, 1}, message: "") + s.should be_valid + end + + it "enables disabled region" do + s = Source.new %Q( + # ameba:disable #{NamedRule.name} + # something goes here + # ameba:enable #{NamedRule.name} + # something else goes here + a = 1 + ) + s.add_issue(NamedRule.new, location: {5, 1}, message: "") + s.should_not be_valid + end + + it "disables the rule if disable is not followed by enable directive" do + s = Source.new %Q( + # ameba:enable #{NamedRule.name} + # ameba:disable #{NamedRule.name} + a = 1 + ) + s.add_issue(NamedRule.new, location: {3, 1}, message: "") + s.should be_valid + end end context "with group name" do it "disables one rule with a group" do s = Source.new %Q( - a = 1 # ameba:disable #{DummyRule.rule_name} + a = 1 # ameba:disable_line #{DummyRule.rule_name} ) s.add_issue(DummyRule.new, location: {1, 12}, message: "") s.should be_valid @@ -135,15 +180,15 @@ module Ameba it "doesn't disable others rules" do s = Source.new %Q( - a = 1 # ameba:disable #{DummyRule.rule_name} + a = 1 # ameba:disable_line #{DummyRule.rule_name} ) - s.add_issue(NamedRule.new, location: {2, 12}, message: "") + s.add_issue(NamedRule.new, location: {1, 12}, message: "") s.should_not be_valid end it "disables a hole group of rules" do s = Source.new %Q( - a = 1 # ameba:disable #{DummyRule.group_name} + a = 1 # ameba:disable_line #{DummyRule.group_name} ) s.add_issue(DummyRule.new, location: {1, 12}, message: "") s.should be_valid @@ -151,9 +196,9 @@ module Ameba it "does not disable rules which do not belong to the group" do s = Source.new %Q( - a = 1 # ameba:disable Lint + a = 1 # ameba:disable_line Lint ) - s.add_issue(DummyRule.new, location: {2, 12}, message: "") + s.add_issue(DummyRule.new, location: {1, 12}, message: "") s.should_not be_valid end end diff --git a/spec/ameba/rule/lint/bad_directive_spec.cr b/spec/ameba/rule/lint/bad_directive_spec.cr index 765a83ec6..447233de4 100644 --- a/spec/ameba/rule/lint/bad_directive_spec.cr +++ b/spec/ameba/rule/lint/bad_directive_spec.cr @@ -13,7 +13,7 @@ module Ameba::Rule::Lint it "reports if there is incorrect action" do expect_issue subject, <<-CRYSTAL # ameba:foo Lint/BadDirective - # ^{} error: Bad action in comment directive: 'foo'. Possible values: disable, enable + # ^{} error: Bad action in comment directive: 'foo'. Possible values: enable, disable, disable_line, disable_next_line CRYSTAL end diff --git a/spec/ameba/runner_spec.cr b/spec/ameba/runner_spec.cr index f5ec70c78..3e796c2ab 100644 --- a/spec/ameba/runner_spec.cr +++ b/spec/ameba/runner_spec.cr @@ -185,7 +185,7 @@ module Ameba rules = [NamedRule.new] of Rule::Base source = Source.new %( def foo - bar = 1 # ameba:disable #{NamedRule.name} + bar = 1 # ameba:disable_line #{NamedRule.name} end ) source.add_issue NamedRule.new, location: {2, 1}, diff --git a/src/ameba/inline_comments.cr b/src/ameba/inline_comments.cr index 160a91441..3b2d2ccba 100644 --- a/src/ameba/inline_comments.cr +++ b/src/ameba/inline_comments.cr @@ -1,51 +1,77 @@ module Ameba # A module that utilizes inline comments parsing and processing logic. module InlineComments - COMMENT_DIRECTIVE_REGEX = /# ameba:(?\w+) (?\w+(?:\/\w+)?(?:,? \w+(?:\/\w+)?)*)/ + COMMENT_DIRECTIVE_REGEX = /# ameba:(?\w+) (?\w+(?:\/\w+)?(?:,? \w+(?:\/\w+)?)*)/ # Available actions in the inline comments enum Action - Disable Enable + Disable + DisableLine + DisableNextLine + end + + # Directive the inline comment is parsed to. + struct Directive + getter action : Action + getter names : Array(String) + getter line_number : Int32 + + def initialize(@action, @names, @line_number) + end + + def includes?(rule) + rule.name.in?(names) || rule.group.in?(names) + end end # Returns true if current location is disabled for a particular rule, # false otherwise. # - # Location is disabled in two cases: - # 1. The line of the location ends with a comment directive. - # 2. The line above the location is a comment directive. + # Location can be disabled in the following cases: + # 1. The line of the location ends with ameba:disable_line directive + # 2. The line above the location ends with ameba:disable_next_line directive + # 3. Any line above the location ends with ameba:disable directive # - # For example, here are two examples of disabled location: + # Note: if there is ameba:enable directive which follows the ameba:disable + # directive with the same rule name, it is automatically re-enabled. + # + # For example, in all the cases below directive disables the rule: # # ``` - # # ameba:disable Style/LargeNumbers + # Time.epoch(1483859302) # ameba:disable_line Style/LargeNumbers + # + # # ameba:disable_next_line Style/LargeNumbers # Time.epoch(1483859302) # - # Time.epoch(1483859302) # ameba:disable Style/LargeNumbers + # # ameba:disable Style/LargeNumbers + # Time.epoch(1483859301) + # Time.epoch(1483859302) # ``` # - # But here are examples which are not considered as disabled location: + # In the case below the rule is not disabled and reports and issue: # # ``` # # ameba:disable Style/LargeNumbers - # # + # # ameba:enable Style/LargeNumbers # Time.epoch(1483859302) - # - # if use_epoch? # ameba:disable Style/LargeNumbers - # Time.epoch(1483859302) - # end # ``` def location_disabled?(location, rule) - return false if rule.name.in?(Rule::SPECIAL) - return false unless line_number = location.try &.line_number.try &.- 1 - return false unless line = lines[line_number]? - - line_disabled?(line, rule) || - (line_number > 0 && - (prev_line = lines[line_number - 1]) && - comment?(prev_line) && - line_disabled?(prev_line, rule)) + return false if directives.empty? || rule.name.in?(Rule::SPECIAL) + return false unless line_number = location.try &.line_number + + line_disabled?(line_number, rule) || + next_line_disabled?(line_number, rule) || + region_disabled?(line_number, rule) + end + + def parse_directives(lines) + ([] of Directive).tap do |directives| + lines.each_with_index do |line, line_number| + next unless d = parse_directive(line, line_number + 1) + directives << d + end + end end # Parses inline comment directive. Returns a tuple that consists of @@ -53,23 +79,30 @@ module Ameba # # ``` # line = "# ameba:disable Rule1, Rule2" - # directive = parse_inline_directive(line) - # directive[:action] # => "disable" - # directive[:rules] # => ["Rule1", "Rule2"] + # directive = parse_directive(line, 1) + # 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 + # parse_directive(line) # => nil # ``` - def parse_inline_directive(line) - return unless directive = COMMENT_DIRECTIVE_REGEX.match(line) - return if commented_out?(line.gsub(directive[0], "")) + def parse_directive(line : String, line_number : Int32) + return unless match = match_inline_comment(line) + return unless action = Action.parse?(match[:action]) + Directive.new(action: action, names: match[:names], line_number: line_number) + end + + def match_inline_comment(line) + return unless match = COMMENT_DIRECTIVE_REGEX.match(line) + return if commented_out?(line.gsub(match[0], "")) + { - action: directive["action"], - rules: directive["rules"].split(/[\s,]/, remove_empty: true), + action: match[1], + names: match[2].split(/[\s,]/, remove_empty: true), } end @@ -83,12 +116,29 @@ module Ameba line.lstrip.starts_with? '#' end - private def line_disabled?(line, rule) - return false unless directive = parse_inline_directive(line) - return false unless Action.parse?(directive[:action]).try(&.disable?) + # Disabled by ameba:disable_line + private def line_disabled?(line_number, rule) + return false unless directive = find_directive(line_number) + directive.action.disable_line? && directive.includes?(rule) + end + + # Disabled by ameba:disable_next_line + private def next_line_disabled?(line_number, rule) + return false unless directive = find_directive(line_number - 1) + directive.action.disable_next_line? && directive.includes?(rule) + end + + # Disabled by ameba:disable + private def region_disabled?(line_number, rule) + directives + .select { |d| d.line_number <= line_number && (d.action.disable? || d.action.enable?) } + .reverse! + .find(&.includes?(rule)) + .try &.action.disable? + end - directive[:rules].includes?(rule.name) || - directive[:rules].includes?(rule.group) + private def find_directive(line_number) + directives.find { |d| d.line_number == line_number } end private def commented_out?(line) diff --git a/src/ameba/rule/lint/bad_directive.cr b/src/ameba/rule/lint/bad_directive.cr index e0ff30f41..3371982a0 100644 --- a/src/ameba/rule/lint/bad_directive.cr +++ b/src/ameba/rule/lint/bad_directive.cr @@ -22,17 +22,17 @@ module Ameba::Rule::Lint description "Reports bad comment directives" end - AVAILABLE_ACTIONS = InlineComments::Action.names.map(&.downcase) + AVAILABLE_ACTIONS = InlineComments::Action.names.map(&.underscore) ALL_RULE_NAMES = Rule.rules.map(&.rule_name) ALL_GROUP_NAMES = Rule.rules.map(&.group_name).uniq! 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 match = source.match_inline_comment(token.value.to_s) - check_action source, token, directive[:action] - check_rules source, token, directive[:rules] + check_action source, token, match[:action] + check_rules source, token, match[:names] end end diff --git a/src/ameba/rule/lint/unneeded_disable_directive.cr b/src/ameba/rule/lint/unneeded_disable_directive.cr index a32215a81..2baf0fc11 100644 --- a/src/ameba/rule/lint/unneeded_disable_directive.cr +++ b/src/ameba/rule/lint/unneeded_disable_directive.cr @@ -34,7 +34,7 @@ module Ameba::Rule::Lint 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 directive = source.parse_directive(token.value.to_s, token.location.line_number) next unless names = unneeded_disables(source, directive, token.location) next if names.empty? @@ -43,9 +43,9 @@ module Ameba::Rule::Lint end private def unneeded_disables(source, directive, location) - return unless directive[:action] == "disable" + return unless directive.action.disable? - directive[:rules].reject do |rule_name| + directive.names.reject do |rule_name| next if rule_name == self.name source.issues.any? do |issue| issue.rule.name == rule_name && diff --git a/src/ameba/rule/style/verbose_block.cr b/src/ameba/rule/style/verbose_block.cr index 0cb6a1efb..9817bbad8 100644 --- a/src/ameba/rule/style/verbose_block.cr +++ b/src/ameba/rule/style/verbose_block.cr @@ -189,7 +189,7 @@ module Ameba::Rule::Style CALL_PATTERN % {call.name, args, name} end - # ameba:disable Metrics/CyclomaticComplexity + # ameba:disable_next_line Metrics/CyclomaticComplexity protected def issue_for_valid(source, call : Crystal::Call, block : Crystal::Block, body : Crystal::Call) return unless location = call.name_location return unless end_location = block.end_location diff --git a/src/ameba/source.cr b/src/ameba/source.cr index d6e0b97dc..ce168d54d 100644 --- a/src/ameba/source.cr +++ b/src/ameba/source.cr @@ -63,6 +63,10 @@ module Ameba .parse end + getter directives : Array(Directive) do + parse_directives(lines) + end + getter fullpath : String do File.expand_path(path) end diff --git a/src/ameba/source/rewriter/action.cr b/src/ameba/source/rewriter/action.cr index 23a009eca..f5a8d1c1c 100644 --- a/src/ameba/source/rewriter/action.cr +++ b/src/ameba/source/rewriter/action.cr @@ -121,7 +121,7 @@ class Ameba::Source::Rewriter # In case a child has equal range to *action*, it is returned as `:parent` # # Reminder: an empty range 1...1 is considered disjoint from 1...10 - protected def analyse_hierarchy(action) # ameba:disable Metrics/CyclomaticComplexity + protected def analyse_hierarchy(action) # ameba:disable_line Metrics/CyclomaticComplexity # left_index is the index of the first child that isn't completely to the left of action left_index = bsearch_child_index { |child| child.end_pos > action.begin_pos } # right_index is the index of the first child that is completely on the right of action