diff --git a/backend/schema/errors.go b/backend/schema/errors.go index f1105c7ef5..48d75383ad 100644 --- a/backend/schema/errors.go +++ b/backend/schema/errors.go @@ -3,6 +3,8 @@ package schema import ( "errors" "fmt" + + schemapb "github.com/TBD54566975/ftl/backend/protos/xyz/block/ftl/v1/schema" ) type Error struct { @@ -12,8 +14,32 @@ type Error struct { Err error `protobuf:"-"` // Wrapped error, if any } +func errorFromProto(e *schemapb.Error) *Error { + return &Error{ + Pos: posFromProto(e.Pos), + Msg: e.Msg, + EndColumn: int(e.EndColumn), + } +} + +func errorsFromProto(errs []*schemapb.Error) []*Error { + var out []*Error + for _, pb := range errs { + s := errorFromProto(pb) + out = append(out, s) + } + return out +} + type ErrorList struct { - Errors []Error `json:"errors" protobuf:"1"` + Errors []*Error `json:"errors" protobuf:"1"` +} + +// ErrorListFromProto converts a protobuf ErrorList to an ErrorList. +func ErrorListFromProto(e *schemapb.ErrorList) *ErrorList { + return &ErrorList{ + Errors: errorsFromProto(e.Errors), + } } func (e Error) Error() string { return fmt.Sprintf("%s-%d: %s", e.Pos, e.EndColumn, e.Msg) } diff --git a/buildengine/build_go_test.go b/buildengine/build_go_test.go index 3a259632ea..657442c91d 100644 --- a/buildengine/build_go_test.go +++ b/buildengine/build_go_test.go @@ -125,7 +125,7 @@ func Nothing(context.Context) error { buildDir: "_ftl", sch: sch, } - testBuild(t, bctx, []assertion{ + testBuild(t, bctx, false, []assertion{ assertGeneratedModule("go/modules/other/external_module.go", expected), }) } @@ -174,7 +174,7 @@ func Call(context.Context, Req) (Resp, error) { buildDir: "_ftl", sch: sch, } - testBuild(t, bctx, []assertion{ + testBuild(t, bctx, false, []assertion{ assertGeneratedModule("go/modules/test/external_module.go", expected), }) } diff --git a/buildengine/build_kotlin.go b/buildengine/build_kotlin.go index 2bdb685a64..c4e94c2979 100644 --- a/buildengine/build_kotlin.go +++ b/buildengine/build_kotlin.go @@ -2,6 +2,7 @@ package buildengine import ( "context" + "errors" "fmt" "os" "path/filepath" @@ -13,8 +14,10 @@ import ( "github.com/beevik/etree" sets "github.com/deckarep/golang-set/v2" "golang.org/x/exp/maps" + "google.golang.org/protobuf/proto" "github.com/TBD54566975/ftl" + schemapb "github.com/TBD54566975/ftl/backend/protos/xyz/block/ftl/v1/schema" "github.com/TBD54566975/ftl/backend/schema" "github.com/TBD54566975/ftl/internal" "github.com/TBD54566975/ftl/internal/exec" @@ -59,7 +62,16 @@ func buildKotlinModule(ctx context.Context, sch *schema.Schema, module Module) e logger.Debugf("Using build command '%s'", module.Build) err := exec.Command(ctx, log.Debug, module.Dir, "bash", "-c", module.Build).RunBuffered(ctx) if err != nil { - return fmt.Errorf("failed to build module %q: %w", module.Module, err) + // read runtime-specific build errors from the build directory + errorList, err := loadProtoErrors(module.AbsDeployDir()) + if err != nil { + return fmt.Errorf("failed to read build errors for module: %w", err) + } + errs := make([]error, 0, len(errorList.Errors)) + for _, e := range errorList.Errors { + errs = append(errs, *e) + } + return errors.Join(errs...) } return nil @@ -249,3 +261,21 @@ func genType(module *schema.Module, t schema.Type) string { } panic(fmt.Sprintf("unsupported type %T", t)) } + +func loadProtoErrors(buildDir string) (*schema.ErrorList, error) { + f := filepath.Join(buildDir, "errors.pb") + if _, err := os.Stat(f); errors.Is(err, os.ErrNotExist) { + return &schema.ErrorList{Errors: make([]*schema.Error, 0)}, nil + } + + content, err := os.ReadFile(f) + if err != nil { + return nil, err + } + errorspb := &schemapb.ErrorList{} + err = proto.Unmarshal(content, errorspb) + if err != nil { + return nil, err + } + return schema.ErrorListFromProto(errorspb), nil +} diff --git a/buildengine/build_kotlin_test.go b/buildengine/build_kotlin_test.go index 70dafe9b4a..1e6729db14 100644 --- a/buildengine/build_kotlin_test.go +++ b/buildengine/build_kotlin_test.go @@ -1,15 +1,9 @@ package buildengine import ( - "bytes" - "context" - "os" "testing" - "github.com/alecthomas/assert/v2" - "github.com/TBD54566975/ftl/backend/schema" - "github.com/TBD54566975/ftl/internal/log" ) func TestGenerateBasicModule(t *testing.T) { @@ -31,7 +25,7 @@ package ftl.test buildDir: "target", sch: sch, } - testBuild(t, bctx, []assertion{ + testBuild(t, bctx, false, []assertion{ assertGeneratedModule("generated-sources/ftl/test/Test.kt", expected), }) } @@ -158,7 +152,7 @@ data class TestResponse( buildDir: "target", sch: sch, } - testBuild(t, bctx, []assertion{ + testBuild(t, bctx, false, []assertion{ assertGeneratedModule("generated-sources/ftl/test/Test.kt", expected), }) } @@ -219,7 +213,7 @@ fun testVerb(context: Context, req: Request): ftl.builtin.Empty = throw buildDir: "target", sch: sch, } - testBuild(t, bctx, []assertion{ + testBuild(t, bctx, false, []assertion{ assertGeneratedModule("generated-sources/ftl/test/Test.kt", expected), }) } @@ -273,7 +267,7 @@ class Empty buildDir: "target", sch: sch, } - testBuild(t, bctx, []assertion{ + testBuild(t, bctx, false, []assertion{ assertGeneratedModule("generated-sources/ftl/builtin/Builtin.kt", expected), }) } @@ -317,7 +311,7 @@ fun emptyVerb(context: Context, req: ftl.builtin.Empty): ftl.builtin.Empty = thr buildDir: "target", sch: sch, } - testBuild(t, bctx, []assertion{ + testBuild(t, bctx, false, []assertion{ assertGeneratedModule("generated-sources/ftl/test/Test.kt", expected), }) } @@ -397,7 +391,7 @@ fun nothing(context: Context): Unit = throw buildDir: "target", sch: sch, } - testBuild(t, bctx, []assertion{ + testBuild(t, bctx, false, []assertion{ assertGeneratedModule("generated-sources/ftl/test/Test.kt", expected), }) } @@ -406,22 +400,12 @@ func TestKotlinExternalType(t *testing.T) { if testing.Short() { t.SkipNow() } - moduleDir := "testdata/projects/externalkotlin" - buildDir := "_ftl" - - ctx := log.ContextWithLogger(context.Background(), log.Configure(os.Stderr, log.Config{})) - module, err := LoadModule(moduleDir) - assert.NoError(t, err) - - logBuffer := bytes.Buffer{} - logger := log.Configure(&logBuffer, log.Config{}) - ctx = log.ContextWithLogger(ctx, logger) - - sch := &schema.Schema{} - err = Build(ctx, sch, module) - assert.Error(t, err) - assert.Contains(t, logBuffer.String(), "Expected module name to be in the form ftl., but was com.google.type.DayOfWeek") - - err = os.RemoveAll(buildDir) - assert.NoError(t, err, "Error removing build directory") + bctx := buildContext{ + moduleDir: "testdata/projects/externalkotlin", + buildDir: "target", + sch: &schema.Schema{}, + } + testBuild(t, bctx, true, []assertion{ + assertBuildProtoErrors("expected module name to be in the form ftl., but was com.google.type.DayOfWeek"), + }) } diff --git a/buildengine/build_test.go b/buildengine/build_test.go index 0ccadcc9c9..9b973d175d 100644 --- a/buildengine/build_test.go +++ b/buildengine/build_test.go @@ -23,22 +23,28 @@ type assertion func(t testing.TB, bctx buildContext) error func testBuild( t *testing.T, bctx buildContext, + expectFail bool, assertions []assertion, ) { t.Helper() ctx := log.ContextWithLogger(context.Background(), log.Configure(os.Stderr, log.Config{})) - module, err := LoadModule(bctx.moduleDir) + abs, err := filepath.Abs(bctx.moduleDir) + assert.NoError(t, err, "Error getting absolute path for module directory") + module, err := LoadModule(abs) assert.NoError(t, err) - err = Build(ctx, bctx.sch, module) - assert.NoError(t, err) + if expectFail { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } for _, a := range assertions { err = a(t, bctx) assert.NoError(t, err) } - err = os.RemoveAll(bctx.buildDir) + err = os.RemoveAll(filepath.Join(bctx.moduleDir, bctx.buildDir)) assert.NoError(t, err, "Error removing build directory") } @@ -54,3 +60,24 @@ func assertGeneratedModule(generatedModulePath string, expectedContent string) a return nil } } + +func assertBuildProtoErrors(msgs ...string) assertion { + return func(t testing.TB, bctx buildContext) error { + t.Helper() + errorList, err := loadProtoErrors(filepath.Join(bctx.moduleDir, bctx.buildDir)) + assert.NoError(t, err, "Error loading proto errors") + + expected := make([]*schema.Error, 0, len(msgs)) + for _, msg := range msgs { + expected = append(expected, &schema.Error{Msg: msg}) + } + + // normalize results + for _, e := range errorList.Errors { + e.EndColumn = 0 + } + + assert.Equal(t, errorList.Errors, expected, assert.Exclude[schema.Position]()) + return nil + } +} diff --git a/internal/exec/exec.go b/internal/exec/exec.go index 5ae21ce8cb..2981ddc643 100644 --- a/internal/exec/exec.go +++ b/internal/exec/exec.go @@ -2,7 +2,6 @@ package exec import ( "context" - "fmt" "os" "os/exec" //nolint:depguard "syscall" @@ -59,7 +58,7 @@ func (c *Cmd) RunBuffered(ctx context.Context) error { err := c.Run() if err != nil { - fmt.Printf("%s", outputBuffer.Bytes()) + log.FromContext(ctx).Infof("%s", outputBuffer.Bytes()) return err } diff --git a/kotlin-runtime/ftl-runtime/src/main/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRule.kt b/kotlin-runtime/ftl-runtime/src/main/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRule.kt index 07500e7b8c..8aacbde3e8 100644 --- a/kotlin-runtime/ftl-runtime/src/main/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRule.kt +++ b/kotlin-runtime/ftl-runtime/src/main/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRule.kt @@ -67,6 +67,8 @@ import xyz.block.ftl.v1.schema.Data import xyz.block.ftl.v1.schema.Decl import xyz.block.ftl.v1.schema.Enum import xyz.block.ftl.v1.schema.EnumVariant +import xyz.block.ftl.v1.schema.Error +import xyz.block.ftl.v1.schema.ErrorList import xyz.block.ftl.v1.schema.Field import xyz.block.ftl.v1.schema.IngressPathComponent import xyz.block.ftl.v1.schema.IngressPathLiteral @@ -160,18 +162,27 @@ class ExtractSchemaRule(config: DetektConfig) : Rule(config) { } override fun postVisit(root: KtFile) { + val errors = extractor.getErrors() + if (errors.errors.isNotEmpty()) { + writeFile(errors.encode(), ERRORS_OUT) + throw IllegalStateException("could not extract schema") + } + val moduleName = root.extractModuleName() modules[moduleName]?.let { - val module = it.toModule(moduleName) - val outputDirectory = File(output).also { f -> Path.of(f.absolutePath).createDirectories() } - val file = File(outputDirectory.absolutePath, OUTPUT_FILENAME) - file.createNewFile() - val os = FileOutputStream(file) - os.write(module.encode()) - os.close() + writeFile(it.toModule(moduleName).encode(), SCHEMA_OUT) } } + private fun writeFile(content: ByteArray, filename: String) { + val outputDirectory = File(output).also { f -> Path.of(f.absolutePath).createDirectories() } + val file = File(outputDirectory.absolutePath, filename) + file.createNewFile() + val os = FileOutputStream(file) + os.write(content) + os.close() + } + private fun ModuleData.toModule(moduleName: String): Module = Module( name = moduleName, decls = this.decls.sortedBy { it.data_ == null }, @@ -179,7 +190,8 @@ class ExtractSchemaRule(config: DetektConfig) : Rule(config) { ) companion object { - const val OUTPUT_FILENAME = "schema.pb" + const val SCHEMA_OUT = "schema.pb" + const val ERRORS_OUT = "errors.pb" } } @@ -187,11 +199,14 @@ class SchemaExtractor( private val modules: MutableMap, ) { private var bindingContext = BindingContext.EMPTY + private val errors = mutableListOf() fun setBindingContext(bindingContext: BindingContext) { this.bindingContext = bindingContext } + fun getErrors(): ErrorList = ErrorList(errors = errors) + fun addModuleComments(file: KtFile) { val module = file.extractModuleName() val comments = file.children @@ -245,18 +260,17 @@ class SchemaExtractor( fun addDatabaseToSchema(database: KtProperty) { val decl = database.children.single().let { - val sourcePos = it.getPosition() val dbName = (it as? KtCallExpression).getResolvedCall(bindingContext)?.valueArguments?.entries?.single { e -> e.key.name.asString() == "name" } ?.value?.toString() ?.trim('"') - requireNotNull(dbName) { "$sourcePos $dbName Could not extract database name" } + require(dbName != null) { it.errorAtPosition("could not extract database name") } Decl( database = xyz.block.ftl.v1.schema.Database( - pos = sourcePos, - name = dbName + pos = it.getPosition(), + name = dbName ?: "" ) ) } @@ -270,64 +284,56 @@ class SchemaExtractor( } private fun validateVerb(verb: KtNamedFunction) { - val verbSourcePos = verb.getLineAndColumn() - requireNotNull(verb.fqName?.asString()) { - "Verbs must be defined in a package" - }.let { fqName -> + require(verb.fqName?.asString() != null) { verb.errorAtPosition("verbs must be defined in a package") } + verb.fqName?.asString()!!.let { fqName -> require(fqName.split(".").let { it.size >= 2 && it.first() == "ftl" }) { - "$verbSourcePos Expected exported function to be in package ftl., but was $fqName" + verb.errorAtPosition("expected exported function to be in package ftl., but was $fqName") } // Validate parameters - 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" } + require(verb.valueParameters.size >= 1) { verb.errorAtPosition("verbs must have at least one argument") } + require(verb.valueParameters.size <= 2) { verb.errorAtPosition("verbs must have at most two arguments") } val ctxParam = verb.valueParameters.first() - require(ctxParam.typeReference?.resolveType()?.fqNameOrNull()?.asString() == Context::class.qualifiedName) { - "${verb.valueParameters.first().getLineAndColumn()} First argument of verb must be Context" + ctxParam.errorAtPosition("first argument of verb must be Context") } if (verb.valueParameters.size == 2) { val reqParam = verb.valueParameters.last() require(reqParam.typeReference?.resolveType() - ?.let { it.toClassDescriptor().isData || it.isEmptyBuiltin() } - ?: false + ?.let { it.toClassDescriptor().isData || it.isEmptyBuiltin() } == true ) { - "${verb.valueParameters.last().getLineAndColumn()} Second argument of ${verb.name} must be a data class or " + - "builtin.Empty" + verb.valueParameters.last().errorAtPosition("request type must be a data class or builtin.Empty") } } // Validate return type verb.createTypeBindingForReturnType(bindingContext)?.type?.let { require(it.toClassDescriptor().isData || it.isEmptyBuiltin() || it.isUnit()) { - "${verbSourcePos}: return type of ${verb.name} must be a data class or builtin.Empty but is ${ - it.fqNameOrNull()?.asString() - }" + verb.errorAtPosition("if present, return type must be a data class or builtin.Empty") } } } } private fun extractVerb(verb: KtNamedFunction): Verb { - val verbSourcePos = verb.getLineAndColumn() - val requestRef = verb.valueParameters.takeIf { it.size > 1 }?.last()?.let { - val position = it.getPosition() - return@let it.typeReference?.resolveType()?.toSchemaType(position) - } ?: Type(unit = xyz.block.ftl.v1.schema.Unit()) + return@let it.typeReference?.resolveType()?.toSchemaType(it.getPosition(), it.textLength) + } ?: Type(unit = Unit()) val returnRef = verb.createTypeBindingForReturnType(bindingContext)?.let { - val position = it.psiElement.getPosition() - return@let it.type.toSchemaType(position) + return@let it.type.toSchemaType(it.psiElement.getPosition(), it.psiElement.textLength) } ?: Type(unit = Unit()) val metadata = mutableListOf() extractIngress(verb, requestRef, returnRef)?.apply { metadata.add(Metadata(ingress = this)) } extractCalls(verb)?.apply { metadata.add(Metadata(calls = this)) } + require(verb.name != null) { + verb.errorAtPosition("verbs must be named") + } return Verb( - name = requireNotNull(verb.name) { "$verbSourcePos Verbs must be named" }, + name = verb.name ?: "", request = requestRef, response = returnRef, metadata = metadata, @@ -339,7 +345,6 @@ class SchemaExtractor( private fun extractSecretOrConfig(property: KtProperty): SecretConfigData { return property.children.single().let { - val position = it.getPosition() var type: KotlinType? = null var name = "" when (it) { @@ -361,11 +366,12 @@ class SchemaExtractor( } else -> { - throw IllegalArgumentException("$position: Could not extract secret or config") + errors.add(it.errorAtPosition("could not extract secret or config")) } } - SecretConfigData(name, type!!.toSchemaType(position), position) + val position = it.getPosition() + SecretConfigData(name, type!!.toSchemaType(position, it.textLength), position) } } @@ -373,21 +379,20 @@ class SchemaExtractor( return verb.annotationEntries.firstOrNull { bindingContext.get(BindingContext.ANNOTATION, it)?.fqName?.asString() == HttpIngress::class.qualifiedName }?.let { annotationEntry -> - val sourcePos = annotationEntry.getLineAndColumn() require(requestType.ref != null) { - "$sourcePos ingress ${verb.name} request must be a data class" + annotationEntry.errorAtPosition("ingress ${verb.name} request must be a data class") } require(responseType.ref != null) { - "$sourcePos ingress ${verb.name} response must be a data class" + annotationEntry.errorAtPosition("ingress ${verb.name} response must be a data class") } - require(requestType.ref.compare("builtin", "HttpRequest")) { - "$sourcePos @HttpIngress-annotated ${verb.name} request must be ftl.builtin.HttpRequest" + require(requestType.ref?.compare("builtin", "HttpRequest") == true) { + annotationEntry.errorAtPosition("@HttpIngress-annotated ${verb.name} request must be ftl.builtin.HttpRequest") } - require(responseType.ref.compare("builtin", "HttpResponse")) { - "$sourcePos @HttpIngress-annotated ${verb.name} response must be ftl.builtin.HttpResponse" + require(responseType.ref?.compare("builtin", "HttpResponse") == true) { + annotationEntry.errorAtPosition("@HttpIngress-annotated ${verb.name} response must be ftl.builtin.HttpResponse") } require(annotationEntry.valueArguments.size >= 2) { - "$sourcePos ${verb.name} @HttpIngress annotation requires at least 2 arguments" + annotationEntry.errorAtPosition("@HttpIngress annotation requires at least 2 arguments") } val args = annotationEntry.valueArguments.partition { arg -> @@ -398,20 +403,23 @@ class SchemaExtractor( ?.asString() == Method::class.qualifiedName } - val methodArg = requireNotNull(args.first.single().getArgumentExpression()?.text?.substringAfter(".")) { - "Could not extract method from ${verb.name} @HttpIngress annotation" + val methodArg = args.first.single().getArgumentExpression()?.text?.substringAfter(".") + require(methodArg != null) { + annotationEntry.errorAtPosition("could not extract method from ${verb.name} @HttpIngress annotation") } - val pathArg = requireNotNull(args.second.single().getArgumentExpression()?.text?.let { + + val pathArg = args.second.single().getArgumentExpression()?.text?.let { extractPathComponents(it.trim('\"')) - }) { - "Could not extract path from ${verb.name} @HttpIngress annotation" + } + require(pathArg != null) { + annotationEntry.errorAtPosition("could not extract path from ${verb.name} @HttpIngress annotation") } MetadataIngress( type = "http", - path = pathArg, - method = methodArg, - pos = sourcePos.toPosition(verb.containingFile.name), + path = pathArg ?: emptyList(), + method = methodArg ?: "", + pos = annotationEntry.getPosition(), ) } } @@ -444,13 +452,14 @@ class SchemaExtractor( val func = element as? KtNamedFunction if (func != null) { - val funcSourcePos = func.getLineAndColumn() - val body = - requireNotNull(func.bodyExpression) { "$funcSourcePos Function body cannot be empty; was in ${func.name}" } + val body = func.bodyExpression + require(body != null) { + func.errorAtPosition("function body cannot be empty") + } - val commentRanges = body.node.children() - .filterIsInstance() - .map { it.textRange.shiftLeft(it.startOffset).shiftRight(it.startOffsetInParent) } + val commentRanges = body?.node?.children() + ?.filterIsInstance() + ?.map { it.textRange.shiftLeft(it.startOffset).shiftRight(it.startOffsetInParent) } val imports = func.containingKtFile.importList?.imports ?: emptyList() @@ -461,12 +470,15 @@ class SchemaExtractor( }.map { ctxParam -> getCallMatcher(ctxParam.text.split(":")[0].trim()) } val refs = callMatchers.flatMap { matcher -> - matcher.findAll(body.text).mapNotNull { match -> + matcher.findAll(body?.text ?: "").mapNotNull { match -> // ignore commented out matches - if (commentRanges.any { it.contains(TextRange(match.range.first, match.range.last)) }) return@mapNotNull null + if (commentRanges?.any { it.contains(TextRange(match.range.first, match.range.last)) } ?: false) { + return@mapNotNull null + } - val verbCall = requireNotNull(match.groups["fn"]?.value?.substringAfter("::")?.trim()) { - "Error processing function defined at $funcSourcePos: Could not extract outgoing verb call" + val verbCall = match.groups["fn"]?.value?.substringAfter("::")?.trim() + require(verbCall != null) { + func.errorAtPosition("could not extract outgoing verb call") } imports.firstOrNull { import -> // if aliased import, match the alias @@ -489,12 +501,12 @@ class SchemaExtractor( it )?.fqName?.asString() == xyz.block.ftl.Export::class.qualifiedName } - } ?: throw IllegalArgumentException( - "Error processing function defined at $funcSourcePos: Could not resolve outgoing verb call" + } ?: errors.add( + func.errorAtPosition("could not resolve outgoing verb call") ) Ref( - name = verbCall, + name = verbCall ?: "", module = element.extractModuleName(), ) } @@ -524,7 +536,7 @@ class SchemaExtractor( Field( name = param.name!!, type = param.typeReference?.let { - return@let it.resolveType().toSchemaType(it.getPosition()) + return@let it.resolveType()?.toSchemaType(it.getPosition(), it.textLength) }, metadata = metadata, ) @@ -543,7 +555,7 @@ class SchemaExtractor( private fun KtClass.toSchemaEnum(): Enum { val variants: List require(this.getValueParameters().isEmpty() || this.getValueParameters().size == 1) { - "${this.getLineAndColumn()}: Enums can have at most one value parameter, of type string or number" + this.errorAtPosition("enums can have at most one value parameter, of type string or number") } if (this.getValueParameters().isEmpty()) { @@ -559,13 +571,12 @@ class SchemaExtractor( } } else { variants = this.declarations.filterIsInstance().map { entry -> - val pos: Position = entry.getPosition() val name: String = entry.name!! val arg: ValueArgument = entry.initializerList?.initializers?.single().let { (it as KtSuperTypeCallEntry).valueArguments.single() } - val value: Value + var value: Value? = null try { value = arg.getArgumentExpression()?.text?.let { if (it.startsWith('"')) { @@ -573,15 +584,18 @@ class SchemaExtractor( } else { return@let Value(intValue = IntValue(value_ = it.toLong())) } - } ?: throw IllegalArgumentException("${pos}: Could not extract enum variant value") + } + if (value == null) { + entry.errorAtPosition("could not extract enum variant value") + } } catch (e: NumberFormatException) { - throw IllegalArgumentException("${pos}: Enum variant value must be a string or number") + errors.add(entry.errorAtPosition("enum variant value must be a string or number")) } EnumVariant( name = name, value_ = value, - pos = pos, + pos = entry.getPosition(), comments = entry.comments(), ) } @@ -595,7 +609,7 @@ class SchemaExtractor( ) } - private fun KotlinType.toSchemaType(position: Position): Type { + private fun KotlinType.toSchemaType(position: Position, tokenLength: Int): Type { if (this.unwrap().constructor.isTypeParameterTypeConstructor()) { return Type( ref = Ref( @@ -613,7 +627,7 @@ class SchemaExtractor( type.isSubtypeOf(builtIns.floatType) -> Type(float = xyz.block.ftl.v1.schema.Float()) type.isSubtypeOf(builtIns.doubleType) -> Type(float = xyz.block.ftl.v1.schema.Float()) type.isSubtypeOf(builtIns.booleanType) -> Type(bool = xyz.block.ftl.v1.schema.Bool()) - type.isSubtypeOf(builtIns.unitType) -> Type(unit = xyz.block.ftl.v1.schema.Unit()) + type.isSubtypeOf(builtIns.unitType) -> Type(unit = Unit()) type.anySuperTypeConstructor { it.getClassFqNameUnsafe().asString() == ByteArray::class.qualifiedName } -> Type(bytes = xyz.block.ftl.v1.schema.Bytes()) @@ -622,7 +636,7 @@ class SchemaExtractor( it.getClassFqNameUnsafe().asString() == builtIns.list.fqNameSafe.asString() } -> Type( array = Array( - element = this.arguments.first().type.toSchemaType(position) + element = this.arguments.first().type.toSchemaType(position, tokenLength) ) ) @@ -630,8 +644,8 @@ class SchemaExtractor( it.getClassFqNameUnsafe().asString() == builtIns.map.fqNameSafe.asString() } -> Type( map = xyz.block.ftl.v1.schema.Map( - key = this.arguments.first().type.toSchemaType(position), - value_ = this.arguments.last().type.toSchemaType(position), + key = this.arguments.first().type.toSchemaType(position, tokenLength), + value_ = this.arguments.last().type.toSchemaType(position, tokenLength), ) ) @@ -641,21 +655,25 @@ class SchemaExtractor( else -> { val descriptor = this.toClassDescriptor() - require( - descriptor.isData - || descriptor.kind == ClassKind.ENUM_CLASS - || this.isEmptyBuiltin() - ) { - "(${position.line},${position.column}) Expected type to be a data class or builtin.Empty, but was ${ - this.fqNameOrNull()?.asString() - }" + if (!(descriptor.isData || descriptor.kind == ClassKind.ENUM_CLASS || this.isEmptyBuiltin())) { + errors.add( + Error( + msg = "expected type to be a data class or builtin.Empty, but was ${this.fqNameOrNull()?.asString()}", + pos = position, + endColumn = position.column + tokenLength + ) + ) + return Type() } val refName = descriptor.name.asString() val fqName = this.fqNameOrNull()!!.asString() require(fqName.startsWith("ftl.")) { - "(${position.line},${position.column}) Expected module name to be in the form ftl., " + - "but was ${this.fqNameOrNull()?.asString()}" + Error( + msg = "expected module name to be in the form ftl., but was $fqName", + pos = position, + endColumn = position.column + tokenLength + ) } // add all referenced types to the schema @@ -669,10 +687,10 @@ class SchemaExtractor( Type( ref = Ref( - name = refName, + name = refName ?: "", module = fqName.extractModuleName(), pos = position, - typeParameters = this.arguments.map { it.type.toSchemaType(position) }.toList(), + typeParameters = this.arguments.map { it.type.toSchemaType(position, tokenLength) }.toList(), ) ) } @@ -686,14 +704,37 @@ class SchemaExtractor( return schemaType } - private fun KtTypeReference.resolveType(): KotlinType = - bindingContext.get(BindingContext.TYPE, this) - ?: throw IllegalStateException("${this.getLineAndColumn()} Could not resolve type ${this.text}") + private fun KtTypeReference.resolveType(): KotlinType? { + val type = bindingContext.get(BindingContext.TYPE, this) + require(type != null) { this.errorAtPosition("could not resolve type ${this.text}") } + return type + } + + private fun KotlinType.toClassDescriptor(): ClassDescriptor = + requireNotNull(this.unwrap().constructor.declarationDescriptor as? ClassDescriptor) + + private fun require(condition: Boolean, error: () -> Error) { + try { + require(condition) + } catch (e: IllegalArgumentException) { + errors.add(error()) + } + } companion object { private fun KtElement.getPosition() = this.getLineAndColumn().toPosition(this.containingFile.name) private fun PsiElement.getPosition() = this.getLineAndColumn().toPosition(this.containingFile.name) + + private fun PsiElement.errorAtPosition(message: String): Error { + val pos = this.getPosition() + return Error( + msg = message, + pos = pos, + endColumn = pos.column + this.textLength + ) + } + private fun PsiElement.getLineAndColumn(): LineAndColumn = getLineAndColumnInPsiFile(this.containingFile, this.textRange) @@ -708,10 +749,6 @@ class SchemaExtractor( return """${ctxVarName}\.call\((?[^,]+),\s*(?[^,]+?)\s*[()]""".toRegex(RegexOption.IGNORE_CASE) } - private fun KotlinType.toClassDescriptor(): ClassDescriptor = - this.unwrap().constructor.declarationDescriptor as? ClassDescriptor - ?: throw IllegalStateException("Could not resolve KotlinType to class") - fun KtElement.extractModuleName(): String { return this.containingKtFile.packageFqName.extractModuleName() } diff --git a/kotlin-runtime/ftl-runtime/src/test/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRuleTest.kt b/kotlin-runtime/ftl-runtime/src/test/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRuleTest.kt index fe185e554e..2a6f86eb9d 100644 --- a/kotlin-runtime/ftl-runtime/src/test/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRuleTest.kt +++ b/kotlin-runtime/ftl-runtime/src/test/kotlin/xyz/block/ftl/schemaextractor/ExtractSchemaRuleTest.kt @@ -9,12 +9,15 @@ import org.jetbrains.kotlin.cli.jvm.config.addJvmClasspathRoots import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test import org.junit.jupiter.api.assertThrows -import xyz.block.ftl.schemaextractor.ExtractSchemaRule.Companion.OUTPUT_FILENAME +import xyz.block.ftl.schemaextractor.ExtractSchemaRule.Companion.ERRORS_OUT +import xyz.block.ftl.schemaextractor.ExtractSchemaRule.Companion.SCHEMA_OUT import xyz.block.ftl.v1.schema.Array import xyz.block.ftl.v1.schema.Data import xyz.block.ftl.v1.schema.Decl import xyz.block.ftl.v1.schema.Enum import xyz.block.ftl.v1.schema.EnumVariant +import xyz.block.ftl.v1.schema.Error +import xyz.block.ftl.v1.schema.ErrorList import xyz.block.ftl.v1.schema.Field import xyz.block.ftl.v1.schema.IngressPathComponent import xyz.block.ftl.v1.schema.IngressPathLiteral @@ -48,6 +51,12 @@ internal class ExtractSchemaRuleTest(private val env: KotlinCoreEnvironment) { env.configuration.addJvmClasspathRoots(dependencies) } + @AfterTest + fun cleanup() { + File(SCHEMA_OUT).delete() + File(ERRORS_OUT).delete() + } + @Test fun `extracts schema`() { val code = """ @@ -124,7 +133,7 @@ internal class ExtractSchemaRuleTest(private val env: KotlinCoreEnvironment) { fun emptyVerb(context: Context) {} """ ExtractSchemaRule(Config.empty).compileAndLintWithContext(env, code) - val file = File(OUTPUT_FILENAME) + val file = File(SCHEMA_OUT) val module = Module.ADAPTER.decode(file.inputStream()) val expected = Module( @@ -395,10 +404,16 @@ fun callTime(context: Context): TimeResponse { return context.call(::time, Empty()) } """ - val message = assertThrows { + val message = assertThrows { ExtractSchemaRule(Config.empty).compileAndLintWithContext(env, code) }.message!! - assertContains(message, "Expected type to be a data class or builtin.Empty, but was kotlin.Char") + assertContains(message, "could not extract schema") + + assertErrorsFileContainsExactly( + Error( + msg = "expected type to be a data class or builtin.Empty, but was kotlin.Char", + ) + ) } @Test @@ -430,10 +445,19 @@ fun echo(context: Context, req: EchoRequest): EchoResponse { return EchoResponse(messages = listOf(EchoMessage(message = "Hello!"))) } """ - val message = assertThrows { + val message = assertThrows { ExtractSchemaRule(Config.empty).compileAndLintWithContext(env, code) }.message!! - assertContains(message, "@HttpIngress-annotated echo request must be ftl.builtin.HttpRequest") + assertContains(message, "could not extract schema") + + assertErrorsFileContainsExactly( + Error( + msg = "@HttpIngress-annotated echo request must be ftl.builtin.HttpRequest", + ), + Error( + msg = "@HttpIngress-annotated echo response must be ftl.builtin.HttpResponse", + ) + ) } @Test @@ -465,16 +489,19 @@ fun echo(context: Context, req: EchoRequest): EchoResponse { return EchoResponse(messages = listOf(EchoMessage(message = "Hello!"))) } """ - val message = assertThrows { + val message = assertThrows { ExtractSchemaRule(Config.empty).compileAndLintWithContext(env, code) }.message!! - assertContains(message, "@HttpIngress-annotated echo request must be ftl.builtin.HttpRequest") - } - - @AfterTest - fun cleanup() { - val file = File(OUTPUT_FILENAME) - file.delete() + assertContains(message, "could not extract schema") + + assertErrorsFileContainsExactly( + Error( + msg = "@HttpIngress-annotated echo request must be ftl.builtin.HttpRequest", + ), + Error( + msg = "@HttpIngress-annotated echo response must be ftl.builtin.HttpResponse", + ) + ) } @Test @@ -541,7 +568,7 @@ fun echo(context: Context, req: EchoRequest): EchoResponse { } """ ExtractSchemaRule(Config.empty).compileAndLintWithContext(env, code) - val file = File(OUTPUT_FILENAME) + val file = File(SCHEMA_OUT) val module = Module.ADAPTER.decode(file.inputStream()) val expected = Module( @@ -669,7 +696,7 @@ fun echo(context: Context, req: EchoRequest): EchoResponse { } """ ExtractSchemaRule(Config.empty).compileAndLintWithContext(env, code) - val file = File(OUTPUT_FILENAME) + val file = File(SCHEMA_OUT) val module = Module.ADAPTER.decode(file.inputStream()) val expected = Module( @@ -749,4 +776,17 @@ fun echo(context: Context, req: EchoRequest): EchoResponse { .ignoringFieldsMatchingRegexes(".*hashCode\$") .isEqualTo(expected) } + + private fun assertErrorsFileContainsExactly(vararg expected: Error) { + val file = File(ERRORS_OUT) + val actual = ErrorList.ADAPTER.decode(file.inputStream()) + + assertThat(actual.errors) + .usingRecursiveComparison() + .withEqualsForType({ _, _ -> true }, Position::class.java) + .ignoringFieldsMatchingRegexes(".*hashCode\$") + .ignoringFields("endColumn") + .ignoringCollectionOrder() + .isEqualTo(expected.toList()) + } } diff --git a/lsp/lsp.go b/lsp/lsp.go index a8de096991..024685f276 100644 --- a/lsp/lsp.go +++ b/lsp/lsp.go @@ -79,7 +79,6 @@ func (s *Server) post(err error) { // Deduplicate and associate by filename. for _, subErr := range ftlErrors.UnwrapAll(err) { - //TODO: Need a way to pass structured errors from other runtimes like kotlin. This won't work for them. var ce schema.Error if errors.As(subErr, &ce) { filename := ce.Pos.Filename