fix: support source and sink func declarations #1070

wesbillman · 2024-03-12T18:23:52Z

SOLVED: Some things to figure out still:

These were solved by assuming anything without a req is a source, anything without a resp, is a sink, and anything without either is empty.

How to use Handle with these new definitions. Since the signature no longer matches the Handle func, what should we do here? The generated code here will fail:

func main() {
  verbConstructor := server.NewUserVerbServer("time",
    server.Handle(time.Time),
    server.Handle(time.Sink), // these both have build errors since they don't match the expected signature
    server.Handle(time.Source),
  )
  plugin.Start(context.Background(), "time", verbConstructor, ftlv1connect.VerbServiceName, ftlv1connect.NewVerbServiceHandler)
}

How should we deal with the fact that the generated code doesn't match the original signature? This also leads to issues using things like ftl.callSink and ftl.callSource since the generated code doesn't match the signature.

// original

//ftl:verb
func Source(context.Context) (SourceResp, error) {
	return SourceResp{}, nil
}

// generated

//ftl:verb
func Source(context.Context, ftl.Unit) (SourceResp, error) {
  panic("Verb stubs should not be called directly, instead use github.com/TBD54566975/ftl/runtime-go/ftl.Call()")
}

alecthomas

Nice! Good start, but you're right, the other two points need to be addressed. LMK what you come up with.

wesbillman · 2024-03-13T20:45:30Z

go-runtime/ftl/call.go

+// SourceToRef returns the FTL reference for a Source.
+func SourceToRef[Resp any](source Source[Resp]) SourceRef {
+	ref := runtime.FuncForPC(reflect.ValueOf(source).Pointer()).Name()
+	return SourceRef(goRefToFTLRef(ref))
+}
+
+// EmptyToRef returns the FTL reference for an Empty.
+func EmptyToRef(empty Empty) VerbRef {
+	ref := runtime.FuncForPC(reflect.ValueOf(empty).Pointer()).Name()
+	return goRefToFTLRef(ref)
+}


This stuff might change once we use a generic Ref everywhere. But this is working for now

@worstell says she has a change for this in flight

Awesome I can work with her to get her change and mine merged and updated here.

wesbillman · 2024-03-13T20:46:27Z

go-runtime/server/server.go

+		return ftl.Unit{}, err
+	}
+
+	return handler(ftl.VerbRef{Module: ref.Module, Name: ref.Name}, verb)


Not sure if this is the best approach here, but it works as intended.

Perfectly acceptable I think.

wesbillman · 2024-03-13T20:47:51Z

kotlin-runtime/ftl-runtime/src/main/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRule.kt

+      require(verb.valueParameters.size >= 1) { "$verbSourcePos Verbs must have at least one argument, ${verb.name} did not" }
+      require(verb.valueParameters.size <= 2) { "$verbSourcePos Verbs must have at most two arguments, ${verb.name} did not" }


@worstell would be good to get your 👀 on some of this kotlin stuff if you have a moment.

alecthomas

LGTM but it would be good to add an integration test for these two cases.

alecthomas · 2024-03-13T20:56:07Z

go-runtime/compile/build.go

@@ -201,6 +211,9 @@ var scaffoldFuncs = scaffolder.FuncMap{
 		}
 		panic(fmt.Sprintf("unsupported value %T", v))
 	},
+	"isSink": func(v schema.Verb) string {
+		return "wes"


alecthomas · 2024-03-13T21:59:20Z

go-runtime/server/server.go

+		return ftl.Unit{}, err
+	}
+
+	return handler(ftl.VerbRef{Module: ref.Module, Name: ref.Name}, verb)


Perfectly acceptable I think.

alecthomas · 2024-03-13T22:00:14Z

...d-sources/ftl/{{ range .ExternalModules }}{{ push .Name . }}{{ end }}/{{ .Name | camel }}.kt

@@ -34,8 +34,19 @@ data class {{.Name|title}}
 {{- else if is "Verb" . }}
 {{.Comments|comment -}}@Verb
 @Ignore
