From f39a7a4cd4dd5a0d8ec227424b39702ce363a495 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?fn=20=E2=8C=83=20=E2=8C=A5?=
 <70830482+FnControlOption@users.noreply.github.com>
Date: Mon, 25 Oct 2021 15:09:39 -0700
Subject: [PATCH] Re-run autocorrect until all correctable issues have been
 corrected

---
 spec/ameba/formatter/util_spec.cr        | 16 ++++++-------
 spec/ameba/issue_spec.cr                 | 15 ++++++++----
 spec/ameba/spec/annotated_source_spec.cr |  6 +++--
 src/ameba/formatter/dot_formatter.cr     |  2 +-
 src/ameba/formatter/explain_formatter.cr |  7 ++----
 src/ameba/formatter/util.cr              | 10 ++++++--
 src/ameba/issue.cr                       |  5 +++-
 src/ameba/reportable.cr                  |  2 +-
 src/ameba/runner.cr                      | 23 +++++++++---------
 src/ameba/source.cr                      | 30 +++++++++++++++++++-----
 src/ameba/source/corrector.cr            | 10 --------
 src/ameba/spec/expect_issue.cr           | 12 ++++------
 12 files changed, 78 insertions(+), 60 deletions(-)

diff --git a/spec/ameba/formatter/util_spec.cr b/spec/ameba/formatter/util_spec.cr
index 9087228f..0ddcfb4b 100644
--- a/spec/ameba/formatter/util_spec.cr
+++ b/spec/ameba/formatter/util_spec.cr
@@ -69,24 +69,24 @@ module Ameba::Formatter
 
     describe "#affected_code" do
       it "returns nil if there is no such a line number" do
-        source = Source.new %(
+        code = <<-EOF
           a = 1
-        )
+          EOF
         location = Crystal::Location.new("filename", 2, 1)
-        subject.affected_code(source, location).should be_nil
+        subject.affected_code(code, location).should be_nil
       end
 
       it "returns correct line if it is found" do
-        source = Source.new %(
+        code = <<-EOF
           a = 1
-        )
+          EOF
         location = Crystal::Location.new("filename", 1, 1)
-        subject.deansify(subject.affected_code(source, location))
+        subject.deansify(subject.affected_code(code, location))
           .should eq "> a = 1\n  ^\n"
       end
 
       it "returns correct line if it is found" do
-        source = Source.new <<-EOF
+        code = <<-EOF
           # pre:1
             # pre:2
               # pre:3
@@ -101,7 +101,7 @@ module Ameba::Formatter
           EOF
 
         location = Crystal::Location.new("filename", 6, 1)
-        subject.deansify(subject.affected_code(source, location, context_lines: 3))
+        subject.deansify(subject.affected_code(code, location, context_lines: 3))
           .should eq <<-STR
             >     # pre:3
             >       # pre:4
diff --git a/spec/ameba/issue_spec.cr b/spec/ameba/issue_spec.cr
index b6488552..08188eaf 100644
--- a/spec/ameba/issue_spec.cr
+++ b/spec/ameba/issue_spec.cr
@@ -3,7 +3,8 @@ require "../spec_helper"
 module Ameba
   describe Issue do
     it "accepts rule and message" do
-      issue = Issue.new rule: DummyRule.new,
+      issue = Issue.new code: "",
+        rule: DummyRule.new,
         location: nil,
         end_location: nil,
         message: "Blah",
@@ -15,7 +16,8 @@ module Ameba
 
     it "accepts location" do
       location = Crystal::Location.new("path", 3, 2)
-      issue = Issue.new rule: DummyRule.new,
+      issue = Issue.new code: "",
+        rule: DummyRule.new,
         location: location,
         end_location: nil,
         message: "Blah",
@@ -27,7 +29,8 @@ module Ameba
 
     it "accepts end_location" do
       location = Crystal::Location.new("path", 3, 2)
-      issue = Issue.new rule: DummyRule.new,
+      issue = Issue.new code: "",
+        rule: DummyRule.new,
         location: nil,
         end_location: location,
         message: "Blah",
@@ -38,7 +41,8 @@ module Ameba
     end
 
     it "accepts status" do
-      issue = Issue.new rule: DummyRule.new,
+      issue = Issue.new code: "",
+        rule: DummyRule.new,
         location: nil,
         end_location: nil,
         message: "",
@@ -50,7 +54,8 @@ module Ameba
     end
 
     it "sets status to :enabled by default" do
-      issue = Issue.new rule: DummyRule.new,
+      issue = Issue.new code: "",
+        rule: DummyRule.new,
         location: nil,
         end_location: nil,
         message: ""
diff --git a/spec/ameba/spec/annotated_source_spec.cr b/spec/ameba/spec/annotated_source_spec.cr
index 9f801f46..538f054b 100644
--- a/spec/ameba/spec/annotated_source_spec.cr
+++ b/spec/ameba/spec/annotated_source_spec.cr
@@ -1,6 +1,7 @@
 require "../../spec_helper"
 
