feat: add type aliases, refactor go schema parsing #1493

matt2e · 2024-05-15T10:39:26Z

#1238
Adds the ability to declare type aliases in modules

//ftl:typealias
type UserId string

There is a lot of overlap between typealiases and enums. They both redefine an existing type as a new type, and therefore need to make sure our schema parsing does not skip this defined type by looking at the underlying type.
There were also issues if implicit exports, enum cases (and more?) were encountered in the ast tree before we found the typealias or enum definition.
This PR solves that by doing an initial pass just to find all typealias and enum declarations for the module and then doing the normal pass.

Previously typealiases were allowed without any declaration but they would just fall back to the underlying type.
This PR makes this an error as we do not know if this type should be an enum or a type alias. We may want to discuss this more.

fixes #1475 #1476 #1477 #1492 #1449

matt2e · 2024-05-15T10:41:11Z

making this a draft because its failing tests and I'm done for the night

matt2e · 2024-05-15T10:51:20Z

Actually tests are succeeding, now just lint failing (sumtypes). Will submit and fix that tomorroq

wesbillman

Looks good! Love all the tests :)

wesbillman · 2024-05-15T14:24:56Z

backend/schema/validate.go

+ mdecl := scopes.Resolve(*n)
+ if mdecl == nil && n.Module == "" {
 merr = append(merr, errorf(n, "unknown reference %q", n))
 }