+{{- if and (eq (type $ .Request) "Unit") (eq (type $ .Response) "Unit")}}
+fun {{.Name|lowerCamel}}(context: Context): Unit = throw


You could probably pull fun {{.Name|lowerCamel}}(context: Context out from the if/else bodies if you wanted to DRY this a bit more.

I'll noodle on this one a bit. It makes the template quite a bit tougher for me to read, but maybe I'm just weird :)

I was wondering that too. Just leave it as-is if it's too gross.

wesbillman · 2024-03-13T23:06:47Z

integration/integration_test.go

+func TestPubSub(t *testing.T) {
+	runForRuntimes(t, func(modulesDir string, runtime string, rd runtimeData) []test {
+		return []test{
+			{name: fmt.Sprintf("PubSub%s", rd.testSuffix), assertions: assertions{
+				run("ftl", rd.initOpts...),
+				scaffoldTestData(runtime, "pubsub", rd.modulePath),


I added these under pubsub assuming they can be extended to support pubsub once it's ready for integration tests.

worstell · 2024-03-13T23:08:46Z

kotlin-runtime/ftl-runtime/src/main/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRule.kt

+      if (verb.valueParameters.size == 2) {
+        val reqParam = verb.valueParameters.last()
+        require(reqParam.typeReference?.resolveType()
+          ?.let { it.toClassDescriptor().isData || it.isEmptyBuiltin() || it.isUnit() }


do we want to allow kotlin.Unit as a request type? i thought we were going to infer this from omission with the new change

Ahh good catch! I had this in before I added the size check above and forgot to remove! Thanks for pointing this one out!

worstell · 2024-03-13T23:10:08Z

kotlin-runtime/ftl-runtime/src/main/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRule.kt

-          respClass.fqNameOrNull()?.asString()
-        }"
+      verb.createTypeBindingForReturnType(bindingContext)?.type?.let {
+        require(it.toClassDescriptor().isData || it.isEmptyBuiltin() || it.isUnit()) {


same as above, do we want to look for a null return type instead of a kotlin.Unit return type?

On this one, I think kotlin infers the return type of Unit if it's omitted.

worstell · 2024-03-13T23:35:26Z

kotlin-runtime/ftl-runtime/src/main/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRule.kt

+    var requestRef = Type(unit = xyz.block.ftl.v1.schema.Unit())
+    if (verb.valueParameters.size > 1) {
+      val ref = verb.valueParameters.last()?.let {
+        val position = it.getLineAndColumn().toPosition()
+        return@let it.typeReference?.resolveType()?.toSchemaType(position)
+      }
+      ref?.let { requestRef = it }
+
    }


Suggested change

var requestRef = Type(unit = xyz.block.ftl.v1.schema.Unit())

if (verb.valueParameters.size > 1) {

val ref = verb.valueParameters.last()?.let {

val position = it.getLineAndColumn().toPosition()

return@let it.typeReference?.resolveType()?.toSchemaType(position)

}

ref?.let { requestRef = it }

}

val requestRef = verb.valueParameters.takeIf { it.size > 1 }?.last()?.let {

val position = it.getLineAndColumn().toPosition()

return@let it.typeReference?.resolveType()?.toSchemaType(position)

} ?: Type(unit = xyz.block.ftl.v1.schema.Unit())

worstell · 2024-03-13T23:37:44Z

kotlin-runtime/ftl-runtime/src/main/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRule.kt

+    var returnRef = Type(unit = xyz.block.ftl.v1.schema.Unit())
+    val ref = verb.createTypeBindingForReturnType(bindingContext)?.let {
      val position = it.psiElement.getLineAndColumn().toPosition()
      return@let it.type.toSchemaType(position)
    }


Suggested change

var returnRef = Type(unit = xyz.block.ftl.v1.schema.Unit())

val ref = verb.createTypeBindingForReturnType(bindingContext)?.let {

val position = it.psiElement.getLineAndColumn().toPosition()

return@let it.type.toSchemaType(position)

}

val returnRef = verb.createTypeBindingForReturnType(bindingContext)?.let {

val position = it.psiElement.getLineAndColumn().toPosition()

return@let it.type.toSchemaType(position)

} ?: Type(unit = xyz.block.ftl.v1.schema.Unit())

alecthomas mentioned this pull request Mar 12, 2024

Dashboard #728

Open

alecthomas reviewed Mar 12, 2024

View reviewed changes

wesbillman force-pushed the support-source-and-sink-func-declarations branch 2 times, most recently from 6ebb5d6 to 7945e84 Compare March 13, 2024 20:41

wesbillman marked this pull request as ready for review March 13, 2024 20:42

wesbillman requested review from dhanji and worstell as code owners March 13, 2024 20:42

wesbillman commented Mar 13, 2024

View reviewed changes

wesbillman force-pushed the support-source-and-sink-func-declarations branch from c36342b to 24887ee Compare March 13, 2024 20:46

wesbillman commented Mar 13, 2024

View reviewed changes

alecthomas approved these changes Mar 13, 2024

View reviewed changes

wesbillman force-pushed the support-source-and-sink-func-declarations branch from 6e12590 to 5101fb4 Compare March 13, 2024 23:05

wesbillman commented Mar 13, 2024

View reviewed changes

worstell reviewed Mar 13, 2024

View reviewed changes

wesbillman force-pushed the support-source-and-sink-func-declarations branch from 8220825 to f2778f6 Compare March 13, 2024 23:09

worstell reviewed Mar 13, 2024

View reviewed changes

wesbillman force-pushed the support-source-and-sink-func-declarations branch from f2778f6 to 65356c4 Compare March 13, 2024 23:23

worstell reviewed Mar 13, 2024

View reviewed changes

wesbillman added 6 commits March 14, 2024 09:22

fix: support source and sink func declarations

3458889

working calls to source, sink, empty in go

1a523d5

Add kotlin updates

e60fd42

remove echo and time changes

b12b29b

add some integration tests

6eba799

pr review changes :)

cd12047

wesbillman force-pushed the support-source-and-sink-func-declarations branch from 65356c4 to 3824b82 Compare March 14, 2024 16:34

fix up new Ref types

632022e

wesbillman force-pushed the support-source-and-sink-func-declarations branch from 3824b82 to 632022e Compare March 14, 2024 16:51

wesbillman merged commit bb02db1 into main Mar 14, 2024
11 checks passed

wesbillman deleted the support-source-and-sink-func-declarations branch March 14, 2024 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support source and sink func declarations #1070

fix: support source and sink func declarations #1070

wesbillman commented Mar 12, 2024 •

edited

Loading

alecthomas left a comment

wesbillman Mar 13, 2024

alecthomas Mar 13, 2024

wesbillman Mar 13, 2024

wesbillman Mar 13, 2024

alecthomas Mar 13, 2024

wesbillman Mar 13, 2024

worstell Mar 13, 2024

alecthomas left a comment

alecthomas Mar 13, 2024

wesbillman Mar 13, 2024

alecthomas Mar 13, 2024

alecthomas Mar 13, 2024

wesbillman Mar 13, 2024

alecthomas Mar 13, 2024

wesbillman Mar 13, 2024

worstell Mar 13, 2024

wesbillman Mar 13, 2024

worstell Mar 13, 2024

wesbillman Mar 13, 2024

worstell Mar 13, 2024

worstell Mar 13, 2024

worstell Mar 13, 2024

		require(verb.valueParameters.size >= 1) { "$verbSourcePos Verbs must have at least one argument, ${verb.name} did not" }
		require(verb.valueParameters.size <= 2) { "$verbSourcePos Verbs must have at most two arguments, ${verb.name} did not" }

fix: support source and sink func declarations #1070

fix: support source and sink func declarations #1070

Conversation

wesbillman commented Mar 12, 2024 • edited Loading

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

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

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

wesbillman commented Mar 12, 2024 •

edited

Loading