Location, error formatting and stack trace improvements (google#59)

* Location, error formatting and stack trace improvements

* Static context for AST nodes
* Thunks no longer need `name`
* Prototype for showing snippets in error messages (old format still
available)
* Use ast.Function to represent methods and local function sugar.
* Change tests so that the error output is pretty

Author: sbarzowski

ast/ast.go

@@ -30,17 +30,22 @@ type Identifiers []Identifier
 
 // ---------------------------------------------------------------------------
 
+type Context *string
+
 type Node interface {
+	Context() Context
 	Loc() *LocationRange
 	FreeVariables() Identifiers
 	SetFreeVariables(Identifiers)
+	SetContext(Context)
 }
 type Nodes []Node
 
 // ---------------------------------------------------------------------------
 
 type NodeBase struct {
 	loc           LocationRange
+	context       Context
 	freeVariables Identifiers
 }
 
@@ -70,6 +75,14 @@ func (n *NodeBase) SetFreeVariables(idents Identifiers) {
 	n.freeVariables = idents
 }
 
+func (n *NodeBase) Context() Context {
+	return n.context
+}
+
+func (n *NodeBase) SetContext(context Context) {
+	n.context = context
+}
+
 // ---------------------------------------------------------------------------
 
 type IfSpec struct {
@@ -356,11 +369,9 @@ type Slice struct {
 
 // LocalBind is a helper struct for astLocal
 type LocalBind struct {
-	Variable      Identifier
-	Body          Node
-	FunctionSugar bool
-	Params        *Parameters // if functionSugar is true
-	TrailingComma bool
+	Variable Identifier
+	Body     Node
+	Fun      *Function
 }
 type LocalBinds []LocalBind
 
@@ -439,7 +450,8 @@ type ObjectField struct {
 	Hide          ObjectFieldHide // (ignore if kind != astObjectField*)
 	SuperSugar    bool            // +:  (ignore if kind != astObjectField*)
 	MethodSugar   bool            // f(x, y, z): ...  (ignore if kind  == astObjectAssert)
-	Expr1         Node            // Not in scope of the object
+	Method        *Function
+	Expr1         Node // Not in scope of the object
 	Id            *Identifier
 	Params        *Parameters // If methodSugar == true then holds the params.
 	TrailingComma bool        // If methodSugar == true then remembers the trailing comma
@@ -448,12 +460,8 @@ type ObjectField struct {
 
 // TODO(jbeda): Add the remaining constructor helpers here
 
-func ObjectFieldLocal(methodSugar bool, id *Identifier, params *Parameters, trailingComma bool, body Node) ObjectField {
-	return ObjectField{ObjectLocal, ObjectFieldVisible, false, methodSugar, nil, id, params, trailingComma, body, nil}
-}
-
 func ObjectFieldLocalNoMethod(id *Identifier, body Node) ObjectField {
-	return ObjectField{ObjectLocal, ObjectFieldVisible, false, false, nil, id, nil, false, body, nil}
+	return ObjectField{ObjectLocal, ObjectFieldVisible, false, false, nil, nil, id, nil, false, body, nil}
 }
 
 type ObjectFields []ObjectField

ast/location.go

@@ -16,14 +16,22 @@ limitations under the License.
 
 package ast
 
-import "fmt"
+import (
+	"bytes"
+	"fmt"
+)
+
+type Source struct {
+	lines []string
+}
 
 //////////////////////////////////////////////////////////////////////////////
 // Location
 
 // Location represents a single location in an (unspecified) file.
 type Location struct {
-	Line   int
+	Line int
+	// Column is a byte offset from the beginning of the line
 	Column int
 }
 
@@ -36,14 +44,29 @@ func (l *Location) String() string {
 	return fmt.Sprintf("%v:%v", l.Line, l.Column)
 }
 
+func locationBefore(a Location, b Location) bool {
+	if a.Line != b.Line {
+		return a.Line < b.Line
+	}
+	return a.Column < b.Column
+}
+
 //////////////////////////////////////////////////////////////////////////////
 // LocationRange
 
 // LocationRange represents a range of a source file.
 type LocationRange struct {
 	FileName string
 	Begin    Location
-	End      Location
+	End      Location // TODO(sbarzowski) inclusive? exclusive? a gap?
+	file     *Source
+}
+
+func LocationRangeBetween(a, b *LocationRange) LocationRange {
+	if a.file != b.file {
+		panic("Cannot create a LocationRange between different files")
+	}
+	return MakeLocationRange(a.FileName, a.file, a.Begin, b.End)
 }
 
 // IsSet returns if this LocationRange has been set.
@@ -70,11 +93,101 @@ func (lr *LocationRange) String() string {
 	return fmt.Sprintf("%s(%v)-(%v)", filePrefix, lr.Begin.String(), lr.End.String())
 }
 
+func (l *LocationRange) WithCode() bool {
+	return l.Begin.Line != 0
+}
+
 // This is useful for special locations, e.g. manifestation entry point.
 func MakeLocationRangeMessage(msg string) LocationRange {
 	return LocationRange{FileName: msg}
 }
 
-func MakeLocationRange(fn string, begin Location, end Location) LocationRange {
-	return LocationRange{FileName: fn, Begin: begin, End: end}
+func MakeLocationRange(fn string, fc *Source, begin Location, end Location) LocationRange {
+	return LocationRange{FileName: fn, file: fc, Begin: begin, End: end}
+}
+
+type SourceProvider struct {
+}
+
+func (sp *SourceProvider) GetSnippet(loc LocationRange) string {
+	var result bytes.Buffer
+	if loc.Begin.Line == 0 {
+		return ""
+	}
+	for i := loc.Begin.Line; i <= loc.End.Line; i++ {
+		inLineRange := trimToLine(loc, i)
+		for j := inLineRange.Begin.Column; j < inLineRange.End.Column; j++ {
+			result.WriteByte(loc.file.lines[i-1][j-1])
+		}
+		if i != loc.End.Line {
+			result.WriteByte('\n')
+		}
+	}
+	return result.String()
+}
+
+func BuildSource(s string) *Source {
+	var result []string
+	var lineBuf bytes.Buffer
+	for _, runeValue := range s {
+		lineBuf.WriteRune(runeValue)
+		if runeValue == '\n' {
+			result = append(result, lineBuf.String())
+			lineBuf.Reset()
+		}
+	}
+	rest := lineBuf.String()
+	// Stuff after last end-of-line (EOF or some more code)
+	result = append(result, rest+"\n")
+	return &Source{result}
+}
+
+func trimToLine(loc LocationRange, line int) LocationRange {
+	if loc.Begin.Line > line {
+		panic("invalid")
+	}
+	if loc.Begin.Line != line {
+		loc.Begin.Column = 1
+	}
+	loc.Begin.Line = line
+	if loc.End.Line < line {
+		panic("invalid")
+	}
+	if loc.End.Line != line {
+		loc.End.Column = len(loc.file.lines[line-1])
+	}
+	loc.End.Line = line
+	return loc
+}
+
+// lineBeginning returns a part of the line directly before LocationRange
+// for example:
+// local x = foo()
+//           ^^^^^ <- LocationRange loc
+// then
+// local x = foo()
+// ^^^^^^^^^^ <- lineBeginning(loc)
+func LineBeginning(loc *LocationRange) LocationRange {
+	return LocationRange{
+		Begin:    Location{Line: loc.Begin.Line, Column: 1},
+		End:      loc.Begin,
+		FileName: loc.FileName,
+		file:     loc.file,
+	}
+}
+
+// lineEnding returns a part of the line directly after LocationRange
+// for example:
+// local x = foo() + test
+//           ^^^^^ <- LocationRange loc
+// then
+// local x = foo() + test
+//                ^^^^^^^ <- lineEnding(loc)
+func LineEnding(loc *LocationRange) LocationRange {
+	return LocationRange{
+		Begin:    loc.End,
+		End:      Location{Line: loc.End.Line, Column: len(loc.file.lines[loc.End.Line-1])},
+		FileName: loc.FileName,
+		file:     loc.file,
+	}
 }

builtins.go

@@ -608,7 +608,7 @@ type UnaryBuiltin struct {
 
 func getBuiltinEvaluator(e *evaluator, name ast.Identifier) *evaluator {
 	loc := ast.MakeLocationRangeMessage("<builtin>")
-	context := TraceContext{Name: "builtin function <" + string(name) + ">"}
+	context := "builtin function <" + string(name) + ">"
 	trace := TraceElement{loc: &loc, context: &context}
 	return &evaluator{i: e.i, trace: &trace}
 }

desugarer.go

@@ -128,22 +128,14 @@ func desugarFields(location ast.LocationRange, fields *ast.ObjectFields, objLeve
 		field.Expr2 = assertion
 	}
 
-	// Remove methods
 	for i := range *fields {
 		field := &((*fields)[i])
-		if !field.MethodSugar {
+		if field.Method == nil {
 			continue
 		}
-		origBody := field.Expr2
-		function := &ast.Function{
-			// TODO(sbarzowski) better location
-			NodeBase:   ast.NewNodeBaseLoc(*origBody.Loc()),
-			Parameters: *field.Params,
-			Body:       origBody,
-		}
-		field.MethodSugar = false
-		field.Params = nil
-		field.Expr2 = function
+		field.Expr2 = field.Method
+		field.Method = nil
+		// Body of the function already desugared through expr2
 	}
 
 	// Remove object-level locals
@@ -483,19 +475,11 @@ func desugar(astPtr *ast.Node, objLevel int) (err error) {
 
 	case *ast.Local:
 		for i := range node.Binds {
-			if node.Binds[i].FunctionSugar {
-				origBody := node.Binds[i].Body
-				function := &ast.Function{
-					// TODO(sbarzowski) better location
-					NodeBase:   ast.NewNodeBaseLoc(*origBody.Loc()),
-					Parameters: *node.Binds[i].Params,
-					Body:       origBody,
-				}
+			if node.Binds[i].Fun != nil {
 				node.Binds[i] = ast.LocalBind{
-					Variable:      node.Binds[i].Variable,
-					Body:          function,
-					FunctionSugar: false,
-					Params:        nil,
+					Variable: node.Binds[i].Variable,
+					Body:     node.Binds[i].Fun,
+					Fun:      nil,
 				}
 			}
 			err = desugar(&node.Binds[i].Body, objLevel)

error_formatter.go

@@ -19,6 +19,7 @@ import (
 	"bytes"
 	"fmt"
 
+	"github.com/fatih/color"
 	"github.com/google/go-jsonnet/ast"
 	"github.com/google/go-jsonnet/parser"
 )
@@ -27,10 +28,13 @@ type ErrorFormatter struct {
 	// TODO(sbarzowski) use this
 	// MaxStackTraceSize  is the maximum length of stack trace before cropping
 	MaxStackTraceSize int
-	// TODO(sbarzowski) use these
+
+	// Examples of current state of the art.
+	// http://elm-lang.org/blog/compiler-errors-for-humans
+	// https://clang.llvm.org/diagnostics.html
 	pretty   bool
 	colorful bool
-	SP       SourceProvider
+	SP       *ast.SourceProvider
 }
 
 func (ef *ErrorFormatter) format(err error) string {
@@ -46,12 +50,13 @@ func (ef *ErrorFormatter) format(err error) string {
 
 func (ef *ErrorFormatter) formatRuntime(err *RuntimeError) string {
 	return err.Error() + "\n" + ef.buildStackTrace(err.StackTrace)
-	// TODO(sbarzowski) pretty stuff
 }
 
 func (ef *ErrorFormatter) formatStatic(err *parser.StaticError) string {
-	return err.Error() + "\n"
-	// TODO(sbarzowski) pretty stuff
+	var buf bytes.Buffer
+	buf.WriteString(err.Error() + "\n")
+	ef.showCode(&buf, err.Loc)
+	return buf.String()
 }
 
 const bugURL = "https://github.com/google/go-jsonnet/issues"
@@ -61,19 +66,42 @@ func (ef *ErrorFormatter) formatInternal(err error) string {
 		"Please report a bug here: " + bugURL + "\n"
 }
 
+func (ef *ErrorFormatter) showCode(buf *bytes.Buffer, loc ast.LocationRange) {
+	errFprintf := fmt.Fprintf
+	if ef.colorful {
+		errFprintf = color.New(color.FgRed).Fprintf
+	}
+	if loc.WithCode() {
+		// TODO(sbarzowski) include line numbers
+		// TODO(sbarzowski) underline errors instead of depending only on color
+		fmt.Fprintf(buf, "\n")
+		beginning := ast.LineBeginning(&loc)
+		ending := ast.LineEnding(&loc)
+		fmt.Fprintf(buf, "%v", ef.SP.GetSnippet(beginning))
+		errFprintf(buf, "%v", ef.SP.GetSnippet(loc))
+		fmt.Fprintf(buf, "%v", ef.SP.GetSnippet(ending))
+		buf.WriteByte('\n')
+	}
+	fmt.Fprintf(buf, "\n")
+}
+
 func (ef *ErrorFormatter) buildStackTrace(frames []TraceFrame) string {
 	// https://github.com/google/jsonnet/blob/master/core/libjsonnet.cpp#L594
 	var buf bytes.Buffer
-	for _, f := range frames {
+	for i := len(frames) - 1; i >= 0; i-- {
+		f := frames[i]
+		// TODO(sbarzowski) make pretty format more readable (it's already useful)
+		if ef.pretty {
+			fmt.Fprintf(&buf, "-------------------------------------------------\n")
+		}
+		// TODO(sbarzowski) tabs are probably a bad idea
 		fmt.Fprintf(&buf, "\t%v\t%v\n", &f.Loc, f.Name)
+		if ef.pretty {
+			ef.showCode(&buf, f.Loc)
+		}
+
 		// TODO(sbarzowski) handle max stack trace size
 		// TODO(sbarzowski) I think the order of frames is reversed
 	}
 	return buf.String()
 }
-
-type SourceProvider interface {
-	// TODO(sbarzowski) problem: locationRange.FileName may not necessarily
-	// uniquely identify a file. But this is the interface we want to have here.
-	getCode(ast.LocationRange) string
-}