Preparation for linter.

* Extract some test utilities to a separate package.
* Rename some test utilities.
* Internally expose DirectChildren.
* Add LocationRange to some non-expr AST parts,
  such as local binds, parameters and object fields.
* Add end-of-file-reached testcases.

BUILD.bazel

@@ -45,6 +45,6 @@ go_test(
     deps = [
         "//ast:go_default_library",
         "//internal/parser:go_default_library",
-        "@com_github_sergi_go_diff//diffmatchpatch:go_default_library",
+        "//internal/testutils:go_default_library",
     ],
 )

ast/ast.go

@@ -394,6 +394,7 @@ type Parameter struct {
 	EqFodder    Fodder
 	DefaultArg  Node
 	CommaFodder Fodder
+	LocRange    LocationRange
 }
 
 // CommaSeparatedID represents an expression that is an element of a
@@ -468,6 +469,8 @@ type LocalBind struct {
 	Fun *Function
 	// The fodder before the closing ',' or ';' (whichever it is)
 	CloseFodder Fodder
+
+	LocRange LocationRange
 }
 
 // LocalBinds represents a LocalBind slice.
@@ -594,15 +597,17 @@ type ObjectField struct {
 	OpFodder     Fodder
 	Expr2, Expr3 Node // In scope of the object (can see self).
 	CommaFodder  Fodder
+	LocRange     LocationRange
 }
 
 // ObjectFieldLocalNoMethod creates a non-method local object field.
