Skip to content

Commit

Permalink
fix: fix test cases and improve positions in errors (#1096)
Browse files Browse the repository at this point in the history 
Changes
- Incomplete module boilerplate for test modules
- `ftl/one` was using `two.User`, but `ftl/two` was not including it in
its schema
    - I've added a new Verb which makes use of User
- compiling schema was returning errors at the node that was reached
before finding an error, where as sometimes we have a more accurate
position available
- Now we pass around the latest position we dive deeper into all the
checks, rather than staying at the node's position
- Previously we would get an error on the verb location, rather than the
field on a struct that was used by the verb

Not included in this PR is a fix for #1095, which is why test/data is
using Option in the new verb
  • Loading branch information
@matt2e
matt2e authored Mar 17, 2024
1 parent 1a7e213 commit 664bf1d
Show file tree
Hide file tree
Showing 14 changed files with 701 additions and 84 deletions.
5 changes: 3 additions & 2 deletions go-runtime/compile/errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"errors"
"fmt"
"go/ast"
"go/token"

"github.com/TBD54566975/ftl/backend/schema"
)
Expand All @@ -17,8 +18,8 @@ type Error struct {
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 errorf(pos token.Pos, format string, args ...any) Error {
return Error{Msg: fmt.Sprintf(format, args...), Pos: goPosToSchemaPos(pos)}
}

func wrapf(node ast.Node, err error, format string, args ...any) Error {
Expand Down
123 changes: 60 additions & 63 deletions go-runtime/compile/schema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,14 +154,14 @@ func visitCallExpr(pctx *parseContext, node *ast.CallExpr) error {

func parseCall(pctx *parseContext, node *ast.CallExpr) error {
if len(node.Args) != 3 {
return errorf(node, "call must have exactly three arguments")
return errorf(node.Pos(), "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 errorf(node.Args[1], "call first argument must be a function but is an unresolved reference to %s.%s", sel.X, sel.Sel)
return errorf(node.Args[1].Pos(), "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 %T", node.Args[1])
return errorf(node.Args[1].Pos(), "call first argument must be a function but is %T", node.Args[1])
}
if pctx.activeVerb == nil {
return nil
Expand Down Expand Up @@ -191,13 +191,13 @@ func parseConfigDecl(pctx *parseContext, node *ast.CallExpr, fn *types.Func) err
}
}
if name == "" {
return errorf(node, "config and secret declarations must have a single string literal argument")
return errorf(node.Pos(), "config and secret declarations must have a single string literal argument")
}
index := node.Fun.(*ast.IndexExpr) //nolint:forcetypeassert

// Type parameter
tp := pctx.pkg.TypesInfo.Types[index.Index].Type
st, err := visitType(pctx, index.Index, tp)
st, err := visitType(pctx, index.Index.Pos(), tp)
if err != nil {
return err
}
Expand Down Expand Up @@ -234,12 +234,12 @@ func visitFile(pctx *parseContext, node *ast.File) error {
switch dir := dir.(type) {
case *directiveModule:
if 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)
return errorf(node.Pos(), "%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 errorf(node, "%s: invalid directive", dir)
return errorf(node.Pos(), "%s: invalid directive", dir)
}
}
return nil
Expand Down Expand Up @@ -321,8 +321,7 @@ func visitGenDecl(pctx *parseContext, node *ast.GenDecl) error {
Comments: visitComments(node.Doc),
}
if len(node.Specs) != 1 {
return fmt.Errorf("%s: error parsing ftl enum %s: expected exactly one type spec",
goPosToSchemaPos(node.Pos()), enum.Name)
return errorf(node.Pos(), "error parsing ftl enum %s: expected exactly one type spec", enum.Name)
}
if t, ok := node.Specs[0].(*ast.TypeSpec); ok {
pctx.enums[t.Name.Name] = enum
Expand Down Expand Up @@ -368,7 +367,7 @@ func visitTypeSpec(pctx *parseContext, node *ast.TypeSpec) error {
if enum == nil {
return nil
}
typ, err := visitType(pctx, node, pctx.pkg.TypesInfo.TypeOf(node.Type))
typ, err := visitType(pctx, node.Pos(), pctx.pkg.TypesInfo.TypeOf(node.Type))
if err != nil {
return err
}
Expand All @@ -388,8 +387,7 @@ func visitValueSpec(pctx *parseContext, node *ast.ValueSpec) error {
}
c, ok := pctx.pkg.TypesInfo.Defs[node.Names[0]].(*types.Const)
if !ok {
return fmt.Errorf("%s: could not extract enum %s: expected exactly one variant name",
goPosToSchemaPos(node.Pos()), enum.Name)
return errorf(node.Pos(), "could not extract enum %s: expected exactly one variant name", enum.Name)
}
value, err := visitConst(c)
if err != nil {
Expand Down Expand Up @@ -446,7 +444,7 @@ func visitFuncDecl(pctx *parseContext, node *ast.FuncDecl) (verb *schema.Verb, e
fnt := pctx.pkg.TypesInfo.Defs[node.Name].(*types.Func) //nolint:forcetypeassert
sig := fnt.Type().(*types.Signature) //nolint:forcetypeassert
if sig.Recv() != nil {
return nil, fmt.Errorf("ftl:verb cannot be a method")
return nil, errorf(node.Pos(), "ftl:verb cannot be a method")
}
params := sig.Params()
results := sig.Results()
Expand All @@ -456,7 +454,7 @@ func visitFuncDecl(pctx *parseContext, node *ast.FuncDecl) (verb *schema.Verb, e
}
var req schema.Type
if reqt != nil {
req, err = visitType(pctx, node, params.At(1).Type())
req, err = visitType(pctx, node.Pos(), params.At(1).Type())
if err != nil {
return nil, err
}
Expand All @@ -465,7 +463,7 @@ func visitFuncDecl(pctx *parseContext, node *ast.FuncDecl) (verb *schema.Verb, e
}
var resp schema.Type
if respt != nil {
resp, err = visitType(pctx, node, results.At(0).Type())
resp, err = visitType(pctx, node.Pos(), results.At(0).Type())
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -508,25 +506,25 @@ func visitComments(doc *ast.CommentGroup) []string {
return comments
}

func visitStruct(pctx *parseContext, node ast.Node, tnode types.Type) (*schema.Ref, error) {
func visitStruct(pctx *parseContext, pos token.Pos, tnode types.Type) (*schema.Ref, error) {
named, ok := tnode.(*types.Named)
if !ok {
return nil, fmt.Errorf("expected named type but got %s", tnode)
return nil, errorf(pos, "expected named type but got %s", tnode)
}
nodePath := named.Obj().Pkg().Path()
if !strings.HasPrefix(nodePath, pctx.pkg.PkgPath) {
base := path.Dir(pctx.pkg.PkgPath)
destModule := path.Base(strings.TrimPrefix(nodePath, base+"/"))
dataRef := &schema.Ref{
Pos: goPosToSchemaPos(node.Pos()),
Pos: goPosToSchemaPos(pos),
Module: destModule,
Name: named.Obj().Name(),
}
for i := 0; i < named.TypeArgs().Len(); i++ {
arg := named.TypeArgs().At(i)
typeArg, err := visitType(pctx, node, arg)
typeArg, err := visitType(pctx, pos, arg)
if err != nil {
return nil, fmt.Errorf("type parameter %s: %w", arg.String(), err)
return nil, errorf(pos, "type parameter %s: %v", arg.String(), err)
}

// Fully qualify the Ref if needed
Expand All @@ -542,23 +540,23 @@ func visitStruct(pctx *parseContext, node ast.Node, tnode types.Type) (*schema.R
}

out := &schema.Data{
Pos: goPosToSchemaPos(node.Pos()),
Pos: goPosToSchemaPos(pos),
Name: strcase.ToUpperCamel(named.Obj().Name()),
}
pctx.nativeNames[out] = named.Obj().Name()
dataRef := &schema.Ref{
Pos: goPosToSchemaPos(node.Pos()),
Pos: goPosToSchemaPos(pos),
Name: out.Name,
}
for i := 0; i < named.TypeParams().Len(); i++ {
param := named.TypeParams().At(i)
out.TypeParameters = append(out.TypeParameters, &schema.TypeParameter{
Pos: goPosToSchemaPos(node.Pos()),
Pos: goPosToSchemaPos(pos),
Name: param.Obj().Name(),
})
typeArg, err := visitType(pctx, node, named.TypeArgs().At(i))
typeArg, err := visitType(pctx, pos, named.TypeArgs().At(i))
if err != nil {
return nil, fmt.Errorf("type parameter %s: %w", param.Obj().Name(), err)
return nil, errorf(pos, "type parameter %s: %v", param.Obj().Name(), err)
}
dataRef.TypeParameters = append(dataRef.TypeParameters, typeArg)
}
Expand All @@ -570,8 +568,8 @@ func visitStruct(pctx *parseContext, node ast.Node, tnode types.Type) (*schema.R
}

// Find type declaration so we can extract comments.
pos := named.Obj().Pos()
pkg, path, _ := pctx.pathEnclosingInterval(pos, pos)
namedPos := named.Obj().Pos()
pkg, path, _ := pctx.pathEnclosingInterval(namedPos, namedPos)
if pkg != nil {
for i := len(path) - 1; i >= 0; i-- {
// We have to check both the type spec and the gen decl because the
Expand All @@ -592,18 +590,18 @@ func visitStruct(pctx *parseContext, node ast.Node, tnode types.Type) (*schema.R

s, ok := named.Underlying().(*types.Struct)
if !ok {
return nil, fmt.Errorf("expected struct but got %s", named)
return nil, errorf(pos, "expected struct but got %s", named)
}
for i := 0; i < s.NumFields(); i++ {
f := s.Field(i)
ft, err := visitType(pctx, node, f.Type())
ft, err := visitType(pctx, f.Pos(), f.Type())
if err != nil {
return nil, fmt.Errorf("field %s: %w", f.Name(), err)
return nil, errorf(pos, "field %s: %v", f.Name(), err)
}

// Check if field is exported
if len(f.Name()) > 0 && unicode.IsLower(rune(f.Name()[0])) {
return nil, fmt.Errorf("params field %s must be exported by starting with an uppercase letter", f.Name())
return nil, errorf(pos, "params field %s must be exported by starting with an uppercase letter", f.Name())
}

// Extract the JSON tag and split it to get just the field name
Expand All @@ -617,13 +615,13 @@ func visitStruct(pctx *parseContext, node ast.Node, tnode types.Type) (*schema.R
var metadata []schema.Metadata
if jsonFieldName != "" {
metadata = append(metadata, &schema.MetadataAlias{
Pos: goPosToSchemaPos(node.Pos()),
Pos: goPosToSchemaPos(pos),
Kind: schema.AliasKindJSON,
Alias: jsonFieldName,
})
}
out.Fields = append(out.Fields, &schema.Field{
Pos: goPosToSchemaPos(node.Pos()),
Pos: goPosToSchemaPos(pos),
Name: strcase.ToLowerCamel(f.Name()),
Type: ft,
Metadata: metadata,
Expand All @@ -650,24 +648,23 @@ func visitConst(cnode *types.Const) (schema.Value, error) {
}
return &schema.IntValue{Pos: goPosToSchemaPos(cnode.Pos()), Value: int(value)}, nil
default:
return nil, fmt.Errorf("%s: unsupported basic type %s", goPosToSchemaPos(cnode.Pos()),
b)
return nil, errorf(cnode.Pos(), "unsupported basic type %s", b)
}
}
return nil, fmt.Errorf("%s: unsupported const type %s", goPosToSchemaPos(cnode.Pos()), cnode.Type())
return nil, errorf(cnode.Pos(), "unsupported const type %s", cnode.Type())
}

func visitType(pctx *parseContext, node ast.Node, tnode types.Type) (schema.Type, error) {
func visitType(pctx *parseContext, pos token.Pos, tnode types.Type) (schema.Type, error) {
if tparam, ok := tnode.(*types.TypeParam); ok {
return &schema.Ref{Pos: goPosToSchemaPos(node.Pos()), Name: tparam.Obj().Id()}, nil
return &schema.Ref{Pos: goPosToSchemaPos(pos), Name: tparam.Obj().Id()}, nil
}
switch underlying := tnode.Underlying().(type) {
case *types.Basic:
if named, ok := tnode.(*types.Named); ok {
nodePath := named.Obj().Pkg().Path()
if pctx.enums[named.Obj().Name()] != nil {
return &schema.Ref{
Pos: goPosToSchemaPos(node.Pos()),
Pos: goPosToSchemaPos(pos),
Name: named.Obj().Name(),
}, nil
} else if !strings.HasPrefix(nodePath, pctx.pkg.PkgPath) {
Expand All @@ -676,7 +673,7 @@ func visitType(pctx *parseContext, node ast.Node, tnode types.Type) (schema.Type
base := path.Dir(pctx.pkg.PkgPath)
destModule := path.Base(strings.TrimPrefix(nodePath, base+"/"))
enumRef := &schema.Ref{
Pos: goPosToSchemaPos(node.Pos()),
Pos: goPosToSchemaPos(pos),
Module: destModule,
Name: named.Obj().Name(),
}
Expand All @@ -686,90 +683,90 @@ func visitType(pctx *parseContext, node ast.Node, tnode types.Type) (schema.Type

switch underlying.Kind() {
case types.String:
return &schema.String{Pos: goPosToSchemaPos(node.Pos())}, nil
return &schema.String{Pos: goPosToSchemaPos(pos)}, nil

case types.Int, types.Int64:
return &schema.Int{Pos: goPosToSchemaPos(node.Pos())}, nil
return &schema.Int{Pos: goPosToSchemaPos(pos)}, nil

case types.Bool:
return &schema.Bool{Pos: goPosToSchemaPos(node.Pos())}, nil
return &schema.Bool{Pos: goPosToSchemaPos(pos)}, nil

case types.Float64:
return &schema.Float{Pos: goPosToSchemaPos(node.Pos())}, nil
return &schema.Float{Pos: goPosToSchemaPos(pos)}, nil

default:
return nil, errorf(node, "unsupported basic type %s", underlying)
return nil, errorf(pos, "unsupported basic type %s", underlying)
}

case *types.Struct:
named, ok := tnode.(*types.Named)
if !ok {
return visitStruct(pctx, node, tnode)
return visitStruct(pctx, pos, tnode)
}

// Special-cased types.
switch named.Obj().Pkg().Path() + "." + named.Obj().Name() {
case "time.Time":
return &schema.Time{Pos: goPosToSchemaPos(node.Pos())}, nil
return &schema.Time{Pos: goPosToSchemaPos(pos)}, nil

case "github.com/TBD54566975/ftl/go-runtime/ftl.Unit":
return &schema.Unit{Pos: goPosToSchemaPos(node.Pos())}, nil
return &schema.Unit{Pos: goPosToSchemaPos(pos)}, nil

case "github.com/TBD54566975/ftl/go-runtime/ftl.Option":
underlying, err := visitType(pctx, node, named.TypeArgs().At(0))
underlying, err := visitType(pctx, pos, named.TypeArgs().At(0))
if err != nil {
return nil, err
}
return &schema.Optional{Type: underlying}, nil

default:
return visitStruct(pctx, node, tnode)
return visitStruct(pctx, pos, tnode)
}

case *types.Map:
return visitMap(pctx, node, underlying)
return visitMap(pctx, pos, underlying)

case *types.Slice:
return visitSlice(pctx, node, underlying)
return visitSlice(pctx, pos, underlying)

case *types.Interface:
if underlying.String() == "any" {
return &schema.Any{Pos: goPosToSchemaPos(node.Pos())}, nil
return &schema.Any{Pos: goPosToSchemaPos(pos)}, nil
}
return nil, errorf(node, "unsupported type %T", node)
return nil, errorf(pos, "unsupported type %T", tnode)

default:
return nil, errorf(node, "unsupported type %T", node)
return nil, errorf(pos, "unsupported type %T", tnode)
}
}

func visitMap(pctx *parseContext, node ast.Node, tnode *types.Map) (*schema.Map, error) {
key, err := visitType(pctx, node, tnode.Key())
func visitMap(pctx *parseContext, pos token.Pos, tnode *types.Map) (*schema.Map, error) {
key, err := visitType(pctx, pos, tnode.Key())
if err != nil {
return nil, err
}
value, err := visitType(pctx, node, tnode.Elem())
value, err := visitType(pctx, pos, tnode.Elem())
if err != nil {
return nil, err
}
return &schema.Map{
Pos: goPosToSchemaPos(node.Pos()),
Pos: goPosToSchemaPos(pos),
Key: key,
Value: value,
}, nil
}

func visitSlice(pctx *parseContext, node ast.Node, tnode *types.Slice) (schema.Type, error) {
func visitSlice(pctx *parseContext, pos token.Pos, tnode *types.Slice) (schema.Type, error) {
// If it's a []byte, treat it as a Bytes type.
if basic, ok := tnode.Elem().Underlying().(*types.Basic); ok && basic.Kind() == types.Byte {
return &schema.Bytes{Pos: goPosToSchemaPos(node.Pos())}, nil
return &schema.Bytes{Pos: goPosToSchemaPos(pos)}, nil
}
value, err := visitType(pctx, node, tnode.Elem())
value, err := visitType(pctx, pos, tnode.Elem())
if err != nil {
return nil, err
}
return &schema.Array{
Pos: goPosToSchemaPos(node.Pos()),
Pos: goPosToSchemaPos(pos),
Element: value,
}, nil
}
Expand Down
Loading

0 comments on commit 664bf1d

Please sign in to comment.