Skip to content

Commit

Permalink
feat: chain schema errors in Go for the LSP (#1265)
Browse files Browse the repository at this point in the history 
percolates errors up the chain of affected exports, e.g. if a type
contains schema errors and is used in a verb schema, the errors will be
highlighted in the type itself and the failing type will be highlighted
in the verb that uses it
  • Loading branch information
@worstell
worstell authored Apr 15, 2024
1 parent 8da1552 commit c7d1b4d
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 20 deletions.
6 changes: 5 additions & 1 deletion buildengine/build_go_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,10 @@ func TestExternalType(t *testing.T) {
sch: &schema.Schema{},
}
testBuild(t, bctx, true, []assertion{

Check failure on line 186 in buildengine/build_go_test.go

View workflow job for this annotation

GitHub Actions / Test Go 

=== RUN   TestExternalType
info:external: Building module
debug:external: Generating external modules
debug:external: Extracting schema
    buildengine/build_go_test.go:186: Expected values to be equal:
         []*schema.Error{
           {
        -    Msg: "invalid response type \"ftl/external.ExternalResponse\"",
        -  },
        -  {
             Msg: "unsupported external type \"time.Month\"",
           },
           {
             Msg: "invalid type \"ftl/external.ExternalResponse\"",
           },
        +  {
        +    Msg: "invalid response type \"ftl/external.ExternalResponse\"",
        +  },
         }
--- FAIL: TestExternalType (0.10s)
assertBuildProtoErrors("unsupported external type \"time.Month\""),
assertBuildProtoErrors(
"unsupported external type \"time.Month\"",
"invalid type \"ftl/external.ExternalResponse\"",
"invalid response type \"ftl/external.ExternalResponse\"",
),
})
}
35 changes: 25 additions & 10 deletions go-runtime/compile/schema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,15 +276,15 @@ func checkSignature(pctx *parseContext, node *ast.FuncDecl, sig *types.Signature
if params.Len() == 0 {
pctx.errors = append(pctx.errors, errorf(node, "first parameter must be context.Context"))
} else if !types.AssertableTo(contextIfaceType(), params.At(0).Type()) {
pctx.errors = append(pctx.errors, errorf(node, "first parameter must be of type context.Context but is %s", params.At(0).Type()))
pctx.errors = append(pctx.errors, tokenErrorf(params.At(0).Pos(), params.At(0).Name(), "first parameter must be of type context.Context but is %s", params.At(0).Type()))
}

if params.Len() == 2 {
if !isType[*types.Struct](params.At(1).Type()) {
pctx.errors = append(pctx.errors, errorf(node, "second parameter must be a struct but is %s", params.At(1).Type()))
pctx.errors = append(pctx.errors, tokenErrorf(params.At(1).Pos(), params.At(1).Name(), "second parameter must be a struct but is %s", params.At(1).Type()))
}
if params.At(1).Type().String() == ftlUnitTypePath {
pctx.errors = append(pctx.errors, errorf(node, "second parameter must not be ftl.Unit"))
pctx.errors = append(pctx.errors, tokenErrorf(params.At(1).Pos(), params.At(1).Name(), "second parameter must not be ftl.Unit"))
}

req = optional.Some(params.At(1))
Expand All @@ -296,14 +296,14 @@ func checkSignature(pctx *parseContext, node *ast.FuncDecl, sig *types.Signature
if results.Len() == 0 {
pctx.errors = append(pctx.errors, errorf(node, "must at least return an error"))
} else if !types.AssertableTo(errorIFaceType(), results.At(results.Len()-1).Type()) {
pctx.errors = append(pctx.errors, errorf(node, "must return an error but is %s", results.At(0).Type()))
pctx.errors = append(pctx.errors, tokenErrorf(results.At(results.Len()-1).Pos(), results.At(results.Len()-1).Name(), "must return an error but is %s", results.At(0).Type()))
}
if results.Len() == 2 {
if !isType[*types.Struct](results.At(0).Type()) {
pctx.errors = append(pctx.errors, errorf(node, "first result must be a struct but is %s", results.At(0).Type()))
pctx.errors = append(pctx.errors, tokenErrorf(results.At(0).Pos(), results.At(0).Name(), "first result must be a struct but is %s", results.At(0).Type()))
}
if results.At(1).Type().String() == ftlUnitTypePath {
pctx.errors = append(pctx.errors, errorf(node, "second result must not be ftl.Unit"))
pctx.errors = append(pctx.errors, tokenErrorf(results.At(1).Pos(), results.At(1).Name(), "second result must not be ftl.Unit"))
}
resp = optional.Some(results.At(0))
}
Expand Down Expand Up @@ -465,7 +465,7 @@ func visitFuncDecl(pctx *parseContext, node *ast.FuncDecl) (verb *schema.Verb) {

for _, name := range pctx.nativeNames {
if name == node.Name.Name {
pctx.errors = append(pctx.errors, errorf(node, "verb %q already exported", node.Name.Name))
pctx.errors = append(pctx.errors, noEndColumnErrorf(node.Pos(), "verb %q already exported", node.Name.Name))
return nil
}
}
Expand Down Expand Up @@ -494,10 +494,14 @@ func visitFuncDecl(pctx *parseContext, node *ast.FuncDecl) (verb *schema.Verb) {
}
reqV, reqOk := req.Get()
resV, respOk := resp.Get()
if !reqOk || !respOk {
return nil
if !reqOk {
pctx.errors = append(pctx.errors, tokenErrorf(params.At(1).Pos(), params.At(1).Name(),
"invalid request type %q", params.At(1).Type()))
}
if !respOk {
pctx.errors = append(pctx.errors, tokenErrorf(results.At(0).Pos(), results.At(0).Name(),
"invalid response type %q", results.At(0).Type()))
}

verb = &schema.Verb{
Pos: goPosToSchemaPos(node.Pos()),
Comments: visitComments(node.Doc),
Expand Down Expand Up @@ -613,13 +617,16 @@ func visitStruct(pctx *parseContext, pos token.Pos, tnode types.Type) optional.O
pctx.errors = append(pctx.errors, tokenErrorf(pos, named.String(), "expected struct but got %s", named))
return optional.None[*schema.Ref]()
}

fieldErrors := false
for i := range s.NumFields() {
f := s.Field(i)
if ft, ok := visitType(pctx, f.Pos(), f.Type()).Get(); ok {
// Check if field is exported
if len(f.Name()) > 0 && unicode.IsLower(rune(f.Name()[0])) {
pctx.errors = append(pctx.errors,
tokenErrorf(f.Pos(), f.Name(), "struct field %s must be exported by starting with an uppercase letter", f.Name()))
fieldErrors = true
}

// Extract the JSON tag and split it to get just the field name
Expand All @@ -644,8 +651,14 @@ func visitStruct(pctx *parseContext, pos token.Pos, tnode types.Type) optional.O
Type: ft,
Metadata: metadata,
})
} else {
fieldErrors = true
}
}
if fieldErrors {
return optional.None[*schema.Ref]()
}

pctx.module.AddData(out)
return optional.Some[*schema.Ref](dataRef)
}
Expand Down Expand Up @@ -733,6 +746,7 @@ func visitType(pctx *parseContext, pos token.Pos, tnode types.Type) optional.Opt
if ref, ok := visitStruct(pctx, pos, tnode).Get(); ok {
return optional.Some[schema.Type](ref)
}
pctx.errors = append(pctx.errors, noEndColumnErrorf(pos, "invalid type %q", tnode))
return optional.None[schema.Type]()
}

Expand All @@ -759,6 +773,7 @@ func visitType(pctx *parseContext, pos token.Pos, tnode types.Type) optional.Opt
if ref, ok := visitStruct(pctx, pos, tnode).Get(); ok {
return optional.Some[schema.Type](ref)
}
pctx.errors = append(pctx.errors, noEndColumnErrorf(pos, "invalid type %q", tnode))
return optional.None[schema.Type]()
}

