Better error messages in the CLI

Author: kddnewton

lib/syntax_tree.rb

@@ -26,12 +26,14 @@ class SyntaxTree < Ripper
   # every character in the string is 1 byte in length, so we can just return the
   # start of the line + the index.
   class SingleByteString
+    attr_reader :start
+
     def initialize(start)
       @start = start
     end
 
     def [](byteindex)
-      @start + byteindex
+      start + byteindex
     end
   end
 
@@ -40,7 +42,10 @@ def [](byteindex)
   # an array of indices, such that array[byteindex] will be equal to the index
   # of the character within the string.
   class MultiByteString
+    attr_reader :start, :indices
+
     def initialize(start, line)
+      @start = start
       @indices = []
 
       line.each_char.with_index(start) do |char, index|
@@ -52,7 +57,7 @@ def initialize(start, line)
     # there's a BOM at the beginning of the file, which is the reason we need to
     # compare it to 0 here.
     def [](byteindex)
-      @indices[byteindex < 0 ? 0 : byteindex]
+      indices[byteindex < 0 ? 0 : byteindex]
     end
   end
 
@@ -186,6 +191,10 @@ def parents
   # [Array[ String ]] the list of lines in the source
   attr_reader :lines
 
+  # [Array[ SingleByteString | MultiByteString ]] the list of objects that
+  # represent the start of each line in character offsets
+  attr_reader :line_counts
+
   # [Array[ untyped ]] a running list of tokens that have been found in the
   # source. This list changes a lot as certain nodes will "consume" these tokens
   # to determine their bounds.
@@ -299,7 +308,7 @@ def self.format(source)
   # this line, then we add the number of columns into this line that we've gone
   # through.
   def char_pos
-    @line_counts[lineno - 1][column]
+    line_counts[lineno - 1][column]
   end
 
   # As we build up a list of tokens, we'll periodically need to go backwards and
@@ -313,7 +322,7 @@ def char_pos
   # (which would happen to be the innermost keyword). Then the outer one would
   # only be able to grab the first one. In this way all of the tokens act as
   # their own stack.
-  def find_token(type, value = :any, consume: true)
+  def find_token(type, value = :any, consume: true, location: nil)
     index =
       tokens.rindex do |token|
         token.is_a?(type) && (value == :any || (token.value == value))
@@ -326,8 +335,16 @@ def find_token(type, value = :any, consume: true)
       # could also be caused by accidentally attempting to consume a token twice
       # by two different parser event handlers.
       unless index
-        message = "Cannot find expected #{value == :any ? type : value}"
-        raise ParseError.new(message, lineno, column)
+        token = value == :any ? type.name.split("::", 2).last : value
+        message = "Cannot find expected #{token}"
+
+        if location
+          lineno = location.start_line
+          column = location.start_char - line_counts[lineno - 1].start
+          raise ParseError.new(message, lineno, column)
+        else
+          raise ParseError.new(message, lineno, column)
+        end
       end
 
       tokens.delete_at(index)
@@ -1552,7 +1569,8 @@ def on_array(contents)
         location: lbracket.location.to(rbracket.location)
       )
     else
