From 6968ebdae3426ce23a4bb7bc74fe74a3cb6cbc45 Mon Sep 17 00:00:00 2001 From: worstell Date: Sat, 18 May 2024 01:00:32 -0400 Subject: [PATCH] fix: support inlined or aliased structs as type enum variants (#1535) small changes + fixes preempting the refactor fixes #1531, #1527, #1496 --- .../testdata/projects/another/another.go | 1 - buildengine/testdata/projects/other/other.go | 12 +-- buildengine/testdata/type_registry_main.go | 22 ++--- go-runtime/compile/build.go | 13 +++ .../external_module.go | 2 +- go-runtime/compile/schema.go | 96 +++++++++++-------- go-runtime/compile/schema_test.go | 20 +++- .../compile/testdata/failing/failing.go | 6 ++ go-runtime/compile/testdata/one/one.go | 23 +++++ 9 files changed, 131 insertions(+), 64 deletions(-) diff --git a/buildengine/testdata/projects/another/another.go b/buildengine/testdata/projects/another/another.go index 3feeb64258..29db36055a 100644 --- a/buildengine/testdata/projects/another/another.go +++ b/buildengine/testdata/projects/another/another.go @@ -27,7 +27,6 @@ type One int func (One) typeEnum() {} -//ftl:typealias export type Two string func (Two) typeEnum() {} diff --git a/buildengine/testdata/projects/other/other.go b/buildengine/testdata/projects/other/other.go index 01a8c2e900..fba9dbf85c 100644 --- a/buildengine/testdata/projects/other/other.go +++ b/buildengine/testdata/projects/other/other.go @@ -35,21 +35,21 @@ type MyTime time.Time func (MyTime) tag() {} -type List []string +type MyList []string -func (List) tag() {} +func (MyList) tag() {} -type Map map[string]string +type MyMap map[string]string -func (Map) tag() {} +func (MyMap) tag() {} type MyString string func (MyString) tag() {} -type Struct struct{} +type MyStruct struct{} -func (Struct) tag() {} +func (MyStruct) tag() {} type MyOption ftl.Option[string] diff --git a/buildengine/testdata/type_registry_main.go b/buildengine/testdata/type_registry_main.go index 72dbbdcaee..c3fbba29c6 100644 --- a/buildengine/testdata/type_registry_main.go +++ b/buildengine/testdata/type_registry_main.go @@ -32,17 +32,17 @@ func main() { *new(other.B), ), reflection.WithSumType[other.TypeEnum]( - *new(other.Bool), - *new(other.Bytes), - *new(other.Float), - *new(other.Int), - *new(other.Time), - *new(other.List), - *new(other.Map), - *new(other.String), - *new(other.Struct), - *new(other.Option), - *new(other.Unit), + *new(other.MyBool), + *new(other.MyBytes), + *new(other.MyFloat), + *new(other.MyInt), + *new(other.MyTime), + *new(other.MyList), + *new(other.MyMap), + *new(other.MyString), + *new(other.MyStruct), + *new(other.MyOption), + *new(other.MyUnit), ), ) ctx = reflection.ContextWithTypeRegistry(ctx, tr) diff --git a/go-runtime/compile/build.go b/go-runtime/compile/build.go index f027d2fcd2..82b36edac8 100644 --- a/go-runtime/compile/build.go +++ b/go-runtime/compile/build.go @@ -350,6 +350,19 @@ var scaffoldFuncs = scaffolder.FuncMap{ return out }, "schemaType": schemaType, + // A standalone enum variant is one that is purely an alias to a type and does not appear + // elsewhere in the schema. + "isStandaloneEnumVariant": func(v schema.EnumVariant) bool { + tv, ok := v.Value.(*schema.TypeValue) + if !ok { + return false + } + if ref, ok := tv.Value.(*schema.Ref); ok { + return ref.Name != v.Name + } + + return false + }, } func schemaType(t schema.Type) string { diff --git a/go-runtime/compile/external-module-template/_ftl/go/modules/{{ range .NonMainModules }}{{ push .Name . }}{{ end }}/external_module.go b/go-runtime/compile/external-module-template/_ftl/go/modules/{{ range .NonMainModules }}{{ push .Name . }}{{ end }}/external_module.go index 3af5a9754c..2c55ad8d27 100644 --- a/go-runtime/compile/external-module-template/_ftl/go/modules/{{ range .NonMainModules }}{{ push .Name . }}{{ end }}/external_module.go +++ b/go-runtime/compile/external-module-template/_ftl/go/modules/{{ range .NonMainModules }}{{ push .Name . }}{{ end }}/external_module.go @@ -31,7 +31,7 @@ const ( {{$enumInterfaceFuncName := enumInterfaceFunc . -}} type {{.Name|title}} interface { {{$enumInterfaceFuncName}}() } {{- range .Variants }} -{{if basicType $ .}} +{{if (or (basicType $ .) (isStandaloneEnumVariant .))}} type {{.Name|title}} {{type $ .Value.Value}} {{end}} func ({{.Name|title}}) {{$enumInterfaceFuncName}}() {} diff --git a/go-runtime/compile/schema.go b/go-runtime/compile/schema.go index c9a7e1649e..9632a05ec8 100644 --- a/go-runtime/compile/schema.go +++ b/go-runtime/compile/schema.go @@ -239,7 +239,7 @@ func extractTypeDeclsForNode(pctx *parseContext, node *ast.GenDecl) { switch dir := dir.(type) { case *directiveEnum: typ := pctx.pkg.TypesInfo.TypeOf(t.Type) - switch typ.Underlying().(type) { + switch underlying := typ.Underlying().(type) { case *types.Basic: enum := &schema.Enum{ Pos: goPosToSchemaPos(node.Pos()), @@ -251,6 +251,23 @@ func extractTypeDeclsForNode(pctx *parseContext, node *ast.GenDecl) { pctx.module.Decls = append(pctx.module.Decls, enum) pctx.nativeNames[enum] = nativeName case *types.Interface: + if underlying.NumMethods() == 0 { + pctx.errors.add(errorf(node, "enum discriminator %q must define at least one method", t.Name.Name)) + break + } + + hasExportedMethod := false + for i, n := 0, underlying.NumMethods(); i < n; i++ { + if underlying.Method(i).Exported() { + pctx.errors.add(noEndColumnErrorf(underlying.Method(i).Pos(), "enum discriminator %q cannot "+ + "contain exported methods", t.Name.Name)) + hasExportedMethod = true + } + } + if hasExportedMethod { + break + } + enum := &schema.Enum{ Pos: goPosToSchemaPos(node.Pos()), Comments: visitComments(node.Doc), @@ -670,7 +687,7 @@ func visitGenDecl(pctx *parseContext, node *ast.GenDecl) { } } } else if _, ok := dir.(*directiveTypeAlias); ok { - decl, ok := pctx.getDeclForTypeName(t.Name.Name) + decl, ok := pctx.getDeclForTypeName(t.Name.Name).Get() if !ok { pctx.errors.add(errorf(node, "could not find type alias declaration for %q", t.Name.Name)) return @@ -781,10 +798,7 @@ func maybeVisitTypeEnumVariant(pctx *parseContext, node *ast.GenDecl, directives isExported := enum.IsExported() for _, dir := range directives { if exportableDir, ok := dir.(exportable); ok { - if enum.Export && !exportableDir.IsExported() { - pctx.errors.add(errorf(node, "parent enum %q is exported, but directive %q on %q is not: all variants of exported enums that have a directive must be explicitly exported as well", enumName, exportableDir, t.Name.Name)) - } - isExported = exportableDir.IsExported() + isExported = exportableDir.IsExported() || isExported } } vType, ok := visitTypeValue(pctx, named, t.Type, nil, isExported).Get() @@ -864,7 +878,11 @@ func visitTypeValue(pctx *parseContext, named *types.Named, tnode ast.Expr, inde } default: - if typ, ok := visitType(pctx, tnode.Pos(), named, isExported).Get(); ok { + variantNode := pctx.pkg.TypesInfo.TypeOf(tnode) + if _, ok := variantNode.(*types.Struct); ok { + variantNode = named + } + if typ, ok := visitType(pctx, tnode.Pos(), variantNode, isExported).Get(); ok { return optional.Some(&schema.TypeValue{Value: typ}) } else { pctx.errors.add(errorf(tnode, "unsupported type %q for type enum variant", named)) @@ -1233,20 +1251,14 @@ func visitType(pctx *parseContext, pos token.Pos, tnode types.Type, isExported b if tparam, ok := tnode.(*types.TypeParam); ok { return optional.Some[schema.Type](&schema.Ref{Pos: goPosToSchemaPos(pos), Name: tparam.Obj().Id()}) } + if named, ok := tnode.(*types.Named); ok { // Handle refs to type aliases and enums, rather than the underlying type. - decl, ok := pctx.getDeclForTypeName(named.Obj().Name()) + decl, ok := pctx.getDeclForTypeName(named.Obj().Name()).Get() if ok { switch decl.(type) { case *schema.TypeAlias, *schema.Enum: - if isExported { - pctx.markAsExported(decl) - } - return optional.Some[schema.Type](&schema.Ref{ - Pos: goPosToSchemaPos(pos), - Module: pctx.module.Name, - Name: strcase.ToUpperCamel(named.Obj().Name()), - }) + return visitNamedRef(pctx, pos, named, isExported) case *schema.Data, *schema.Verb, *schema.Config, *schema.Secret, *schema.Database, *schema.FSM: } } @@ -1255,12 +1267,8 @@ func visitType(pctx *parseContext, pos token.Pos, tnode types.Type, isExported b switch underlying := tnode.Underlying().(type) { case *types.Basic: if named, ok := tnode.(*types.Named); ok { - ref, doneWithVisit := visitNamedRef(pctx, pos, named) - if doneWithVisit { - return ref - } + return visitNamedRef(pctx, pos, named, isExported) } - switch underlying.Kind() { case types.String: return optional.Some[schema.Type](&schema.String{Pos: goPosToSchemaPos(pos)}) @@ -1322,10 +1330,7 @@ func visitType(pctx *parseContext, pos token.Pos, tnode types.Type, isExported b return optional.Some[schema.Type](&schema.Any{Pos: goPosToSchemaPos(pos)}) } if named, ok := tnode.(*types.Named); ok { - ref, doneWithVisit := visitNamedRef(pctx, pos, named) - if doneWithVisit { - return ref - } + return visitNamedRef(pctx, pos, named, isExported) } return optional.None[schema.Type]() @@ -1334,29 +1339,36 @@ func visitType(pctx *parseContext, pos token.Pos, tnode types.Type, isExported b } } -func visitNamedRef(pctx *parseContext, pos token.Pos, named *types.Named) (optional.Option[schema.Type], bool) { +func visitNamedRef(pctx *parseContext, pos token.Pos, named *types.Named, isExported bool) optional.Option[schema.Type] { if named.Obj().Pkg() == nil { - return optional.None[schema.Type](), false + return optional.None[schema.Type]() + } + + // Update the visibility of the reference if the referencer is exported (ensuring refs are transitively + // exported as needed). + if isExported { + if decl, ok := pctx.getDeclForTypeName(named.Obj().Name()).Get(); ok { + pctx.markAsExported(decl) + } } + nodePath := named.Obj().Pkg().Path() - var ref *schema.Ref + destModule := pctx.module.Name if !pctx.isPathInPkg(nodePath) { if !strings.HasPrefix(named.Obj().Pkg().Path(), "ftl/") { pctx.errors.add(noEndColumnErrorf(pos, "unsupported external type %q", named.Obj().Pkg().Path()+"."+named.Obj().Name())) - return optional.None[schema.Type](), true + return optional.None[schema.Type]() } base := path.Dir(pctx.pkg.PkgPath) - destModule := path.Base(strings.TrimPrefix(nodePath, base+"/")) - ref = &schema.Ref{ - Pos: goPosToSchemaPos(pos), - Module: destModule, - Name: named.Obj().Name(), - } - return optional.Some[schema.Type](ref), true + destModule = path.Base(strings.TrimPrefix(nodePath, base+"/")) } - return optional.None[schema.Type](), false - + ref := &schema.Ref{ + Pos: goPosToSchemaPos(pos), + Module: destModule, + Name: strcase.ToUpperCamel(named.Obj().Name()), + } + return optional.Some[schema.Type](ref) } func visitMap(pctx *parseContext, pos token.Pos, tnode *types.Map, isExported bool) optional.Option[schema.Type] { @@ -1499,7 +1511,7 @@ func (p *parseContext) isPathInPkg(path string) bool { // getEnumForTypeName returns the enum and interface for a given type name. func (p *parseContext) getEnumForTypeName(name string) (optional.Option[*schema.Enum], optional.Option[*types.Interface]) { - aDecl, ok := p.getDeclForTypeName(name) + aDecl, ok := p.getDeclForTypeName(name).Get() if !ok { return optional.None[*schema.Enum](), optional.None[*types.Interface]() } @@ -1518,7 +1530,7 @@ func (p *parseContext) getEnumForTypeName(name string) (optional.Option[*schema. return optional.Some(decl), optional.None[*types.Interface]() } -func (p *parseContext) getDeclForTypeName(name string) (enum schema.Decl, ok bool) { +func (p *parseContext) getDeclForTypeName(name string) optional.Option[schema.Decl] { for _, decl := range p.module.Decls { nativeName, ok := p.nativeNames[decl] if !ok { @@ -1527,9 +1539,9 @@ func (p *parseContext) getDeclForTypeName(name string) (enum schema.Decl, ok boo if nativeName != p.pkg.Name+"."+name { continue } - return decl, true + return optional.Some(decl) } - return nil, false + return optional.None[schema.Decl]() } func (p *parseContext) markAsExported(node schema.Node) { diff --git a/go-runtime/compile/schema_test.go b/go-runtime/compile/schema_test.go index ccb971501b..ed28941be6 100644 --- a/go-runtime/compile/schema_test.go +++ b/go-runtime/compile/schema_test.go @@ -65,7 +65,7 @@ func TestExtractModuleSchema(t *testing.T) { } // Comments about ColorInt. - enum ColorInt: Int { + export enum ColorInt: Int { // RedInt is a color. RedInt = 0 BlueInt = 1 @@ -92,6 +92,13 @@ func TestExtractModuleSchema(t *testing.T) { Two = 2 } + export enum TypeEnum { + Option String? + InlineStruct one.InlineStruct + AliasedStruct one.UnderlyingStruct + ValueEnum one.ColorInt + } + data Config { field String } @@ -107,6 +114,9 @@ func TestExtractModuleSchema(t *testing.T) { export data ExportedStruct { } + export data InlineStruct { + } + export data Nested { } @@ -140,6 +150,9 @@ func TestExtractModuleSchema(t *testing.T) { data SourceResp { } + export data UnderlyingStruct { + } + data WithoutDirectiveStruct { } @@ -310,7 +323,7 @@ func TestExtractModuleSchemaNamedTypes(t *testing.T) { assert.Equal(t, normaliseString(expected), normaliseString(actual.String())) } -func TestParseDirectives(t *testing.T) { +func TestParsedirectives(t *testing.T) { tests := []struct { name string input string @@ -439,10 +452,11 @@ func TestErrorReporting(t *testing.T) { `89:2-12: struct field unexported must be exported by starting with an uppercase letter`, `101:2-24: cannot attach enum value to BadValueEnum because it is a variant of type enum TypeEnum, not a value enum`, `108:2-41: cannot attach enum value to BadValueEnumOrderDoesntMatter because it is a variant of type enum TypeEnum, not a value enum`, - `117:1-26: parent enum "ExportedTypeEnum" is exported, but directive "ftl:data" on "PrivateData" is not: all variants of exported enums that have a directive must be explicitly exported as well`, `121:21-60: config and secret names must be valid identifiers`, `127:1-26: only one directive expected for type alias`, `143:1-35: type can not be a variant of more than 1 type enums (TypeEnum1, TypeEnum2)`, + `149:27-27: enum discriminator "TypeEnum3" cannot contain exported methods`, + `152:1-35: enum discriminator "NoMethodsTypeEnum" must define at least one method`, } assert.Equal(t, expected, actual) } diff --git a/go-runtime/compile/testdata/failing/failing.go b/go-runtime/compile/testdata/failing/failing.go index 880db2b379..65f6c473bc 100644 --- a/go-runtime/compile/testdata/failing/failing.go +++ b/go-runtime/compile/testdata/failing/failing.go @@ -144,3 +144,9 @@ type ConformsToTwoTypeEnums string func (ConformsToTwoTypeEnums) typeEnum1() {} func (ConformsToTwoTypeEnums) typeEnum2() {} + +//ftl:enum +type TypeEnum3 interface{ ExportedMethod() } + +//ftl:enum +type NoMethodsTypeEnum interface{} diff --git a/go-runtime/compile/testdata/one/one.go b/go-runtime/compile/testdata/one/one.go index 6f7140a4fa..48dca4ac9b 100644 --- a/go-runtime/compile/testdata/one/one.go +++ b/go-runtime/compile/testdata/one/one.go @@ -64,6 +64,29 @@ func (List) blobOrList() {} type Nested struct { } +//ftl:enum +type TypeEnum interface { + tag() +} + +type Option ftl.Option[string] + +func (Option) tag() {} + +type InlineStruct struct{} + +func (InlineStruct) tag() {} + +type AliasedStruct UnderlyingStruct + +func (AliasedStruct) tag() {} + +type UnderlyingStruct struct{} + +type ValueEnum ColorInt + +func (ValueEnum) tag() {} + //ftl:enum type PrivateEnum interface{ privateEnum() }