- if mdecl := scopes.Resolve(*n); mdecl != nil {
+ if mdecl != nil {


oh nice! :)

wesbillman · 2024-05-15T14:26:55Z

go-runtime/compile/schema_test.go

+ `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`,
+ `133:2-2: type is not declared as an ftl type alias or enum: NonFTLAlias`,


I was a bit confused by this error message at first. I wonder if shuffling it around would make it more clear 🤔

type "NonFTLAlias" is not declared as an ftl type alias or enum

Agreed, but also in general I tend to leave out the typename iff the error is pointing directly at it already, as it's redundant information (not sure if that is the case here).

Switched it to:

type is not declared as an ftl enum or type alias

wesbillman · 2024-05-15T14:29:00Z

go-runtime/compile/testdata/failing/failing.go

@@ -117,3 +119,16 @@ type PrivateData struct{}
 func (PrivateData) exportedTypeEnum() {}

 var invalidConfig = ftl.Config[string]("not an identifier")
+
+// There can be only one


alecthomas · 2024-05-15T19:28:59Z

Added @worstell as she's been touching this code a lot recently!

alecthomas

Huge!

Who knew typealias support would be such a PITA?!

backend/schema/enum.go

backend/schema/module.go

alecthomas · 2024-05-15T19:36:12Z

buildengine/testdata/projects/other/other.go

@@ -15,59 +15,71 @@ type TypeEnum interface {
 tag()
 }

-type Bool bool
+//ftl:typealias


Not for this PR but I'm wondering if we should move these tests into go-runtime

...te/_ftl/go/modules/{{ range .NonMainModules }}{{ push .Name . }}{{ end }}/external_module.go

alecthomas · 2024-05-15T20:01:13Z

go-runtime/compile/schema.go

- if maybeVisitTypeEnumVariant(pctx, node, directives) {
- return
- }
+ maybeVisitTypeEnumVariant(pctx, node, directives)


Is this intentional?

This allowed this case: #1492
Updating it now to collect errors for subsequent type enums it conforms to

Actually no. This is there because even though something is a variant we still need to continue to process it's declaration. eg: a type alias that conforms to am enum.

go-runtime/compile/schema.go

alecthomas · 2024-05-15T20:32:10Z

go-runtime/compile/schema_test.go

+ `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`,
+ `133:2-2: type is not declared as an ftl type alias or enum: NonFTLAlias`,


Agreed, but also in general I tend to leave out the typename iff the error is pointing directly at it already, as it's redundant information (not sure if that is the case here).

# Conflicts: # backend/protos/xyz/block/ftl/v1/schema/schema.pb.go # go-runtime/compile/schema.go # Conflicts: # backend/protos/xyz/block/ftl/v1/schema/schema.pb.go

# Conflicts: # buildengine/testdata/type_registry_main.go # Conflicts: # backend/protos/xyz/block/ftl/v1/schema/schema.pb.go # go-runtime/compile/external-module-template/_ftl/go/modules/{{ range .NonMainModules }}{{ push .Name . }}{{ end }}/external_module.go

matt2e requested review from alecthomas and a team as code owners May 15, 2024 10:39

matt2e requested review from wesbillman and removed request for a team May 15, 2024 10:39

matt2e marked this pull request as draft May 15, 2024 10:39

alecthomas mentioned this pull request May 15, 2024

Dashboard #728

Open

matt2e force-pushed the matt2e/type-alias branch 2 times, most recently from 5336209 to af9a4b1 Compare May 15, 2024 10:47

matt2e marked this pull request as ready for review May 15, 2024 10:51

wesbillman approved these changes May 15, 2024

View reviewed changes

alecthomas requested a review from worstell May 15, 2024 19:28

alecthomas approved these changes May 15, 2024

View reviewed changes

matt2e force-pushed the matt2e/type-alias branch 3 times, most recently from f5c8aed to 9612b84 Compare May 16, 2024 05:03

matt2e changed the title ~~feat: Add type aliases, refactor go schema parsing~~ feat: add type aliases, refactor go schema parsing May 16, 2024

matt2e added 6 commits May 16, 2024 15:33

wip

d9db7ca

# Conflicts: # backend/protos/xyz/block/ftl/v1/schema/schema.pb.go # go-runtime/compile/schema.go # Conflicts: # backend/protos/xyz/block/ftl/v1/schema/schema.pb.go

wip 2

47a1547

# Conflicts: # buildengine/testdata/type_registry_main.go # Conflicts: # backend/protos/xyz/block/ftl/v1/schema/schema.pb.go # go-runtime/compile/external-module-template/_ftl/go/modules/{{ range .NonMainModules }}{{ push .Name . }}{{ end }}/external_module.go

add test case for enum const before enum decl

9f59d5a

use optional rather than ok

8e6f9ac

reword error for non declared type

b9ae1c1

add comment

ef79024

matt2e added 6 commits May 16, 2024 15:36

include an error if a type conforms to more than one type enum

d7a0678

use optionals for getEnumForTypeName

015819a

fix test

b83d060

fix sumtype checks

3cf635e

fix ingress

3e47a17

always sort type enums in error to make them predictable

873eb7f

matt2e force-pushed the matt2e/type-alias branch from 24b4e5b to 873eb7f Compare May 16, 2024 05:36

use “else if” when generating external module

dfd72c7

matt2e merged commit a44e99b into main May 16, 2024
23 checks passed

matt2e deleted the matt2e/type-alias branch May 16, 2024 05:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add type aliases, refactor go schema parsing #1493

feat: add type aliases, refactor go schema parsing #1493

matt2e commented May 15, 2024 •

edited

matt2e commented May 15, 2024

matt2e commented May 15, 2024

wesbillman left a comment

wesbillman May 15, 2024

wesbillman May 15, 2024

alecthomas May 15, 2024

matt2e May 16, 2024

wesbillman May 15, 2024

alecthomas commented May 15, 2024

alecthomas left a comment

alecthomas May 15, 2024

alecthomas May 15, 2024

matt2e May 16, 2024

matt2e May 16, 2024

alecthomas May 15, 2024

feat: add type aliases, refactor go schema parsing #1493

feat: add type aliases, refactor go schema parsing #1493

Conversation

matt2e commented May 15, 2024 • edited

matt2e commented May 15, 2024

matt2e commented May 15, 2024

wesbillman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecthomas commented May 15, 2024

alecthomas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt2e commented May 15, 2024 •

edited