diff --git a/spec/ameba/runner_spec.cr b/spec/ameba/runner_spec.cr index 703f39ee..f5ec70c7 100644 --- a/spec/ameba/runner_spec.cr +++ b/spec/ameba/runner_spec.cr @@ -8,6 +8,13 @@ module Ameba config.update_rule ErrorRule.rule_name, enabled: false config.update_rule PerfRule.rule_name, enabled: false + config.update_rule AtoAA.rule_name, enabled: false + config.update_rule AtoB.rule_name, enabled: false + config.update_rule BtoA.rule_name, enabled: false + config.update_rule BtoC.rule_name, enabled: false + config.update_rule CtoA.rule_name, enabled: false + config.update_rule ClassToModule.rule_name, enabled: false + config.update_rule ModuleToClass.rule_name, enabled: false Runner.new(config) end @@ -54,6 +61,15 @@ module Ameba Runner.new(all_rules, [source], formatter, default_severity).run.success?.should be_true end + it "aborts because of an infinite loop" do + rules = [AtoAA.new] of Rule::Base + source = Source.new "class A; end", "source.cr" + message = "Infinite loop in source.cr caused by Ameba/AtoAA" + expect_raises(Runner::InfiniteCorrectionLoopError, message) do + Runner.new(rules, [source], formatter, default_severity, autocorrect: true).run + end + end + context "exception in rule" do it "raises an exception raised in fiber while running a rule" do rule = RaiseRule.new @@ -180,5 +196,56 @@ module Ameba .run.success?.should be_true end end + + describe "#run with rules autocorrecting each other" do + context "with two conflicting rules" do + context "if there is an offense in an inspected file" do + it "aborts because of an infinite loop" do + rules = [AtoB.new, BtoA.new] + source = Source.new "class A; end", "source.cr" + message = "Infinite loop in source.cr caused by Ameba/AtoB -> Ameba/BtoA" + expect_raises(Runner::InfiniteCorrectionLoopError, message) do + Runner.new(rules, [source], formatter, default_severity, autocorrect: true).run + end + end + end + + context "if there are multiple offenses in an inspected file" do + it "aborts because of an infinite loop" do + rules = [AtoB.new, BtoA.new] + source = Source.new %( + class A; end + class A_A; end + ), "source.cr" + message = "Infinite loop in source.cr caused by Ameba/AtoB -> Ameba/BtoA" + expect_raises(Runner::InfiniteCorrectionLoopError, message) do + Runner.new(rules, [source], formatter, default_severity, autocorrect: true).run + end + end + end + end + + context "with two pairs of conflicting rules" do + it "aborts because of an infinite loop" do + rules = [ClassToModule.new, ModuleToClass.new, AtoB.new, BtoA.new] + source = Source.new "class A_A; end", "source.cr" + message = "Infinite loop in source.cr caused by Ameba/ClassToModule, Ameba/AtoB -> Ameba/ModuleToClass, Ameba/BtoA" + expect_raises(Runner::InfiniteCorrectionLoopError, message) do + Runner.new(rules, [source], formatter, default_severity, autocorrect: true).run + end + end + end + + context "with three rule cycle" do + it "aborts because of an infinite loop" do + rules = [AtoB.new, BtoC.new, CtoA.new] + source = Source.new "class A; end", "source.cr" + message = "Infinite loop in source.cr caused by Ameba/AtoB -> Ameba/BtoC -> Ameba/CtoA" + expect_raises(Runner::InfiniteCorrectionLoopError, message) do + Runner.new(rules, [source], formatter, default_severity, autocorrect: true).run + end + end + end + end end end diff --git a/spec/spec_helper.cr b/spec/spec_helper.cr index c468bed5..aa489fca 100644 --- a/spec/spec_helper.cr +++ b/spec/spec_helper.cr @@ -95,6 +95,133 @@ module Ameba end end + class AtoAA < Rule::Base + include AST::Util + + properties do + description "This rule is only used to test infinite loop detection" + end + + def test(source, node : Crystal::ClassDef | Crystal::ModuleDef) + return unless (name = node_source(node.name, source.lines)) + return unless name.includes?("A") + + issue_for(node.name, message: "A to AA") do |corrector| + corrector.replace(node.name, name.sub("A", "AA")) + end + end + end + + class AtoB < Rule::Base + include AST::Util + + properties do + description "This rule is only used to test infinite loop detection" + end + + def test(source, node : Crystal::ClassDef | Crystal::ModuleDef) + return unless (name = node_source(node.name, source.lines)) + return unless name.includes?("A") + + issue_for(node.name, message: "A to B") do |corrector| + corrector.replace(node.name, name.tr("A", "B")) + end + end + end + + class BtoA < Rule::Base + include AST::Util + + properties do + description "This rule is only used to test infinite loop detection" + end + + def test(source, node : Crystal::ClassDef | Crystal::ModuleDef) + return unless (name = node_source(node.name, source.lines)) + return unless name.includes?("B") + + issue_for(node.name, message: "B to A") do |corrector| + corrector.replace(node.name, name.tr("B", "A")) + end + end + end + + class BtoC < Rule::Base + include AST::Util + + properties do + description "This rule is only used to test infinite loop detection" + end + + def test(source, node : Crystal::ClassDef | Crystal::ModuleDef) + return unless (name = node_source(node.name, source.lines)) + return unless name.includes?("B") + + issue_for(node.name, message: "B to C") do |corrector| + corrector.replace(node.name, name.tr("B", "C")) + end + end + end + + class CtoA < Rule::Base + include AST::Util + + properties do + description "This rule is only used to test infinite loop detection" + end + + def test(source, node : Crystal::ClassDef | Crystal::ModuleDef) + return unless (name = node_source(node.name, source.lines)) + return unless name.includes?("C") + + issue_for(node.name, message: "C to A") do |corrector| + corrector.replace(node.name, name.tr("C", "A")) + end + end + end + + class ClassToModule < Ameba::Rule::Base + include Ameba::AST::Util + + properties do + description "This rule is only used to test infinite loop detection" + end + + def test(source, node : Crystal::ClassDef) + return unless (location = node.location) + + end_location = Crystal::Location.new( + location.filename, + location.line_number, + location.column_number + "class".size - 1 + ) + issue_for(location, end_location, message: "class to module") do |corrector| + corrector.replace(location, end_location, "module") + end + end + end + + class ModuleToClass < Ameba::Rule::Base + include Ameba::AST::Util + + properties do + description "This rule is only used to test infinite loop detection" + end + + def test(source, node : Crystal::ModuleDef) + return unless (location = node.location) + + end_location = Crystal::Location.new( + location.filename, + location.line_number, + location.column_number + "module".size - 1 + ) + issue_for(location, end_location, message: "module to class") do |corrector| + corrector.replace(location, end_location, "class") + end + end + end + class DummyFormatter < Formatter::BaseFormatter property started_sources : Array(Source)? property finished_sources : Array(Source)? diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr index 429e7bd5..c22ad752 100644 --- a/src/ameba/runner.cr +++ b/src/ameba/runner.cr @@ -1,3 +1,5 @@ +require "digest" + module Ameba # Represents a runner for inspecting sources files. # Holds a list of rules to do inspection based on, @@ -11,6 +13,25 @@ module Ameba # ``` # class Runner + # An error indicating that the inspection loop got stuck correcting + # issues back and forth. + class InfiniteCorrectionLoopError < RuntimeError + def initialize(path, issues_by_iteration, loop_start = -1) + root_cause = + issues_by_iteration[loop_start..-1] + .map(&.map(&.rule.name).uniq!.join(", ")) + .join(" -> ") + + message = String.build do |io| + io << "Infinite loop" + io << " in " << path unless path.empty? + io << " caused by " << root_cause + end + + super message + end + end + # A list of rules to do inspection based on. @rules : Array(Rule::Base) @@ -52,8 +73,7 @@ module Ameba .find &.name.==(Rule::Lint::UnneededDisableDirective.rule_name) end - protected def initialize(@rules, @sources, @formatter, @severity) - @autocorrect = false + protected def initialize(@rules, @sources, @formatter, @severity, @autocorrect = false) end # Performs the inspection. Iterates through all sources and test it using @@ -93,8 +113,18 @@ module Ameba private def run_source(source) @formatter.source_started source - corrected_issues = [] of Issue - loop do + # This variable is a 2D array used to track corrected issues after each + # inspection iteration. This is used to output meaningful infinite loop + # error message. + corrected_issues = [] of Array(Issue) + + # When running with --fix, we need to inspect the source until no more + # corrections are made (because automatic corrections can introduce new + # issues). In the normal case the loop is only executed once. + loop_unless_infinite(source, corrected_issues) do + # We have to reprocess the source to pick up any changes. Since a + # change could (theoretically) introduce syntax errors, we break the + # loop if we find any. @syntax_rule.test(source) break unless source.valid? @@ -105,12 +135,18 @@ module Ameba check_unneeded_directives(source) break unless autocorrect? && source.correct - corrected_issues += source.issues.select(&.correctable?) + # The issues that couldn't be corrected will be found again so we + # only keep the corrected ones in order to avoid duplicate reporting. + corrected_issues << source.issues.select(&.correctable?) source.issues.clear end - corrected_issues.reverse_each { |issue| source.issues.unshift(issue) } - File.write(source.path, source.code) unless corrected_issues.empty? + corrected_issues.flatten.reverse_each do |issue| + source.issues.unshift(issue) + end + + File.write(source.path, source.code) unless corrected_issues.empty? + ensure @formatter.source_finished source end @@ -144,6 +180,46 @@ module Ameba end end + private MAX_ITERATIONS = 200 + + private def loop_unless_infinite(source, corrected_issues) + # Keep track of the state of the source. If a rule modifies the source + # and another rule undoes it producing identical source we have an + # infinite loop. + processed_sources = [] of String + + # It is possible for a rule to keep adding indefinitely to a file, + # making it bigger and bigger. If the inspection loop runs for an + # excessively high number of iterations, this is likely happening. + iterations = 0 + + loop do + check_for_infinite_loop(source, corrected_issues, processed_sources) + + if (iterations += 1) > MAX_ITERATIONS + raise InfiniteCorrectionLoopError.new(source.path, corrected_issues) + end + + yield + end + end + + # Check whether a run created source identical to a previous run, which + # means that we definitely have an infinite loop. + private def check_for_infinite_loop(source, corrected_issues, processed_sources) + checksum = Digest::SHA1.hexdigest(source.code) + + if (loop_start = processed_sources.index(checksum)) + raise InfiniteCorrectionLoopError.new( + source.path, + corrected_issues, + loop_start: loop_start + ) + end + + processed_sources << checksum + end + private def check_unneeded_directives(source) if (rule = @unneeded_disable_directive_rule) && rule.enabled rule.test(source)