Add Source::Corrector and Source::Rewriter

spec/ameba/source/rewriter_spec.cr

@@ -0,0 +1,110 @@
+require "../../spec_helper"
+
+class Ameba::Source
+  describe Rewriter do
+    code = "puts(:hello, :world)"
+    hello = {5, 11}
+    comma_space = {11, 13}
+    world = {13, 19}
+
+    it "can remove" do
+      rewriter = Rewriter.new(code)
+      rewriter.remove(*hello)
+      rewriter.process.should eq "puts(, :world)"
+    end
+
+    it "can insert before" do
+      rewriter = Rewriter.new(code)
+      rewriter.insert_before(*world, "42, ")
+      rewriter.process.should eq "puts(:hello, 42, :world)"
+    end
+
+    it "can insert after" do
+      rewriter = Rewriter.new(code)
+      rewriter.insert_after(*hello, ", 42")
+      rewriter.process.should eq "puts(:hello, 42, :world)"
+    end
+
+    it "can wrap" do
+      rewriter = Rewriter.new(code)
+      rewriter.wrap(*hello, '[', ']')
+      rewriter.process.should eq "puts([:hello], :world)"
+    end
+
+    it "can replace" do
+      rewriter = Rewriter.new(code)
+      rewriter.replace(*hello, ":hi")
+      rewriter.process.should eq "puts(:hi, :world)"
+    end
+
+    it "accepts crossing deletions" do
+      rewriter = Rewriter.new(code)
+      rewriter.remove(hello[0], comma_space[1])
+      rewriter.remove(comma_space[0], world[1])
+      rewriter.process.should eq "puts()"
+    end
+
+    it "accepts multiple actions" do
+      rewriter = Rewriter.new(code)
+      rewriter.replace(*comma_space, " => ")
+      rewriter.wrap(hello[0], world[1], '{', '}')
+      rewriter.replace(*world, ":everybody")
+      rewriter.wrap(*world, '[', ']')
+      rewriter.process.should eq "puts({:hello => [:everybody]})"
+    end
+
+    it "can wrap the same range" do
+      rewriter = Rewriter.new(code)
+      rewriter.wrap(*hello, '(', ')')
+      rewriter.wrap(*hello, '[', ']')
+      rewriter.process.should eq "puts([(:hello)], :world)"
+    end
+
+    it "can insert on empty ranges" do
+      rewriter = Rewriter.new(code)
+      rewriter.insert_before(hello[0], '{')
+      rewriter.replace(hello[0], hello[0], 'x')
+      rewriter.insert_after(hello[0], '}')
+      rewriter.insert_before(hello[1], '[')
+      rewriter.replace(hello[1], hello[1], 'y')
+      rewriter.insert_after(hello[1], ']')
+      rewriter.process.should eq "puts({x}:hello[y], :world)"
+    end
+
+    it "can replace the same range" do
+      rewriter = Rewriter.new(code)
+      rewriter.replace(*hello, ":hi")
+      rewriter.replace(*hello, ":hey")
+      rewriter.process.should eq "puts(:hey, :world)"
+    end
+
+    it "can swallow insertions" do
+      rewriter = Rewriter.new(code)
+      rewriter.wrap(hello[0] + 1, hello[1], "__", "__")
+      rewriter.replace(world[0], world[1] - 2, "xx")
+      rewriter.replace(hello[0], world[1], ":hi")
+      rewriter.process.should eq "puts(:hi)"
+    end
+
+    it "rejects out-of-range ranges" do
+      rewriter = Rewriter.new(code)
+      expect_raises(IndexError) { rewriter.insert_before(0, 100, "hola") }
+    end
+
+    it "ignores trivial actions" do
+      rewriter = Rewriter.new(code)
+      rewriter.empty?.should be_true
+
+      # This is a trivial wrap
+      rewriter.wrap(2, 5, "", "")
+      rewriter.empty?.should be_true
+
+      # This is a trivial deletion
+      rewriter.remove(2, 2)
+      rewriter.empty?.should be_true
+
+      rewriter.remove(2, 5)
+      rewriter.empty?.should be_false
+    end
+  end
+end

src/ameba.cr

@@ -2,6 +2,7 @@ require "./ameba/*"
 require "./ameba/ast/**"
 require "./ameba/rule/**"
 require "./ameba/formatter/*"
+require "./ameba/source/**"
 
 # Ameba's entry module.
 #

src/ameba/issue.cr