Expand Down
39 changes: 30 additions & 9 deletions go-runtime/compile/schema_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,21 +249,42 @@ func TestErrorReporting(t *testing.T) {
filename+":13:2-2: unsupported type \"error\"\n"+
filename+":16:2-2: unsupported basic type \"uint64\"\n"+
filename+":19:3-3: unexpected token \"verb\" (expected Directive)\n"+
filename+":25:1-1: invalid type \"ftl/failing.Request\"\n"+
filename+":25:1-1: invalid type \"ftl/failing.Response\"\n"+
filename+":25:36-39: invalid request type \"ftl/failing.Request\"\n"+
filename+":25:50-50: invalid response type \"ftl/failing.Response\"\n"+
filename+":26:16-29: call first argument must be a function in an ftl module\n"+
filename+":27:2-46: call must have exactly three arguments\n"+
filename+":28:16-25: call first argument must be a function\n"+
filename+":33:1-1: invalid type \"ftl/failing.Response\"\n"+
filename+":33:1-2: must have at most two parameters (context.Context, struct)\n"+
filename+":38:1-2: first parameter must be of type context.Context but is ftl/failing.Request\n"+
filename+":38:1-2: second parameter must be a struct but is string\n"+
filename+":43:1-2: second parameter must not be ftl.Unit\n"+
filename+":33:69-69: invalid response type \"ftl/failing.Response\"\n"+
filename+":38:1-1: invalid type \"ftl/failing.Response\"\n"+
filename+":38:22-27: first parameter must be of type context.Context but is ftl/failing.Request\n"+
filename+":38:37-43: second parameter must be a struct but is string\n"+
filename+":38:53-53: invalid response type \"ftl/failing.Response\"\n"+
filename+":43:1-1: invalid type \"ftl/failing.Response\"\n"+
filename+":43:43-47: second parameter must not be ftl.Unit\n"+
filename+":43:59-59: invalid response type \"ftl/failing.Response\"\n"+
filename+":48:1-1: invalid type \"ftl/failing.Response\"\n"+
filename+":48:1-2: first parameter must be context.Context\n"+
filename+":48:18-18: invalid response type \"ftl/failing.Response\"\n"+
filename+":53:1-1: invalid type \"ftl/failing.Request\"\n"+
filename+":53:1-2: must have at most two results (struct, error)\n"+
filename+":53:41-44: invalid request type \"ftl/failing.Request\"\n"+
filename+":58:1-1: invalid type \"ftl/failing.Request\"\n"+
filename+":58:1-2: must at least return an error\n"+
filename+":62:1-2: must return an error but is ftl/failing.Response\n"+
filename+":67:1-2: first result must be a struct but is string\n"+
filename+":67:1-2: must return an error but is string\n"+
filename+":67:1-2: second result must not be ftl.Unit\n"+
filename+":74:1-2: verb \"WrongResponse\" already exported\n"+
filename+":58:36-39: invalid request type \"ftl/failing.Request\"\n"+
filename+":62:1-1: invalid type \"ftl/failing.Request\"\n"+
filename+":62:35-38: invalid request type \"ftl/failing.Request\"\n"+
filename+":62:48-48: must return an error but is ftl/failing.Response\n"+
filename+":67:1-1: invalid type \"ftl/failing.Request\"\n"+
filename+":67:41-44: invalid request type \"ftl/failing.Request\"\n"+
filename+":67:55-55: first result must be a struct but is string\n"+
filename+":67:63-63: must return an error but is string\n"+
filename+":67:63-63: second result must not be ftl.Unit\n"+
filename+":74:1-1: verb \"WrongResponse\" already exported\n"+
filename+":79:6-6: invalid type \"ftl/failing.BadStruct\"\n"+
filename+":80:2-12: struct field unexported must be exported by starting with an uppercase letter",
)
}
Expand All @@ -274,5 +295,5 @@ func TestDuplicateVerbNames(t *testing.T) {
}
pwd, _ := os.Getwd()
_, _, err := ExtractModuleSchema("testdata/duplicateverbs")
assert.EqualError(t, err, filepath.Join(pwd, `testdata/duplicateverbs/duplicateverbs.go`)+`:23:1-2: verb "Time" already exported`)
assert.EqualError(t, err, filepath.Join(pwd, `testdata/duplicateverbs/duplicateverbs.go`)+`:23:1-1: verb "Time" already exported`)
}

0 comments on commit c7d1b4d

Please sign in to comment.