-func ObjectFieldLocalNoMethod(id *Identifier, body Node) ObjectField {
+func ObjectFieldLocalNoMethod(id *Identifier, body Node, loc LocationRange) ObjectField {
 	return ObjectField{
 		Kind:     ObjectLocal,
 		Hide:     ObjectFieldVisible,
 		Id:       id,
 		Expr2:    body,
+		LocRange: loc,
 	}
 }
 
@@ -628,6 +633,8 @@ type DesugaredObjectField struct {
 	Name      Node
 	Body      Node
 	PlusSuper bool
+
+	LocRange LocationRange
 }
 
 // DesugaredObjectFields represents a DesugaredObjectField slice.

internal/parser/context.go

@@ -30,14 +30,14 @@ const anonymous = "anonymous"
 // package or a separate internal astutils package. The only reason I'm not doing it
 // right now is that it's a pretty invasive change that deserves a separate PR.
 
-// directChildren are children of AST node that are executed in the same context
+// DirectChildren are children of AST node that are executed in the same context
 // and environment as their parent. It supports ASTs before and after desugaring.
 //
 // They must satisfy the following rules:
 // * (no-delayed-evaluation) They are evaluated when their parent is evaluated or never.
 // * (no-indirect-evaluation) They cannot be evaluated during evaluation of any non-direct children
 // * (same-environment) They must be evaluated in the same environment as their parent
-func directChildren(node ast.Node) []ast.Node {
+func DirectChildren(node ast.Node) []ast.Node {
 	switch node := node.(type) {
 	case *ast.Apply:
 		return []ast.Node{node.Target}
@@ -354,7 +354,7 @@ func specialChildren(node ast.Node) []ast.Node {
 // Children returns all children of a node. It supports ASTs before and after desugaring.
 func Children(node ast.Node) []ast.Node {
 	var result []ast.Node
-	result = append(result, directChildren(node)...)
+	result = append(result, DirectChildren(node)...)
 	result = append(result, thunkChildren(node)...)
 	result = append(result, specialChildren(node)...)
 	return result
@@ -400,7 +400,7 @@ func addContext(node ast.Node, context *string, bind string) {
 	case *ast.Object:
 		// TODO(sbarzowski) include fieldname, maybe even chains
 
-		outOfObject := directChildren(node)
+		outOfObject := DirectChildren(node)
 		for _, f := range outOfObject {
 			// This actually is evaluated outside of object
 			addContext(f, context, anonymous)
@@ -414,7 +414,7 @@ func addContext(node ast.Node, context *string, bind string) {
 		}
 
 	case *ast.ObjectComp:
-		outOfObject := directChildren(node)
+		outOfObject := DirectChildren(node)
 		for _, f := range outOfObject {
 			// This actually is evaluated outside of object
 			addContext(f, context, anonymous)
@@ -438,7 +438,7 @@ func addContext(node ast.Node, context *string, bind string) {
 		}
 		addContext(node.Body, context, bind)
 	default:
-		for _, child := range directChildren(node) {
+		for _, child := range DirectChildren(node) {
 			addContext(child, context, anonymous)
 		}
 

internal/parser/parser.go

@@ -209,6 +209,7 @@ func (p *parser) parseParameter() (ast.Parameter, errors.StaticError) {
 	}
 	ret.Name = ast.Identifier(ident.data)
 	ret.NameFodder = ident.fodder
+	ret.LocRange = ident.loc
 	if p.peek().kind == tokenOperator && p.peek().data == "=" {
 		eq := p.pop()
 		ret.EqFodder = eq.fodder
@@ -216,6 +217,7 @@ func (p *parser) parseParameter() (ast.Parameter, errors.StaticError) {
 		if err != nil {
 			return ret, err
 		}
+		ret.LocRange = locFromTokenAST(ident, ret.DefaultArg)
 	}
 	return ret, nil
 }
@@ -312,6 +314,7 @@ func (p *parser) parseBind(binds *ast.LocalBinds) (*token, errors.StaticError) {
 			Body:        body,
 			Fun:         fun,
 			CloseFodder: delim.fodder,
+			LocRange:    locFromTokenAST(varID, body),
 		})
 	} else {
 		*binds = append(*binds, ast.LocalBind{
@@ -320,6 +323,7 @@ func (p *parser) parseBind(binds *ast.LocalBinds) (*token, errors.StaticError) {
 			EqFodder:    eqToken.fodder,
 			Body:        body,
 			CloseFodder: delim.fodder,
+			LocRange:    locFromTokenAST(varID, body),
 		})
 	}
 
@@ -505,6 +509,7 @@ func (p *parser) parseObjectRemainderField(literalFields *LiteralFieldSet, tok *
 		OpFodder:    opFodder,
 		Expr2:       body,
 		CommaFodder: commaFodder,
+		LocRange:    locFromTokenAST(next, body),
 	}, nil
 }
 
@@ -575,6 +580,7 @@ func (p *parser) parseObjectRemainderLocal(binds *ast.IdentifierSet, tok *token,
 		OpFodder:    opToken.fodder,
 		Expr2:       body,
 		CommaFodder: commaFodder,
+		LocRange:    locFromTokenAST(varID, body),
 	}, nil
 }
 
@@ -583,6 +589,7 @@ func (p *parser) parseObjectRemainderAssert(tok *token, next *token) (*ast.Objec
 	if err != nil {
 		return nil, err
 	}
+	lastAST := cond // for determining location
 	var msg ast.Node
 	var colonFodder ast.Fodder
 	if p.peek().kind == tokenOperator && p.peek().data == ":" {
@@ -592,6 +599,7 @@ func (p *parser) parseObjectRemainderAssert(tok *token, next *token) (*ast.Objec
 		if err != nil {
 			return nil, err
 		}
+		lastAST = msg
 	}
 
 	var commaFodder ast.Fodder
@@ -607,6 +615,7 @@ func (p *parser) parseObjectRemainderAssert(tok *token, next *token) (*ast.Objec
 		OpFodder:    colonFodder,
 		Expr3:       msg,
 		CommaFodder: commaFodder,
+		LocRange:    locFromTokenAST(next, lastAST),
 	}, nil
 }
 

internal/program/desugarer.go

@@ -64,6 +64,9 @@ func desugarFields(nodeBase ast.NodeBase, fields *ast.ObjectFields, objLevel int
 			}
 			onFailure := &ast.Error{Expr: msg}
 			asserts = append(asserts, &ast.Conditional{
+				NodeBase: ast.NodeBase{
+					LocRange: field.LocRange,
+				},
 				Cond:        field.Expr2,
 				BranchTrue:  &ast.LiteralBoolean{Value: true}, // ignored anyway
 				BranchFalse: onFailure,
@@ -74,6 +77,7 @@ func desugarFields(nodeBase ast.NodeBase, fields *ast.ObjectFields, objLevel int
 				Name:      makeStr(string(*field.Id)),
 				Body:      field.Expr2,
 				PlusSuper: field.SuperSugar,
+				LocRange:  field.LocRange,
 			})
 
 		case ast.ObjectFieldExpr, ast.ObjectFieldStr:
@@ -82,12 +86,14 @@ func desugarFields(nodeBase ast.NodeBase, fields *ast.ObjectFields, objLevel int
 				Name:      field.Expr1,
 				Body:      field.Expr2,
 				PlusSuper: field.SuperSugar,
+				LocRange:  field.LocRange,
 			})
 
 		case ast.ObjectLocal:
 			locals = append(locals, ast.LocalBind{
 				Variable: *field.Id,
 				Body:     ast.Clone(field.Expr2), // TODO(sbarzowski) not sure if clone is needed
+				LocRange: field.LocRange,
 			})
 		default:
 			panic(fmt.Sprintf("Unexpected object field kind %v", field.Kind))

internal/testutils/BUILD.bazel

@@ -0,0 +1,9 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "go_default_library",
+    srcs = ["test_utils.go"],
+    importpath = "github.com/google/go-jsonnet/internal/testutils",
+    visibility = ["//:__subpackages__"],
+    deps = ["@com_github_sergi_go_diff//diffmatchpatch:go_default_library"],
+)

internal/testutils/test_utils.go

@@ -0,0 +1,45 @@
+package testutils
+
+import (
+	"bytes"
+	"io/ioutil"
+	"os"
+
+	"github.com/sergi/go-diff/diffmatchpatch"
+)
+
+// Diff produces a pretty diff of two files
+func Diff(a, b string) string {
+	dmp := diffmatchpatch.New()
+	diffs := dmp.DiffMain(a, b, false)
+	return dmp.DiffPrettyText(diffs)
+}
+
+// CompareWithGolden check if a file is the same as golden file.
+// If it is not it produces a pretty diff.
+func CompareWithGolden(result string, golden []byte) (string, bool) {
+	if !bytes.Equal(golden, []byte(result)) {
+		// TODO(sbarzowski) better reporting of differences in whitespace
+		// missing newline issues can be very subtle now
+		return Diff(result, string(golden)), true
+	}
+	return "", false
+}
+
+// UpdateGoldenFile updates a golden file with new contents if the new contents
+// are actually different from what is already there. It returns whether or not
+// the overwrite was performed (i.e. the desired content was different than actual).
+func UpdateGoldenFile(path string, content []byte, mode os.FileMode) (changed bool, err error) {
+	old, err := ioutil.ReadFile(path)
+	if err != nil && !os.IsNotExist(err) {
+		return false, err
+	}
+	// If it exists and already has the right content, do nothing,
+	if bytes.Equal(old, content) && !os.IsNotExist(err) {
+		return false, nil
+	}
+	if err := ioutil.WriteFile(path, content, mode); err != nil {
+		return false, err
+	}
+	return true, nil
+}

main_test.go

@@ -32,7 +32,7 @@ import (
 
 	"github.com/google/go-jsonnet/ast"
 	"github.com/google/go-jsonnet/internal/parser"
-	"github.com/sergi/go-diff/diffmatchpatch"
+	"github.com/google/go-jsonnet/internal/testutils"
 )
 
 var update = flag.Bool("update", false, "update .golden files")
@@ -215,29 +215,6 @@ func runJsonnet(i jsonnetInput) jsonnetResult {
 	return runInternalJsonnet(i)
 }
 
-func compareGolden(result string, golden []byte) (string, bool) {
-	if !bytes.Equal(golden, []byte(result)) {
-		// TODO(sbarzowski) better reporting of differences in whitespace
-		// missing newline issues can be very subtle now
-		return diff(result, string(golden)), true
-	}
-	return "", false
-}
-
-func writeFile(path string, content []byte, mode os.FileMode) (changed bool, err error) {
-	old, err := ioutil.ReadFile(path)
-	if err != nil && !os.IsNotExist(err) {
-		return false, err
-	}
-	if bytes.Equal(old, content) && !os.IsNotExist(err) {
-		return false, nil
-	}
-	if err := ioutil.WriteFile(path, content, mode); err != nil {
-		return false, err
-	}
-	return true, nil
-}
-
 func compareSingleGolden(path string, result jsonnetResult) []error {
 	if result.outputMulti != nil {
 		return []error{fmt.Errorf("outputMulti is populated in a single-file test for %v", path)}
@@ -246,7 +223,7 @@ func compareSingleGolden(path string, result jsonnetResult) []error {
 	if err != nil {
 		return []error{fmt.Errorf("reading file %s: %v", path, err)}
 	}
-	if diff, hasDiff := compareGolden(result.output, golden); hasDiff {
+	if diff, hasDiff := testutils.CompareWithGolden(result.output, golden); hasDiff {
 		return []error{fmt.Errorf("golden file %v has diff:\n%v", path, diff)}
 	}
 	return nil
@@ -256,7 +233,7 @@ func updateSingleGolden(path string, result jsonnetResult) (updated []string, er
 	if result.outputMulti != nil {
 		return nil, fmt.Errorf("outputMulti is populated in a single-file test for %v", path)
 	}
-	changed, err := writeFile(path, []byte(result.output), 0666)
+	changed, err := testutils.UpdateGoldenFile(path, []byte(result.output), 0666)
 	if err != nil {
 		return nil, fmt.Errorf("updating golden file %v: %v", path, err)
 	}
@@ -289,7 +266,7 @@ func compareMultifileGolden(path string, result jsonnetResult) []error {
 			errs = append(errs, fmt.Errorf("jsonnet outputted file %v which does not exist in goldens", fn))
 			continue
 		}
-		if diff, hasDiff := compareGolden(content, goldenContent[fn]); hasDiff {
+		if diff, hasDiff := testutils.CompareWithGolden(content, goldenContent[fn]); hasDiff {
 			errs = append(errs, fmt.Errorf("golden file %v has diff:\n%v", fn, diff))
 		}
 	}
@@ -303,7 +280,7 @@ func updateMultifileGolden(path string, result jsonnetResult) ([]string, error)
 	}
 	var updatedFiles []string
 	for fn, content := range result.outputMulti {
-		updated, err := writeFile(filepath.Join(path, fn), []byte(content), 0666)
+		updated, err := testutils.UpdateGoldenFile(filepath.Join(path, fn), []byte(content), 0666)
 		if err != nil {
 			return nil, fmt.Errorf("updating golden file %v: %v", fn, err)
 		}
@@ -531,9 +508,3 @@ func TestEvalUnusualFilenames(t *testing.T) {
 		})
 	}
 }
-
-func diff(a, b string) string {
-	dmp := diffmatchpatch.New()
-	diffs := dmp.DiffMain(a, b, false)
-	return dmp.DiffPrettyText(diffs)
-}

testdata/import_syntax_error.golden

@@ -0,0 +1,10 @@
+RUNTIME ERROR: testdata/syntax_error.jsonnet:2:1 Unexpected end of file
+-------------------------------------------------
+	testdata/import_syntax_error:1:1-30	$
+
+import "syntax_error.jsonnet"
+
+-------------------------------------------------
+	During evaluation	
+
+

testdata/import_syntax_error.jsonnet

@@ -0,0 +1 @@
+import "syntax_error.jsonnet"

testdata/syntax_error.golden

@@ -0,0 +1,5 @@
+testdata/syntax_error:2:1 Unexpected end of file
+
+
+
+

testdata/syntax_error.jsonnet

@@ -0,0 +1 @@
+2 +