Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: chain schema errors in Go for the LSP #1265

Merged
merged 1 commit into from
Apr 15, 2024
Merged

feat: chain schema errors in Go for the LSP #1265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion buildengine/build_go_test.go
View file
Open in desktop
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{
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
Open in desktop
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
Open in desktop
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"+
Comment on lines +277 to +287
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So good!

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`)
}