-      tstring_end = find_token(TStringEnd)
+      tstring_end =
+        find_token(TStringEnd, location: contents.beginning.location)
 
       contents.class.new(
         beginning: contents.beginning,
@@ -5195,7 +5213,7 @@ def on_dyna_symbol(string_content)
     if find_token(SymBeg, consume: false)
       # A normal dynamic symbol
       symbeg = find_token(SymBeg)
-      tstring_end = find_token(TStringEnd)
+      tstring_end = find_token(TStringEnd, location: symbeg.location)
 
       DynaSymbol.new(
         quote: symbeg.value,
@@ -11291,7 +11309,7 @@ def on_string_literal(string)
       )
     else
       tstring_beg = find_token(TStringBeg)
-      tstring_end = find_token(TStringEnd)
+      tstring_end = find_token(TStringEnd, location: tstring_beg.location)
 
       location =
         Location.new(
@@ -13503,7 +13521,7 @@ def on_xstring_literal(xstring)
         location: heredoc.location
       )
     else
-      ending = find_token(TStringEnd)
+      ending = find_token(TStringEnd, location: xstring.location)
 
       XStringLiteral.new(
         parts: xstring.parts,

lib/syntax_tree/cli.rb

@@ -3,18 +3,28 @@
 class SyntaxTree
   module CLI
     # A utility wrapper around colored strings in the output.
-    class ColoredString
-      COLORS = { default: "0", gray: "38;5;102", yellow: "33" }
+    class Color
+      attr_reader :value, :code
 
-      attr_reader :code, :string
-
-      def initialize(color, string)
-        @code = COLORS[color]
-        @string = string
+      def initialize(value, code)
+        @value = value
+        @code = code
       end
 
       def to_s
-        "\033[#{code}m#{string}\033[0m"
+        "\033[#{code}m#{value}\033[0m"
+      end
+
+      def self.gray(value)
+        new(value, "38;5;102")
+      end
+
+      def self.red(value)
+        new(value, "1;31")
+      end
+
+      def self.yellow(value)
+        new(value, "33")
       end
     end
 
@@ -37,23 +47,52 @@ def run(filepath, source)
       end
     end
 
+    # An action of the CLI that ensures that the filepath is formatted as
+    # expected.
+    class Check < Action
+      class UnformattedError < StandardError
+      end
+
+      def run(filepath, source)
+        raise UnformattedError if source != SyntaxTree.format(source)
+      rescue
+        warn("[#{Color.yellow("warn")}] #{filepath}")
+        raise
+      end
+
+      def success
+        puts("All files matched expected format.")
+      end
+
+      def failure
+        warn("The listed files did not match the expected format.")
+      end
+    end
+
     # An action of the CLI that formats the source twice to check if the first
     # format is not idempotent.
-    class Check < Action
+    class Debug < Action
+      class NonIdempotentFormatError < StandardError
+      end
+
       def run(filepath, source)
+        warning = "[#{Color.yellow("warn")}] #{filepath}"
         formatted = SyntaxTree.format(source)
-        return true if formatted == SyntaxTree.format(formatted)
 
-        puts "[#{ColoredString.new(:yellow, "warn")}] #{filepath}"
-        false
+        if formatted != SyntaxTree.format(formatted)
+          raise NonIdempotentFormatError
+        end
+      rescue
+        warn(warning)
+        raise
       end
 
       def success
-        puts "All files matched expected format."
+        puts("All files can be formatted idempotently.")
       end
 
       def failure
-        warn("The listed files did not match the expected format.")
+        warn("The listed files could not be formatted idempotently.")
       end
     end
 
@@ -83,10 +122,10 @@ def run(filepath, source)
         formatted = SyntaxTree.format(source)
         File.write(filepath, formatted)
 
+        color = source == formatted ? Color.gray(filepath) : filepath
         delta = ((Time.now - start) * 1000).round
-        color = source == formatted ? :gray : :default
 
-        puts "\r#{ColoredString.new(color, filepath)} #{delta}ms"
+        puts "\r#{color} #{delta}ms"
       end
     end
 
@@ -95,7 +134,7 @@ def run(filepath, source)
     HELP = <<~HELP
       stree MODE FILE
 
-      MODE: ast | check | doc | format | write
+      MODE: ast | check | debug | doc | format | write
       FILE: one or more paths to files to parse
     HELP
 
@@ -115,7 +154,9 @@ def run(argv)
             AST.new
           when "c", "check"
             Check.new
-          when "d", "doc"
+          when "debug"
+            Debug.new
+          when "doc"
             Doc.new
           when "f", "format"
             Format.new
@@ -130,11 +171,37 @@ def run(argv)
         patterns.each do |pattern|
           Dir.glob(pattern).each do |filepath|
             next unless File.file?(filepath)
+            source = source_for(filepath)
 
             begin
-              action.run(filepath, source_for(filepath))
+              action.run(filepath, source)
+            rescue ParseError => error
+              warn("Error: #{error.message}")
+              lines = source.lines
+
+              maximum = [error.lineno + 3, lines.length].min
+              digits = Math.log10(maximum).ceil
+
+              ([error.lineno - 3, 0].max...maximum).each do |line_index|
+                line_number = line_index + 1
+
+                if line_number == error.lineno
+                  part1 = Color.red(">")
+                  part2 = Color.gray("%#{digits}d |" % line_number)
+                  warn("#{part1} #{part2} #{lines[line_index]}")
+
+                  part3 = Color.gray("  %#{digits}s |" % " ")
+                  warn("#{part3} #{" " * error.column}#{Color.red("^")}")
+                else
+                  prefix = Color.gray("  %#{digits}d |" % line_number)
+                  warn("#{prefix} #{lines[line_index]}")
+                end
+              end
+
+              errored = true
+            rescue Check::UnformattedError, Debug::NonIdempotentFormatError
+              errored = true
             rescue => error
-              warn("!!! Failed on #{filepath}")
               warn(error.message)
               warn(error.backtrace)
               errored = true

lib/syntax_tree/prettyprint.rb

@@ -751,9 +751,9 @@ def flush
     # This is a special sort used to order the line suffixes by both the
     # priority set on the line suffix and the index it was in the original
     # array.
-    line_suffix_sort = ->(line_suffix) {
+    line_suffix_sort = ->(line_suffix) do
       [-line_suffix.last, -line_suffixes.index(line_suffix)]
-    }
+    end
 
     # This is a linear stack instead of a mutually recursive call defined on
     # the individual doc nodes for efficiency.