@@ -24,12 +24,20 @@ module Ameba
     delegate :enabled?, :disabled?,
       to: status
 
-    def initialize(@rule, @location, @end_location, @message, status : Status? = nil)
+    def initialize(@rule, @location, @end_location, @message, status : Status? = nil, @block : (Source::Corrector ->)? = nil)
       @status = status || Status::Enabled
     end
 
     def syntax?
       rule.is_a?(Rule::Lint::Syntax)
     end
+
+    def correctable?
+      !@block.nil?
+    end
+
+    def correct(corrector)
+      @block.try &.call(corrector)
+    end
   end
 end

src/ameba/reportable.cr

@@ -5,41 +5,76 @@ module Ameba
     getter issues = [] of Issue
 
     # Adds a new issue to the list of issues.
-    def add_issue(rule, location, end_location, message, status : Issue::Status? = nil) : Issue
+    def add_issue(rule, location, end_location, message, status : Issue::Status? = nil, block : (Source::Corrector ->)? = nil) : Issue
       status ||=
         Issue::Status::Disabled if location_disabled?(location, rule)
 
-      Issue.new(rule, location, end_location, message, status).tap do |issue|
+      Issue.new(rule, location, end_location, message, status, block).tap do |issue|
         issues << issue
       end
     end
 
+    # :ditto:
+    def add_issue(rule, location, end_location, message, status : Issue::Status? = nil, &block : Source::Corrector ->) : Issue
+      add_issue rule, location, end_location, message, status, block
+    end
+
     # Adds a new issue for Crystal AST *node*.
-    def add_issue(rule, node : Crystal::ASTNode, message, status : Issue::Status? = nil) : Issue
-      add_issue rule, node.location, node.end_location, message, status
+    def add_issue(rule, node : Crystal::ASTNode, message, status : Issue::Status? = nil, block : (Source::Corrector ->)? = nil) : Issue
+      add_issue rule, node.location, node.end_location, message, status, block
+    end
+
+    # :ditto:
+    def add_issue(rule, node : Crystal::ASTNode, message, status : Issue::Status? = nil, &block : Source::Corrector ->) : Issue
+      add_issue rule, node, message, status, block
     end
 
     # Adds a new issue for Crystal *token*.
-    def add_issue(rule, token : Crystal::Token, message, status : Issue::Status? = nil) : Issue
-      add_issue rule, token.location, nil, message, status
+    def add_issue(rule, token : Crystal::Token, message, status : Issue::Status? = nil, block : (Source::Corrector ->)? = nil) : Issue
+      add_issue rule, token.location, nil, message, status, block
+    end
+
+    # :ditto:
+    def add_issue(rule, token : Crystal::Token, message, status : Issue::Status? = nil, &block : Source::Corrector ->) : Issue
+      add_issue rule, token, message, status, block
     end
 
     # Adds a new issue for *location* defined by line and column numbers.
-    def add_issue(rule, location : {Int32, Int32}, message, status : Issue::Status? = nil) : Issue
+    def add_issue(rule, location : {Int32, Int32}, message, status : Issue::Status? = nil, block : (Source::Corrector ->)? = nil) : Issue
       location =
         Crystal::Location.new(path, *location)
 
-      add_issue rule, location, nil, message, status
+      add_issue rule, location, nil, message, status, block
+    end
+
+    # :ditto:
+    def add_issue(rule, location : {Int32, Int32}, message, status : Issue::Status? = nil, &block : Source::Corrector ->) : Issue
+      add_issue rule, location, message, status, block
     end
 
     # Adds a new issue for *location* and *end_location* defined by line and column numbers.
-    def add_issue(rule, location : {Int32, Int32}, end_location : {Int32, Int32}, message, status : Issue::Status? = nil) : Issue
+    def add_issue(rule,
+                  location : {Int32, Int32},
+                  end_location : {Int32, Int32},
+                  message,
+                  status : Issue::Status? = nil,
+                  block : (Source::Corrector ->)? = nil) : Issue
       location =
         Crystal::Location.new(path, *location)
       end_location =
         Crystal::Location.new(path, *end_location)
 
-      add_issue rule, location, end_location, message, status
+      add_issue rule, location, end_location, message, status, block
+    end
+
+    # :ditto:
+    def add_issue(rule,
+                  location : {Int32, Int32},
+                  end_location : {Int32, Int32},
+                  message,
+                  status : Issue::Status? = nil,
+                  &block : Source::Corrector ->) : Issue
+      add_issue rule, location, end_location, message, status, block
     end
 
     # Returns `true` if the list of not disabled issues is empty, `false` otherwise.

