Raise error if infinite correction loop

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

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)?

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)
+    protected def initialize(@rules, @sources, @formatter, @severity, @autocorrect = false)
-      @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
+      # This variable is a 2D array used to track corrected issues after each
-      loop do
+      # 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)