Merge pull request #35 from samtkit/cycle-detection

feat(semantic): detect cycles in record fields
samtkit · May 30, 2023 · 4cbd9d4 · 4cbd9d4
2 parents c1373e5 + 4233e6a
commit 4cbd9d4
Show file tree

Hide file tree

Showing 2 changed files with 149 additions and 1 deletion.
diff --git a/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt b/semantic/src/main/kotlin/tools/samt/semantic/SemanticModelPostProcessor.kt
@@ -172,7 +172,68 @@ 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<Type>, encounteredOptional: Boolean = false) {
+            val typeReference = (field.type as? ResolvedTypeReference) ?: return
+            val type = typeReference.type
+            val record: RecordType
+            val newVisited: List<Type>
+            var isOptional = encounteredOptional || typeReference.isOptional
+            when (type) {
+                is RecordType -> {
+                    record = type
+                    newVisited = visited + record
+                }
+                is AliasType -> {
+                    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 declaration = rootField.declaration
+                val path = newVisited.joinToString(" ► ") {
+                    buildString {
+                        append(it.humanReadableName)
+                        if (it is AliasType) {
+                            append(" (typealias)")
+                        }
+                    }
+                }
+                if (isOptional) {
+                    declaration.reportWarning(controller) {
+                        message("Record fields should not be cyclical, because they might not be serializable")
+                        highlight("cycle: $path", declaration.location)
+                    }
+                } else {
+                    declaration.reportError(controller) {
+                        message("Required record fields must not be cyclical, because they cannot be serialized")
+                        highlight("illegal cycle: $path", declaration.location)
+                    }
+                }
+                return
+            }
+            if (record in visited) {
+                // we ran into a cycle from a different record
+                return
+            }
+            record.fields.forEach { impl(it, newVisited, isOptional) }
+        }
+
+        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
@@ -816,6 +816,93 @@ 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: Required record fields must not be cyclical, because they cannot be serialized" }
+            )
+        }
+
+        @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: Required record fields must not be cyclical, because they cannot be serialized" }
+            )
+        }
+
+        @Test
+        fun `can have List or Map of same type`() {
+            val source = """
+                package cycles
+
+                record A {
+                    children: List<A>
+                    childrenByName: Map<String, A>
+                }
+            """.trimIndent()
+            parseAndCheck(
+                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, because they might not be serializable" }
+            )
+        }
     }
 
     @Nested