chore: migrate verb schema parsing #1711

worstell · 2024-06-08T00:17:26Z

restructuring and adding some additional passes to provide data for the extractors-

metadata pass parses all comments in the packages and provides results via facts
transitive pass extracts nodes marked as needing extraction via implicit reference from other decls

deniseli

This is awesome!!! Thanks so much for doing this

deniseli · 2024-06-08T00:19:23Z

go-runtime/schema/analyzers/common.go

+	"golang.org/x/exp/maps"
+)
+
+var (


Should we make these const now?

deniseli · 2024-06-08T00:21:52Z

go-runtime/schema/analyzers/initialize.go

+
+// Initializer prepares data prior to the schema extractor runs, e.g. loads FTL types for reference by other
+// analyzers.
+var Initializer = &analysis.Analyzer{


All of this is super nice :D

deniseli · 2024-06-08T00:23:13Z

go-runtime/schema/analyzers/initialize.go

+	ftlTypes map[string]*types.Interface
+}
+
+func (r initializeResult) isFtlErrorType(typ types.Type) bool {


nittiest of nits: isFTLErrorType for go style

I'm also not sure what these functions mean, can you add some comments? What is an "ftl error type", for example?

deniseli · 2024-06-08T00:23:55Z

go-runtime/schema/analyzers/initialize.go

+	return r.assertableToFtlType(typ, "builtin", "error")
+}
+
+func (r initializeResult) isFtlContextType(typ types.Type) bool {


should this just be isContextType since we don't have our own special context type and just use the standard Go one?

deniseli · 2024-06-08T00:25:49Z

go-runtime/schema/analyzers/initialize.go

+		return nil, err
+	}
+	if len(pkgs) != 1 {
+		return nil, errors.New("expected one package")


Could we make this expected one package, got %d? Since that usually saves some time debugging whenever an error like that does appear

+1 and maybe even do something like strings.Join(slices.Map(pkgs, func(p *packages.Package) (string, error) { return p.Name }), ", ")

deniseli · 2024-06-08T00:27:05Z

go-runtime/schema/analyzers/verb.go

+var VerbExtractor = newExtractor("verb", extractVerbs, (*verbExtractorFact)(nil))
+
+type verbExtractorFact struct {
+	decl schema.Decl


would this always be a schema.Verb? Or do we need to leave this as a schema.Decl to make the interface happy?

deniseli · 2024-06-08T00:32:02Z

go-runtime/schema/analyzers/verb.go

+	if results.Len() == 0 {
+		pass.Report(errorf(node, "must at least return an error"))
+	} else if !loaded.isFtlErrorType(results.At(results.Len() - 1).Type()) {
+		pass.Report(tokenErrorf(pass.Fset, results.At(results.Len()-1).Pos(), results.At(results.Len()-1).Name(), "must return an error but is %s", results.At(0).Type()))


In this case, when returning multiple values, should we specify in the error message that last value in the return statement must be error, in case they have something like func(context.Context) (error, struct)?

I think that's what this is already doing isn't it? results.At(results.Len()-1) is the last out value.

Ah I just meant clarifying the error message. The logic itself looks good yeah

deniseli · 2024-06-08T00:34:34Z

go-runtime/schema/extractor.go

@@ -21,7 +21,8 @@ func Extract(moduleDir string) (analyzers.ExtractResult, error) {
 		RunDespiteLoadErrors: true,
 		Patterns:             []string{"./..."},
 	}
-	results, diagnostics, err := checker.Run(cConfig, append(analyzers.Extractors, analyzers.Finalizer)...)
+	as := []*analysis.Analyzer{analyzers.Initializer, analyzers.Finalizer}


Not related to this line, just a general comment: did you happen to check if we need to add any more test coverage? I can't recall if we're missing any significant branch cases for the new approach.

alecthomas

So good!

alecthomas · 2024-06-08T00:45:26Z

go-runtime/schema/analyzers/finalize.go

 	ResultType:       reflect.TypeFor[ExtractResult](),
 	RunDespiteErrors: true,
 }

-// ExtractResult contains the final schema extraction result.
+// ExtractResult contains the final schema extraction extractorResult.


nit: overly enthusiastic type rename

alecthomas · 2024-06-08T01:21:51Z

go-runtime/schema/analyzers/initialize.go

+	ftlTypes map[string]*types.Interface
+}
+
+func (r initializeResult) isFtlErrorType(typ types.Type) bool {


I'm also not sure what these functions mean, can you add some comments? What is an "ftl error type", for example?

alecthomas · 2024-06-08T01:23:27Z

go-runtime/schema/analyzers/initialize.go

+		return nil, err
+	}
+	if len(pkgs) != 1 {
+		return nil, errors.New("expected one package")


+1 and maybe even do something like strings.Join(slices.Map(pkgs, func(p *packages.Package) (string, error) { return p.Name }), ", ")

alecthomas · 2024-06-08T01:24:11Z

go-runtime/schema/analyzers/initialize.go

+	}
+	obj := pkgs[0].Types.Scope().Lookup(name)
+	if obj == nil {
+		return nil, errors.New("interface not found")


Suggested change

return nil, errors.New("interface not found")

return nil, errors.Errorf("interface %q not found", name)

alecthomas · 2024-06-08T01:24:43Z

go-runtime/schema/analyzers/initialize.go

+	}
+	ifaceType, ok := obj.Type().Underlying().(*types.Interface)
+	if !ok {
+		return nil, errors.New("expected an interface, got " + obj.Type().String())


Suggested change

return nil, errors.New("expected an interface, got " + obj.Type().String())

return nil, errors.Errorf("expected an interface, got %s" + obj.Type())

alecthomas · 2024-06-08T02:39:07Z

go-runtime/schema/analyzers/verb.go

+	nodeFilter := []ast.Node{
+		(*ast.FuncDecl)(nil),
+	}
+	in.Preorder(nodeFilter, func(n ast.Node) {


Oh in.Preorder() is super handy, nice nice. We can delete my shitty astwalk function at some point.

alecthomas · 2024-06-08T02:41:21Z

go-runtime/schema/analyzers/verb.go

+		reqV, reqOk := req.Get()
+		resV, respOk := resp.Get()
+		if !reqOk {
+			pass.Report(tokenErrorf(pass.Fset, params.At(1).Pos(), params.At(1).Name(),


alecthomas · 2024-06-08T02:45:39Z

go-runtime/schema/analyzers/verb.go

+	return newExtractorResult(pass, verbs, nn), nil
+}
+
+func extractVerbMetadata(pass *analysis.Pass, node ast.Node, directives []directive) []schema.Metadata {


Metadata extraction could be another pass. We could look for any //ftl: directives on any declaration, and add them as facts to the node. Then in verb analysis, pubsub, types, etc. we just pull the metadata fact directly from the node.

alecthomas · 2024-06-08T02:46:58Z

go-runtime/schema/analyzers/verb.go

+	if results.Len() == 0 {
+		pass.Report(errorf(node, "must at least return an error"))
+	} else if !loaded.isFtlErrorType(results.At(results.Len() - 1).Type()) {
+		pass.Report(tokenErrorf(pass.Fset, results.At(results.Len()-1).Pos(), results.At(results.Len()-1).Name(), "must return an error but is %s", results.At(0).Type()))


I think that's what this is already doing isn't it? results.At(results.Len()-1) is the last out value.

alecthomas · 2024-06-08T02:47:06Z

go-runtime/schema/analyzers/verb.go

+	}
+
+	if results.Len() > 2 {
+		pass.Report(errorf(node, "must have at most two results (struct, error)"))


Suggested change

pass.Report(errorf(node, "must have at most two results (struct, error)"))

pass.Report(errorf(node, "must have at most two results (<type>, error)"))

restructuring and adding some additional passes to provide data for the extractors- 1. metadata pass parses all comments in the packages and provides results via facts 2. transitive pass extracts nodes marked as needing extraction via transitive reference from other decls

worstell · 2024-06-17T20:13:51Z

ok, completely overhauled this change 😅 the scope creep required to get something working was kind of insane. it's quite different now but all comments should be addressed!

this pr has gotten kind of out of hand but the next ones should be muuuuuch better contained for migrating future pieces.

worstell requested a review from alecthomas as a code owner June 8, 2024 00:17

worstell requested review from a team and deniseli and removed request for a team June 8, 2024 00:17

ftl-robot mentioned this pull request Jun 8, 2024

Dashboard #728

Open

worstell force-pushed the worstell/20240607-migrate-verb-schema-parsing branch 2 times, most recently from 3333831 to 4a06dff Compare June 8, 2024 00:21

deniseli approved these changes Jun 8, 2024

View reviewed changes

alecthomas approved these changes Jun 8, 2024

View reviewed changes

worstell force-pushed the worstell/20240607-migrate-verb-schema-parsing branch 17 times, most recently from b084e25 to 07db4f9 Compare June 17, 2024 20:06

worstell force-pushed the worstell/20240607-migrate-verb-schema-parsing branch from 07db4f9 to e0e5a12 Compare June 17, 2024 20:09

worstell merged commit 15b653f into main Jun 17, 2024
41 checks passed

worstell deleted the worstell/20240607-migrate-verb-schema-parsing branch June 17, 2024 20:17

worstell mentioned this pull request Jun 17, 2024

Allow verbs to be defined in subpackages (similar to the type change) #1611

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: migrate verb schema parsing #1711

chore: migrate verb schema parsing #1711

worstell commented Jun 8, 2024 •

edited

Loading

deniseli left a comment

deniseli Jun 8, 2024

deniseli Jun 8, 2024

deniseli Jun 8, 2024

alecthomas Jun 8, 2024

deniseli Jun 8, 2024

deniseli Jun 8, 2024

alecthomas Jun 8, 2024

deniseli Jun 8, 2024

deniseli Jun 8, 2024

alecthomas Jun 8, 2024

deniseli Jun 10, 2024

deniseli Jun 8, 2024

alecthomas left a comment

alecthomas Jun 8, 2024

alecthomas Jun 8, 2024

alecthomas Jun 8, 2024

alecthomas Jun 8, 2024

alecthomas Jun 8, 2024

alecthomas Jun 8, 2024

alecthomas Jun 8, 2024

alecthomas Jun 8, 2024

alecthomas Jun 8, 2024

alecthomas Jun 8, 2024

worstell commented Jun 17, 2024

	return nil, errors.New("interface not found")
	return nil, errors.Errorf("interface %q not found", name)

	return nil, errors.New("expected an interface, got " + obj.Type().String())
	return nil, errors.Errorf("expected an interface, got %s" + obj.Type())

	pass.Report(errorf(node, "must have at most two results (struct, error)"))
	pass.Report(errorf(node, "must have at most two results (<type>, error)"))

chore: migrate verb schema parsing #1711

chore: migrate verb schema parsing #1711

Conversation

worstell commented Jun 8, 2024 • edited Loading

deniseli 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

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

Choose a reason for hiding this comment

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

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

worstell commented Jun 17, 2024

worstell commented Jun 8, 2024 •

edited

Loading