From 3a245f70d44f3aa1195246054fe152d6d5285697 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stanis=C5=82aw=20Barzowski?= Date: Sat, 7 Mar 2020 18:00:12 +0100 Subject: [PATCH] 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 | 2 +- ast/ast.go | 17 +++++++---- internal/parser/context.go | 12 ++++---- internal/parser/parser.go | 9 ++++++ internal/program/desugarer.go | 6 ++++ internal/testutils/BUILD.bazel | 9 ++++++ internal/testutils/test_utils.go | 45 ++++++++++++++++++++++++++++ main_test.go | 39 ++++-------------------- testdata/import_syntax_error.golden | 10 +++++++ testdata/import_syntax_error.jsonnet | 1 + testdata/syntax_error.golden | 5 ++++ testdata/syntax_error.jsonnet | 1 + 12 files changed, 110 insertions(+), 46 deletions(-) create mode 100644 internal/testutils/BUILD.bazel create mode 100644 internal/testutils/test_utils.go create mode 100644 testdata/import_syntax_error.golden create mode 100644 testdata/import_syntax_error.jsonnet create mode 100644 testdata/syntax_error.golden create mode 100644 testdata/syntax_error.jsonnet diff --git a/BUILD.bazel b/BUILD.bazel index d2cf52e..8666362 100644 --- a/BUILD.bazel +++ b/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", ], ) diff --git a/ast/ast.go b/ast/ast.go index 1f3b636..087e52f 100644 --- a/ast/ast.go +++ b/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, + 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. diff --git a/internal/parser/context.go b/internal/parser/context.go index a1c0220..9952825 100644 --- a/internal/parser/context.go +++ b/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) } diff --git a/internal/parser/parser.go b/internal/parser/parser.go index 5249998..3a1f19f 100644 --- a/internal/parser/parser.go +++ b/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 } diff --git a/internal/program/desugarer.go b/internal/program/desugarer.go index 08b754d..e9c8dce 100644 --- a/internal/program/desugarer.go +++ b/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)) diff --git a/internal/testutils/BUILD.bazel b/internal/testutils/BUILD.bazel new file mode 100644 index 0000000..d69b262 --- /dev/null +++ b/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"], +) diff --git a/internal/testutils/test_utils.go b/internal/testutils/test_utils.go new file mode 100644 index 0000000..ad215b7 --- /dev/null +++ b/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 +} diff --git a/main_test.go b/main_test.go index 4dd3b45..a595d6e 100644 --- a/main_test.go +++ b/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) -} diff --git a/testdata/import_syntax_error.golden b/testdata/import_syntax_error.golden new file mode 100644 index 0000000..2c5ac24 --- /dev/null +++ b/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 + + diff --git a/testdata/import_syntax_error.jsonnet b/testdata/import_syntax_error.jsonnet new file mode 100644 index 0000000..64f633d --- /dev/null +++ b/testdata/import_syntax_error.jsonnet @@ -0,0 +1 @@ +import "syntax_error.jsonnet" diff --git a/testdata/syntax_error.golden b/testdata/syntax_error.golden new file mode 100644 index 0000000..914e700 --- /dev/null +++ b/testdata/syntax_error.golden @@ -0,0 +1,5 @@ +testdata/syntax_error:2:1 Unexpected end of file + + + + diff --git a/testdata/syntax_error.jsonnet b/testdata/syntax_error.jsonnet new file mode 100644 index 0000000..de86045 --- /dev/null +++ b/testdata/syntax_error.jsonnet @@ -0,0 +1 @@ +2 +