From 4221f57f6e0e4f45c9ee173c511cb9219ab664e9 Mon Sep 17 00:00:00 2001 From: Alec Thomas Date: Tue, 12 Mar 2024 14:35:01 +1100 Subject: [PATCH] fix(go-runtime): remove duplicate positional information in errors (#1064) Fixes #1062 --- go-runtime/compile/errors.go | 40 +++++++++++++++++++ go-runtime/compile/schema.go | 30 ++++++++------ go-runtime/compile/schema_test.go | 8 ++++ .../compile/testdata/failing/failing.go | 17 ++++++++ 4 files changed, 82 insertions(+), 13 deletions(-) create mode 100644 go-runtime/compile/errors.go create mode 100644 go-runtime/compile/testdata/failing/failing.go diff --git a/go-runtime/compile/errors.go b/go-runtime/compile/errors.go new file mode 100644 index 0000000000..86af23a1af --- /dev/null +++ b/go-runtime/compile/errors.go @@ -0,0 +1,40 @@ +package compile + +import ( + "errors" + "fmt" + "go/ast" + + "github.com/TBD54566975/ftl/backend/schema" +) + +type Error struct { + Msg string + Pos schema.Position + Err error // Wrapped error, if any +} + +func (e Error) Error() string { return fmt.Sprintf("%s: %s", e.Pos, e.Msg) } +func (e Error) Unwrap() error { return e.Err } + +func errorf(node ast.Node, format string, args ...any) Error { + return Error{Msg: fmt.Sprintf(format, args...), Pos: goPosToSchemaPos(node.Pos())} +} + +func wrapf(node ast.Node, err error, format string, args ...any) Error { + if format == "" { + format = "%s" + } else { + format += ": %s" + } + // Propagate existing error position if available + var pos schema.Position + if perr := (Error{}); errors.As(err, &perr) { + pos = perr.Pos + args = append(args, perr.Msg) + } else { + pos = goPosToSchemaPos(node.Pos()) + args = append(args, err) + } + return Error{Msg: fmt.Sprintf(format, args...), Pos: pos, Err: err} +} diff --git a/go-runtime/compile/schema.go b/go-runtime/compile/schema.go index 61c37387f7..218c6c6696 100644 --- a/go-runtime/compile/schema.go +++ b/go-runtime/compile/schema.go @@ -61,7 +61,11 @@ func ExtractModuleSchema(dir string) (NativeNames, *schema.Module, error) { for _, pkg := range pkgs { if len(pkg.Errors) > 0 { for _, perr := range pkg.Errors { - merr = append(merr, fmt.Errorf("%s: %w", pkg.PkgPath, perr)) + if len(pkg.Syntax) > 0 { + merr = append(merr, wrapf(pkg.Syntax[0], perr, "%s", pkg.PkgPath)) + } else { + merr = append(merr, fmt.Errorf("%s: %w", pkg.PkgPath, perr)) + } } } pctx := &parseContext{pkg: pkg, pkgs: pkgs, module: module, nativeNames: NativeNames{}, enums: enums{}} @@ -69,7 +73,7 @@ func ExtractModuleSchema(dir string) (NativeNames, *schema.Module, error) { err := goast.Visit(file, func(node ast.Node, next func() error) (err error) { defer func() { if err != nil { - err = fmt.Errorf("%s: %w", fset.Position(node.Pos()).String(), err) + err = wrapf(node, err, "") } }() switch node := node.(type) { @@ -149,14 +153,14 @@ func visitCallExpr(pctx *parseContext, node *ast.CallExpr) error { func parseCall(pctx *parseContext, node *ast.CallExpr) error { if len(node.Args) != 3 { - return fmt.Errorf("%s: call must have exactly three arguments", goPosToSchemaPos(node.Pos())) + return errorf(node, "call must have exactly three arguments") } _, verbFn := deref[*types.Func](pctx.pkg, node.Args[1]) if verbFn == nil { if sel, ok := node.Args[1].(*ast.SelectorExpr); ok { - return fmt.Errorf("call first argument must be a function but is an unresolved reference to %s.%s", sel.X, sel.Sel) + return errorf(node.Args[1], "call first argument must be a function but is an unresolved reference to %s.%s", sel.X, sel.Sel) } - return fmt.Errorf("call first argument must be a function but is %T", node.Args[1]) + return errorf(node.Args[1], "call first argument must be a function but is %T", node.Args[1]) } if pctx.activeVerb == nil { return nil @@ -181,12 +185,12 @@ func parseConfigDecl(pctx *parseContext, node *ast.CallExpr, fn *types.Func) err var err error name, err = strconv.Unquote(literal.Value) if err != nil { - return fmt.Errorf("%s: %w", goPosToSchemaPos(node.Pos()), err) + return wrapf(node, err, "") } } } if name == "" { - return fmt.Errorf("%s: config and secret declarations must have a single string literal argument", goPosToSchemaPos(node.Pos())) + return errorf(node, "config and secret declarations must have a single string literal argument") } index := node.Fun.(*ast.IndexExpr) //nolint:forcetypeassert @@ -229,12 +233,12 @@ func visitFile(pctx *parseContext, node *ast.File) error { switch dir := dir.(type) { case *directiveModule: if dir.Name != pctx.pkg.Name { - return fmt.Errorf("%s: FTL module name %q does not match Go package name %q", dir, dir.Name, pctx.pkg.Name) + return errorf(node, "%s: FTL module name %q does not match Go package name %q", dir, dir.Name, pctx.pkg.Name) } pctx.module.Name = dir.Name default: - return fmt.Errorf("%s: invalid directive", dir) + return errorf(node, "%s: invalid directive", dir) } } return nil @@ -442,7 +446,7 @@ func visitFuncDecl(pctx *parseContext, node *ast.FuncDecl) (verb *schema.Verb, e results := sig.Results() reqt, respt, err := checkSignature(sig) if err != nil { - return nil, err + return nil, wrapf(node, err, "") } var req schema.Type if reqt != nil { @@ -688,7 +692,7 @@ func visitType(pctx *parseContext, node ast.Node, tnode types.Type) (schema.Type return &schema.Float{Pos: goPosToSchemaPos(node.Pos())}, nil default: - return nil, fmt.Errorf("unsupported basic type %s", underlying) + return nil, errorf(node, "unsupported basic type %s", underlying) } case *types.Struct: @@ -726,10 +730,10 @@ func visitType(pctx *parseContext, node ast.Node, tnode types.Type) (schema.Type if underlying.String() == "any" { return &schema.Any{Pos: goPosToSchemaPos(node.Pos())}, nil } - return nil, fmt.Errorf("%s: unsupported type %T", goPosToSchemaPos(node.Pos()), node) + return nil, errorf(node, "unsupported type %T", node) default: - return nil, fmt.Errorf("%s: unsupported type %T", goPosToSchemaPos(node.Pos()), node) + return nil, errorf(node, "unsupported type %T", node) } } diff --git a/go-runtime/compile/schema_test.go b/go-runtime/compile/schema_test.go index 93a8c39e95..df498c575e 100644 --- a/go-runtime/compile/schema_test.go +++ b/go-runtime/compile/schema_test.go @@ -3,6 +3,8 @@ package compile import ( "go/ast" "go/types" + "os" + "path/filepath" "strings" "testing" @@ -175,3 +177,9 @@ func TestParseBasicTypes(t *testing.T) { func normaliseString(s string) string { return strings.TrimSpace(strings.Join(slices.Map(strings.Split(s, "\n"), strings.TrimSpace), "\n")) } + +func TestErrorReporting(t *testing.T) { + pwd, _ := os.Getwd() + _, _, err := ExtractModuleSchema("testdata/failing") + assert.EqualError(t, err, filepath.Join(pwd, `testdata/failing/failing.go`)+`:15:2: call must have exactly three arguments`) +} diff --git a/go-runtime/compile/testdata/failing/failing.go b/go-runtime/compile/testdata/failing/failing.go new file mode 100644 index 0000000000..f1bfd07f1b --- /dev/null +++ b/go-runtime/compile/testdata/failing/failing.go @@ -0,0 +1,17 @@ +//ftl:module failing +package failing + +import ( + "context" + + "github.com/TBD54566975/ftl/go-runtime/ftl" +) + +type Request struct{} +type Response struct{} + +//ftl:verb +func FailingVerb(ctx context.Context, req Request) (Response, error) { + ftl.Call(ctx, "failing", "failingVerb", req) + return Response{}, nil +}