From a7ca09b3e8c50ec66c8a88c43954a5198507e0c7 Mon Sep 17 00:00:00 2001 From: Marcel Joss Date: Mon, 29 May 2023 15:13:56 +0200 Subject: [PATCH 1/5] feat(semantic): detect cycles in record fields --- .../semantic/SemanticModelPostProcessor.kt | 28 ++++++++++++++++++- .../tools/samt/semantic/SemanticModelTest.kt | 28 +++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt b/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt index 0422a777..9ca1b325 100644 --- a/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt +++ b/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt @@ -172,7 +172,33 @@ internal class SemanticModelPostProcessor(private val controller: DiagnosticCont } private fun checkRecord(record: RecordType) { - record.fields.forEach { checkModelType(it.type) } + record.fields.forEach { + checkModelType(it.type) + checkCycle(record, it) + } + } + + private fun checkCycle(rootRecord: RecordType, rootField: RecordType.Field) { + fun impl(field: RecordType.Field, visited: List) { + val typeReference = field.type as? ResolvedTypeReference ?: return + val record = typeReference.type as? RecordType ?: return + val newVisited = visited + record + if (record == rootRecord) { + val location = rootField.declaration.location + controller.getOrCreateContext(location.source).error { + message("Record fields must not be cyclical") + highlight("illegal cycle: ${newVisited.joinToString(" ► ") { it.humanReadableName }}", location) + } + return + } + if (record in visited) { + // we ran into a cycle from a different record + return + } + record.fields.forEach { impl(it, newVisited) } + } + + impl(rootField, listOf(rootRecord)) } private fun checkService(service: ServiceType) { diff --git a/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt b/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt index dced9482..0d856349 100644 --- a/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt +++ b/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt @@ -816,6 +816,34 @@ class SemanticModelTest { service to emptyList(), ) } + + @Test + fun `cannot have cyclic records`() { + val source = """ + package cycles + + record Recursive { + recursive: Recursive + } + + record IndirectA { + b: IndirectB + } + + record IndirectB { + a: IndirectA + } + + record ReferencesAll { + r: Recursive + a: IndirectA + b: IndirectB + } + """.trimIndent() + parseAndCheck( + source to List(3) { "Error: Record fields must not be cyclical" } + ) + } } @Nested From 704fda5d946f40841ff20f8835572b4728f054d7 Mon Sep 17 00:00:00 2001 From: Marcel Joss Date: Mon, 29 May 2023 16:23:54 +0200 Subject: [PATCH 2/5] feat(semantic): detect cycles in record fields when using typealiases --- .../semantic/SemanticModelPostProcessor.kt | 34 ++++++++++++++++--- .../tools/samt/semantic/SemanticModelTest.kt | 20 +++++++++++ 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt b/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt index 9ca1b325..9597214d 100644 --- a/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt +++ b/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt @@ -179,15 +179,39 @@ internal class SemanticModelPostProcessor(private val controller: DiagnosticCont } private fun checkCycle(rootRecord: RecordType, rootField: RecordType.Field) { - fun impl(field: RecordType.Field, visited: List) { - val typeReference = field.type as? ResolvedTypeReference ?: return - val record = typeReference.type as? RecordType ?: return - val newVisited = visited + record + fun impl(field: RecordType.Field, visited: List) { + val type = (field.type as? ResolvedTypeReference)?.type ?: return + val record: RecordType + val newVisited: List + when (type) { + is RecordType -> { + record = type + newVisited = visited + record + } + is AliasType -> { + val actualType = type.fullyResolvedType?.type + if (actualType !is RecordType) { + return + } + record = actualType + newVisited = visited + listOf(type, actualType) + } + else -> return + } + if (record == rootRecord) { val location = rootField.declaration.location controller.getOrCreateContext(location.source).error { message("Record fields must not be cyclical") - highlight("illegal cycle: ${newVisited.joinToString(" ► ") { it.humanReadableName }}", location) + val path = newVisited.joinToString(" ► ") { + buildString { + append(it.humanReadableName) + if (it is AliasType) { + append(" (typealias)") + } + } + } + highlight("illegal cycle: $path", location) } return } diff --git a/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt b/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt index 0d856349..d8d259e6 100644 --- a/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt +++ b/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt @@ -844,6 +844,26 @@ class SemanticModelTest { source to List(3) { "Error: Record fields must not be cyclical" } ) } + + @Test + fun `cannot have cyclic records with typealiases`() { + val source = """ + package cycles + + record A { + b: B + } + + record B { + c: C + } + + typealias C = A + """.trimIndent() + parseAndCheck( + source to List(2) { "Error: Record fields must not be cyclical" } + ) + } } @Nested From c64992d7eead157754fb6ec6b1724632960d38ca Mon Sep 17 00:00:00 2001 From: Marcel Joss Date: Mon, 29 May 2023 17:36:07 +0200 Subject: [PATCH 3/5] test(semantic): add test for List and Map members of the same type --- .../tools/samt/semantic/SemanticModelTest.kt | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt b/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt index d8d259e6..8437dd40 100644 --- a/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt +++ b/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt @@ -864,6 +864,21 @@ class SemanticModelTest { source to List(2) { "Error: Record fields must not be cyclical" } ) } + + @Test + fun `can have List or Map of same type`() { + val source = """ + package cycles + + record A { + children: List + childrenByName: Map + } + """.trimIndent() + parseAndCheck( + source to emptyList() + ) + } } @Nested From a7a6849e8e8211011f0e7939c16047da8fc42821 Mon Sep 17 00:00:00 2001 From: Marcel Joss Date: Mon, 29 May 2023 18:02:37 +0200 Subject: [PATCH 4/5] feat(semantic): allow cyclical records with a warning if there is an optional field --- .../semantic/SemanticModelPostProcessor.kt | 38 ++++++++++++------- .../tools/samt/semantic/SemanticModelTest.kt | 28 +++++++++++++- 2 files changed, 51 insertions(+), 15 deletions(-) diff --git a/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt b/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt index 9597214d..c23b1cc8 100644 --- a/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt +++ b/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt @@ -179,39 +179,51 @@ internal class SemanticModelPostProcessor(private val controller: DiagnosticCont } private fun checkCycle(rootRecord: RecordType, rootField: RecordType.Field) { - fun impl(field: RecordType.Field, visited: List) { - val type = (field.type as? ResolvedTypeReference)?.type ?: return + fun impl(field: RecordType.Field, visited: List, encounteredOptional: Boolean = false) { + val typeReference = (field.type as? ResolvedTypeReference) ?: return + val type = typeReference.type val record: RecordType val newVisited: List + var isOptional = encounteredOptional || typeReference.isOptional when (type) { is RecordType -> { record = type newVisited = visited + record } is AliasType -> { - val actualType = type.fullyResolvedType?.type + val reference = type.fullyResolvedType ?: return + val actualType = reference.type if (actualType !is RecordType) { return } record = actualType newVisited = visited + listOf(type, actualType) + isOptional = isOptional || reference.isOptional } else -> return } if (record == rootRecord) { val location = rootField.declaration.location - controller.getOrCreateContext(location.source).error { - message("Record fields must not be cyclical") - val path = newVisited.joinToString(" ► ") { - buildString { - append(it.humanReadableName) - if (it is AliasType) { - append(" (typealias)") - } + val path = newVisited.joinToString(" ► ") { + buildString { + append(it.humanReadableName) + if (it is AliasType) { + append(" (typealias)") } } - highlight("illegal cycle: $path", location) + } + val context = controller.getOrCreateContext(location.source) + if (isOptional) { + context.warn { + message("Record fields should not be cyclical") + highlight("cycle: $path", location) + } + } else { + context.error { + message("Required record fields must not be cyclical") + highlight("illegal cycle: $path", location) + } } return } @@ -219,7 +231,7 @@ internal class SemanticModelPostProcessor(private val controller: DiagnosticCont // we ran into a cycle from a different record return } - record.fields.forEach { impl(it, newVisited) } + record.fields.forEach { impl(it, newVisited, isOptional) } } impl(rootField, listOf(rootRecord)) diff --git a/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt b/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt index 8437dd40..9ae6e9db 100644 --- a/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt +++ b/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt @@ -841,7 +841,7 @@ class SemanticModelTest { } """.trimIndent() parseAndCheck( - source to List(3) { "Error: Record fields must not be cyclical" } + source to List(3) { "Error: Required record fields must not be cyclical" } ) } @@ -861,7 +861,7 @@ class SemanticModelTest { typealias C = A """.trimIndent() parseAndCheck( - source to List(2) { "Error: Record fields must not be cyclical" } + source to List(2) { "Error: Required record fields must not be cyclical" } ) } @@ -879,6 +879,30 @@ class SemanticModelTest { source to emptyList() ) } + + @Test + fun `cycle with optional type is warning`() { + val source = """ + package cycles + + record A { + b: B? + } + + record B { + a: A + } + + record Recursive { + recursive: R + } + + typealias R = Recursive? + """.trimIndent() + parseAndCheck( + source to List(3) { "Warning: Record fields should not be cyclical" } + ) + } } @Nested From 4233e6a70b436a83ab7a349135e22fb8004fed41 Mon Sep 17 00:00:00 2001 From: Marcel Joss Date: Tue, 30 May 2023 13:15:48 +0200 Subject: [PATCH 5/5] feat(semantic): mention serializability in cycle error message --- .../samt/semantic/SemanticModelPostProcessor.kt | 15 +++++++-------- .../tools/samt/semantic/SemanticModelTest.kt | 6 +++--- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt b/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt index c23b1cc8..17d110eb 100644 --- a/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt +++ b/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt @@ -204,7 +204,7 @@ internal class SemanticModelPostProcessor(private val controller: DiagnosticCont } if (record == rootRecord) { - val location = rootField.declaration.location + val declaration = rootField.declaration val path = newVisited.joinToString(" ► ") { buildString { append(it.humanReadableName) @@ -213,16 +213,15 @@ internal class SemanticModelPostProcessor(private val controller: DiagnosticCont } } } - val context = controller.getOrCreateContext(location.source) if (isOptional) { - context.warn { - message("Record fields should not be cyclical") - highlight("cycle: $path", location) + declaration.reportWarning(controller) { + message("Record fields should not be cyclical, because they might not be serializable") + highlight("cycle: $path", declaration.location) } } else { - context.error { - message("Required record fields must not be cyclical") - highlight("illegal cycle: $path", location) + declaration.reportError(controller) { + message("Required record fields must not be cyclical, because they cannot be serialized") + highlight("illegal cycle: $path", declaration.location) } } return diff --git a/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt b/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt index 9ae6e9db..9a0b2156 100644 --- a/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt +++ b/semantic/src/test/kotlin/tools/samt/semantic/SemanticModelTest.kt @@ -841,7 +841,7 @@ class SemanticModelTest { } """.trimIndent() parseAndCheck( - source to List(3) { "Error: Required record fields must not be cyclical" } + source to List(3) { "Error: Required record fields must not be cyclical, because they cannot be serialized" } ) } @@ -861,7 +861,7 @@ class SemanticModelTest { typealias C = A """.trimIndent() parseAndCheck( - source to List(2) { "Error: Required record fields must not be cyclical" } + source to List(2) { "Error: Required record fields must not be cyclical, because they cannot be serialized" } ) } @@ -900,7 +900,7 @@ class SemanticModelTest { typealias R = Recursive? """.trimIndent() parseAndCheck( - source to List(3) { "Warning: Record fields should not be cyclical" } + source to List(3) { "Warning: Record fields should not be cyclical, because they might not be serializable" } ) } }