src/ameba/rule/base.cr

@@ -112,8 +112,8 @@ module Ameba::Rule
       name.hash
     end
 
-    macro issue_for(*args)
-      source.add_issue self, {{*args}}
+    macro issue_for(*args, &block)
+      source.add_issue self, {{*args}} {{block}}
     end
 
     protected def self.rule_name

src/ameba/source/corrector.cr

@@ -0,0 +1,57 @@
+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)
+      @rewriter = Rewriter.new(code)
+      @line_sizes = code.lines(chomp: false).map(&.size)
+    end
+
+    alias SourceLocation = Crystal::Location | {Int32, Int32}
+
+    def replace(location : SourceLocation, end_location : SourceLocation, content)
+      @rewriter.replace(loc_to_pos(location), loc_to_pos(end_location) + 1, content)
+    end
+
+    def wrap(location : SourceLocation, end_location : SourceLocation, insert_before, insert_after)
+      @rewriter.wrap(loc_to_pos(location), loc_to_pos(end_location) + 1, insert_before, insert_after)
+    end
+
+    def remove(location : SourceLocation, end_location : SourceLocation)
+      @rewriter.remove(loc_to_pos(location), loc_to_pos(end_location) + 1)
+    end
+
+    def insert_before(location : SourceLocation, content)
+      @rewriter.insert_before(loc_to_pos(location), content)
+    end
+
+    def insert_after(location : SourceLocation, content)
+      @rewriter.insert_after(loc_to_pos(location) + 1, content)
+    end
+
+    private def loc_to_pos(location : SourceLocation)
+      if location.is_a?(Crystal::Location)
+        line, column = location.line_number, location.column_number
+      else
+        line, column = location
+      end
+      @line_sizes[0...line - 1].sum + (column - 1)
+    end
+
+    def process
+      @rewriter.process
+    end
+  end
+end

src/ameba/source/rewriter.cr

@@ -0,0 +1,132 @@
+class Ameba::Source
+  # This class performs the heavy lifting in the source rewriting process.
+  # It schedules code updates to be performed in the correct order.
+  #
+  # For simple cases, the resulting source will be obvious.
+  #
+  # Examples for more complex cases follow. Assume these examples are acting on
+  # the source `puts(:hello, :world)`. The methods `#wrap`, `#remove`, etc.
+  # receive a range as the first two arguments; for clarity, examples below use
+  # English sentences and a string of raw code instead.
+  #
+  # ## Overlapping deletions:
+  #
+  # * remove `:hello, `
+  # * remove `, :world`
+  #
+  # The overlapping ranges are merged and `:hello, :world` will be removed.
+  #
+  # ## Multiple actions at the same end points:
+  #
+  # Results will always be independent of the order they were given.
+  # Exception: rewriting actions done on exactly the same range (covered next).
+  #
+  # Example:
+  #
+  # * replace `, ` by ` => `
+  # * wrap `:hello, :world` with `{` and `}`
+  # * replace `:world` with `:everybody`
+  # * wrap `:world` with `[`, `]`
+  #
+  # The resulting string will be `puts({:hello => [:everybody]})`
+  # and this result is independent of the order the instructions were given in.
+  #
+  # ## Multiple wraps on same range:
+  #
+  # * wrap `:hello` with `(` and `)`
+  # * wrap `:hello` with `[` and `]`
+  #
+  # The wraps are combined in order given and results would be `puts([(:hello)], :world)`.
+  #
+  # ## Multiple replacements on same range:
+  #
+  # * replace `:hello` by `:hi`, then
+  # * replace `:hello` by `:hey`
+  #
+  # The replacements are made in the order given, so the latter replacement
+  # supersedes the former and `:hello` will be replaced by `:hey`.
+  #
+  # ## Swallowed insertions:
+  #
+  # * wrap `world` by `__`, `__`
+  # * replace `:hello, :world` with `:hi`
+  #
+  # A containing replacement will swallow the contained rewriting actions
+  # and `:hello, :world` will be replaced by `:hi`.
+  #
+  # ## Implementation
+  #
+  # The updates are organized in a tree, according to the ranges they act on
+  # (where children are strictly contained by their parent).
+  class Rewriter
+    getter code
+
+    def initialize(@code : String)
+      @action_root = Rewriter::Action.new(0, code.size)
+    end
+
+    # Returns true if no (non trivial) update has been recorded
+    def empty?
+      @action_root.empty?
+    end
+
+    # Replaces the code of the given range with `content`.
+    def replace(begin_pos, end_pos, content)
+      combine(begin_pos, end_pos, replacement: content.to_s)
+    end
+
+    # Inserts the given strings before and after the given range.
+    def wrap(begin_pos, end_pos, insert_before, insert_after)
+      combine(begin_pos, end_pos, insert_before: insert_before.to_s, insert_after: insert_after.to_s)
+    end
+
+    # Shortcut for `replace(begin_pos, end_pos, "")`
+    def remove(begin_pos, end_pos)
+      replace(begin_pos, end_pos, "")
+    end
+
+    # Shortcut for `wrap(begin_pos, end_pos, content, nil)`
+    def insert_before(begin_pos, end_pos, content)
+      wrap(begin_pos, end_pos, content, nil)
+    end
+
+    # Shortcut for `wrap(begin_pos, end_pos, nil, content)`
+    def insert_after(begin_pos, end_pos, content)
+      wrap(begin_pos, end_pos, nil, content)
+    end
+
+    # Shortcut for `insert_before(pos, pos, content)`
+    def insert_before(pos, content)
+      insert_before(pos, pos, content)
+    end
+
+    # Shortcut for `insert_after(pos, pos, content)`
+    def insert_after(pos, content)
+      insert_after(pos, pos, content)
+    end
+
+    # Applies all scheduled changes and returns modified source as a new string.
+    def process
+      String.build do |io|
+        last_end = 0
+        @action_root.ordered_replacements.each do |begin_pos, end_pos, replacement|
+          io << code[last_end...begin_pos] << replacement
+          last_end = end_pos
+        end
+        io << code[last_end...code.size]
+      end
+    end
+
+    protected def combine(begin_pos, end_pos, **attributes)
+      check_range_validity(begin_pos, end_pos)
+      action = Rewriter::Action.new(begin_pos, end_pos, **attributes)
+      @action_root = @action_root.combine(action)
+    end
+
+    private def check_range_validity(begin_pos, end_pos)
+      if begin_pos < 0 || end_pos > code.size
+        raise IndexError.new("The range #{begin_pos}...#{end_pos} is outside the bounds of the source")
+      end
+    end
+  end
+end