-private def dummy_issue(message,
+private def dummy_issue(code,
+                        message,
                         position : {Int32, Int32}?,
                         end_position : {Int32, Int32}?,
                         path = "")
@@ -9,6 +10,7 @@ private def dummy_issue(message,
   end_location = Crystal::Location.new(path, *end_position) if end_position
 
   Ameba::Issue.new(
+    code: code,
     rule: Ameba::DummyRule.new,
     location: location,
     end_location: end_location,
@@ -25,7 +27,7 @@ private def expect_invalid_location(code,
   expect_raises Exception, exception_message, file, line do
     Ameba::Spec::AnnotatedSource.new(
       lines: code.lines,
-      issues: [dummy_issue("Message", position, end_position, "path")]
+      issues: [dummy_issue(code, "Message", position, end_position, "path")]
     )
   end
 end
diff --git a/src/ameba/formatter/dot_formatter.cr b/src/ameba/formatter/dot_formatter.cr
index 91854b6c..9f998ecd 100644
--- a/src/ameba/formatter/dot_formatter.cr
+++ b/src/ameba/formatter/dot_formatter.cr
@@ -50,7 +50,7 @@ module Ameba::Formatter
             "#{issue.rule.name}: " \
             "#{issue.message}".colorize(:red)
 
-          if show_affected_code && (code = affected_code(source, location, issue.end_location))
+          if show_affected_code && (code = affected_code(issue))
             output << code.colorize(:default)
           end
 
diff --git a/src/ameba/formatter/explain_formatter.cr b/src/ameba/formatter/explain_formatter.cr
index f8fe87c1..33c8b7d5 100644
--- a/src/ameba/formatter/explain_formatter.cr
+++ b/src/ameba/formatter/explain_formatter.cr
@@ -38,10 +38,7 @@ module Ameba::Formatter
     private def explain(source, issue)
       rule = issue.rule
 
-      location, end_location =
-        issue.location, issue.end_location
-
-      return unless location
+      return unless (location = issue.location)
 
       output_title "ISSUE INFO"
       output_paragraph [
@@ -49,7 +46,7 @@ module Ameba::Formatter
         location.to_s.colorize(:cyan).to_s,
       ]
 
-      if affected_code = affected_code(source, location, end_location, context_lines: 3)
+      if affected_code = affected_code(issue, context_lines: 3)
         output_title "AFFECTED CODE"
         output_paragraph affected_code
       end
diff --git a/src/ameba/formatter/util.cr b/src/ameba/formatter/util.cr
index d6c411c8..cbf0907c 100644
--- a/src/ameba/formatter/util.cr
+++ b/src/ameba/formatter/util.cr
@@ -40,8 +40,14 @@ module Ameba::Formatter
       {pre_context, post_context}
     end
 
-    def affected_code(source, location, end_location = nil, context_lines = 0, max_length = 120, ellipsis = " ...", prompt = "> ")
-      lines = source.lines
+    def affected_code(issue : Issue, context_lines = 0, max_length = 120, ellipsis = " ...", prompt = "> ")
+      return unless (location = issue.location)
+
+      affected_code(issue.code, location, issue.end_location, context_lines, max_length, ellipsis, prompt)
+    end
+
+    def affected_code(code, location, end_location = nil, context_lines = 0, max_length = 120, ellipsis = " ...", prompt = "> ")
+      lines = code.split('\n') # must preserve trailing newline
       lineno, column =
         location.line_number, location.column_number
 
diff --git a/src/ameba/issue.cr b/src/ameba/issue.cr
index 06619686..b6fa55e8 100644
--- a/src/ameba/issue.cr
+++ b/src/ameba/issue.cr
@@ -6,6 +6,9 @@ module Ameba
       Disabled
     end
 
+    # The source code that triggered this issue.
+    getter code : String
+
     # A rule that triggers this issue.
     getter rule : Rule::Base
 
@@ -24,7 +27,7 @@ module Ameba
     delegate :enabled?, :disabled?,
       to: status
 
-    def initialize(@rule, @location, @end_location, @message, status : Status? = nil, @block : (Source::Corrector ->)? = nil)
+    def initialize(@code, @rule, @location, @end_location, @message, status : Status? = nil, @block : (Source::Corrector ->)? = nil)
       @status = status || Status::Enabled
     end
 
diff --git a/src/ameba/reportable.cr b/src/ameba/reportable.cr
index a85bf28b..ec9e77a5 100644
--- a/src/ameba/reportable.cr
+++ b/src/ameba/reportable.cr
@@ -9,7 +9,7 @@ module Ameba
       status ||=
         Issue::Status::Disabled if location_disabled?(location, rule)
 
-      Issue.new(rule, location, end_location, message, status, block).tap do |issue|
+      Issue.new(code, rule, location, end_location, message, status, block).tap do |issue|
         issues << issue
       end
     end
diff --git a/src/ameba/runner.cr b/src/ameba/runner.cr
index 0bc33f08..b6820e9a 100644
--- a/src/ameba/runner.cr
+++ b/src/ameba/runner.cr
@@ -93,17 +93,23 @@ module Ameba
     private def run_source(source)
       @formatter.source_started source
 
-      # TODO: run autocorrection recursively. A new `Issue#source` property must
-      #       be added so that `affected_code` will return the code from the old
-      #       source instead of the autocorrected one.
-      if @syntax_rule.catch(source).valid?
+      corrected_issues = [] of Issue
+      loop do
+        @syntax_rule.test(source)
+        break unless source.valid?
+
         @rules.each do |rule|
           next if rule.excluded?(source)
           rule.test(source)
         end
         check_unneeded_directives(source)
-        autocorrect(source) if autocorrect?
+        break unless autocorrect? && source.correct
+
+        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) if corrected_issues.any?
 
       @formatter.source_finished source
     end
@@ -143,12 +149,5 @@ module Ameba
         rule.test(source)
       end
     end
-
-    private def autocorrect(source)
-      corrected_code = Source::Corrector.correct(source)
-      return unless corrected_code
-
-      File.write(source.path, corrected_code)
-    end
   end
 end
diff --git a/src/ameba/source.cr b/src/ameba/source.cr
index 68dd2a1c..ccd71af1 100644
--- a/src/ameba/source.cr
+++ b/src/ameba/source.cr
@@ -11,6 +11,9 @@ module Ameba
     # Crystal code (content of a source file).
     getter code : String
 
+    @lines : Array(String)?
+    @ast : Crystal::ASTNode?
+
     # Creates a new source by `code` and `path`.
     #
     # For example:
@@ -22,6 +25,18 @@ module Ameba
     def initialize(@code, @path = "")
     end
 
+    def correct
+      corrector = Corrector.new(code)
+      issues.each(&.correct(corrector))
+      corrected_code = corrector.process
+      return false if code == corrected_code
+
+      @code = corrected_code
+      @lines = nil
+      @ast = nil
+      true
+    end
+
     # Returns lines of code splitted by new line character.
     # Since `code` is immutable and can't be changed, this
     # method caches lines in an instance variable, so calling
@@ -33,7 +48,9 @@ module Ameba
     # source.lines # => ["a = 1", "b = 2"]
     # ```
     #
-    getter lines : Array(String) { code.split('\n') }
+    def lines : Array(String)
+      @lines ||= code.split('\n')
+    end
 
     # Returns AST nodes constructed by `Crystal::Parser`.
     #
@@ -42,11 +59,12 @@ module Ameba
     # source.ast
     # ```
     #
-    getter ast : Crystal::ASTNode do
-      Crystal::Parser.new(code)
-        .tap(&.wants_doc = true)
-        .tap(&.filename = path)
-        .parse
+    def ast : Crystal::ASTNode
+      @ast ||=
+        Crystal::Parser.new(code)
+          .tap(&.wants_doc = true)
+          .tap(&.filename = path)
+          .parse
     end
 
     getter fullpath : String do
diff --git a/src/ameba/source/corrector.cr b/src/ameba/source/corrector.cr
index ae3725e7..a8d29446 100644
--- a/src/ameba/source/corrector.cr
+++ b/src/ameba/source/corrector.cr
@@ -2,16 +2,6 @@ require "./rewriter"
 
 class Ameba::Source
   class Corrector
-    def self.correct(source : Source)
-      code = source.code
-      corrector = new(code)
-      source.issues.each(&.correct(corrector))
-      corrected_code = corrector.process
-      return if code == corrected_code
-
-      corrected_code
-    end
-
     @line_sizes : Array(Int32)
 
     def initialize(code : String)
diff --git a/src/ameba/spec/expect_issue.cr b/src/ameba/spec/expect_issue.cr
index 3571b164..7e9872ed 100644
--- a/src/ameba/spec/expect_issue.cr
+++ b/src/ameba/spec/expect_issue.cr
@@ -85,9 +85,8 @@ module Ameba::Spec::ExpectIssue
     source = ExpectIssue.source
     raise "`expect_correction` must follow `expect_issue`" unless source
 
-    corrected_code = Source::Corrector.correct(source) # TODO: recursive
-    raise "Use `expect_no_corrections` if the code will not change" unless corrected_code
-    return if correction == corrected_code
+    raise "Use `expect_no_corrections` if the code will not change" unless source.correct
+    return if correction == source.code
 
     fail <<-MSG, file, line
       Expected correction:
@@ -96,7 +95,7 @@ module Ameba::Spec::ExpectIssue
 
       Got:
 
-      #{corrected_code}
+      #{source.code}
       MSG
   end
 
@@ -104,13 +103,12 @@ module Ameba::Spec::ExpectIssue
     source = ExpectIssue.source
     raise "`expect_no_corrections` must follow `expect_offense`" unless source
 
-    corrected_code = Source::Corrector.correct(source)
-    return unless corrected_code
+    return unless source.correct
 
     fail <<-MSG, file, line
       Expected no corrections, but got:
 
-      #{corrected_code}
+      #{source.code}
       MSG
   end