src/ameba/source/rewriter/action.cr

@@ -0,0 +1,181 @@
+class Ameba::Source::Rewriter
+  # Actions are arranged in a tree and get combined so that:
+  # - children are strictly contained by their parent
+  # - siblings all disjoint from one another and ordered
+  # - only actions with `replacement == nil` may have children
+  class Action
+    getter begin_pos : Int32
+    getter end_pos : Int32
+    getter replacement : String?
+    getter insert_before : String
+    getter insert_after : String
+    protected getter children : Array(Action)
+
+    def initialize(@begin_pos,
+                   @end_pos,
+                   @insert_before = "",
+                   @replacement = nil,
+                   @insert_after = "",
+                   @children = [] of Action)
+    end
+
+    def combine(action)
+      return self if action.empty? # Ignore empty action
+
+      do_combine(action)
+    end
+
+    def empty?
+      replacement = @replacement
+      @insert_before.empty? &&
+        @insert_after.empty? &&
+        @children.empty? &&
+        (replacement.nil? || (replacement.empty? && @begin_pos == @end_pos))
+    end
+
+    def ordered_replacements
+      replacement = @replacement
+      reps = [] of {Int32, Int32, String}
+      reps << {@begin_pos, @begin_pos, @insert_before} unless @insert_before.empty?
+      reps << {@begin_pos, @end_pos, replacement} if replacement
+      reps.concat(@children.flat_map(&.ordered_replacements))
+      reps << {@end_pos, @end_pos, @insert_after} unless @insert_after.empty?
+      reps
+    end
+
+    def insertion?
+      replacement = @replacement
+      !@insert_before.empty? || !@insert_after.empty? || (replacement && !replacement.empty?)
+    end
+
+    protected def with(*,
+                       begin_pos = @begin_pos,
+                       end_pos = @end_pos,
+                       insert_before = @insert_before,
+                       replacement = @replacement,
+                       insert_after = @insert_after,
+                       children = @children)
+      children = [] of Action if replacement
+      self.class.new(begin_pos, end_pos, insert_before, replacement, insert_after, children)
+    end
+
+    # Assumes *action* is contained within `@begin_pos...@end_pos` and has no children
+    protected def do_combine(action)
+      if action.begin_pos == @begin_pos && action.end_pos == @end_pos
+        merge(action)
+      else
+        place_in_hierarchy(action)
+      end
+    end
+
+    protected def place_in_hierarchy(action)
+      family = analyse_hierarchy(action)
+      sibling_left, sibling_right = family[:sibling_left], family[:sibling_right]
+
+      if (fusible = family[:fusible])
+        child = family[:child]
+        child ||= [] of Action
+        fuse_deletions(action, fusible, sibling_left + child + sibling_right)
+      else
+        extra_sibling = if (parent = family[:parent])
+                          # action should be a descendant of one of the children
+                          parent.do_combine(action)
+                        elsif (child = family[:child])
+                          # or it should become the parent of some of the children,
+                          action.with(children: child).combine_children(action.children)
+                        else
+                          # or else it should become an additional child
+                          action
+                        end
+        self.with(children: sibling_left + [extra_sibling] + sibling_right)
+      end
+    end
+
+    # Assumes *more_children* all contained within `@begin_pos...@end_pos`
+    protected def combine_children(more_children)
+      more_children.reduce(self) do |parent, new_child|
+        parent.place_in_hierarchy(new_child)
+      end
+    end
+
+    protected def fuse_deletions(action, fusible, other_siblings)
+      without_fusible = self.with(children: other_siblings)
+      fusible = [action] + fusible
+      fused_begin_pos = fusible.map(&.begin_pos).min
+      fused_end_pos = fusible.map(&.end_pos).max
+      fused_deletion = action.with(begin_pos: fused_begin_pos, end_pos: fused_end_pos)
+      without_fusible.do_combine(fused_deletion)
+    end
+
+    # Similar to `@children.bsearch_index || size` except allows for a starting point
+    protected def bsearch_child_index(from = 0)
+      size = @children.size
+      (from...size).bsearch { |i| yield @children[i] } || size
+    end
+
+    # Returns the children in a hierarchy with respect to *action*:
+    #
+    # - `:sibling_left`, `:sibling_right` (for those that are disjoint from *action*)
+    # - `:parent` (in case one of our children contains *action*)
+    # - `:child` (in case *action* strictly contains some of our children)
+    # - `:fusible` (in case *action* overlaps some children but they can be fused in one deletion)
+    #
+    # 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
+      # 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
+      start = left_index == 0 ? 0 : left_index - 1 # See "corner case" below for reason of -1
+      right_index = bsearch_child_index(start) { |child| child.begin_pos >= action.end_pos }
+      center = right_index - left_index
+      case center
+      when 0
+        # All children are disjoint from action, nothing else to do
+      when -1
+        # Corner case: if a child has empty range == action's range
+        # then it will appear to be both disjoint and to the left of action,
+        # as well as disjoint and to the right of action.
+        # Since ranges are equal, we return it as parent
+        left_index -= 1  # Fix indices, as otherwise this child would be
+        right_index += 1 # considered as a sibling (both left and right!)
+        parent = @children[left_index]
+      else
+        overlap_left = @children[left_index].begin_pos <=> action.begin_pos
+        overlap_right = @children[right_index - 1].end_pos <=> action.end_pos
+        raise "Unable to compare begin pos" if overlap_left.nil?
+        raise "Unable to compare end pos" if overlap_right.nil?
+
+        # For one child to be the parent of action, we must have:
+        if center == 1 && overlap_left <= 0 && overlap_right >= 0
+          parent = @children[left_index]
+        else
+          # Otherwise consider all non disjoint elements (center) to be contained...
+          contained = @children[left_index...right_index]
+          fusible = [] of Action
+          fusible << contained.shift if overlap_left < 0 # ... but check first and last one
+          fusible << contained.pop if overlap_right > 0  # ... for overlaps
+          fusible = nil if fusible.empty?
+        end
+      end
+
+      {
+        parent:        parent,
+        sibling_left:  @children[0...left_index],
+        sibling_right: @children[right_index...@children.size],
+        fusible:       fusible,
+        child:         contained,
+      }
+    end
+
+    # Assumes *action* has the exact same range and has no children
+    protected def merge(action)
+      self.with(
+        insert_before: "#{action.insert_before}#{insert_before}",
+        replacement: action.replacement || @replacement,
+        insert_after: "#{insert_after}#{action.insert_after}",
+      ).combine_children(action.children)
+    end
+  end
+end