Fix isAstType checking logic for cycles (#1563)

Co-authored-by: Mark Sujew <[email protected]>

Author: montymxb

packages/langium/src/grammar/type-system/types-util.ts

@@ -231,27 +231,34 @@ function findAstTypesInternal(type: PropertyType, visited: Set<PropertyType>): s
 }
 
 export function isAstType(type: PropertyType): boolean {
-    return isAstTypeInternal(type, new Set());
+    return isAstTypeInternal(type, new Map());
 }
 
-export function isAstTypeInternal(type: PropertyType, visited: Set<PropertyType>): boolean {
+export function isAstTypeInternal(type: PropertyType, visited: Map<PropertyType, boolean>): boolean {
     if (visited.has(type)) {
-        return false;
-    } else {
-        visited.add(type);
+        return visited.get(type)!;
     }
+    // This is supposed to prevent infinite recursion.
+    // Setting this to true is a pretty safe assumption.
+    // Setting it to false might lead to false negatives for property unions.
+    visited.set(type, true);
+
+    let result = false;
+
     if (isPropertyUnion(type)) {
-        return type.types.every(e => isAstTypeInternal(e, visited));
+        result = type.types.every(e => isAstTypeInternal(e, visited));
     } else if (isValueType(type)) {
         const value = type.value;
         if ('type' in value) {
-            return isAstTypeInternal(value.type, visited);
+            result = isAstTypeInternal(value.type, visited);
         } else {
             // Is definitely an interface type
-            return true;
+            result = true;
         }
     }
-    return false;
+
+    visited.set(type, result);
+    return result;
 }
 
 export function escapeQuotes(str: string, type: '"' | "'" = '"'): string {

packages/langium/test/grammar/type-system/types-util.test.ts

@@ -5,7 +5,8 @@
  ******************************************************************************/
 
 import { describe, expect, test } from 'vitest';
-import { InterfaceType, isAstType } from 'langium/grammar';
+import type { ValueType } from 'langium/grammar';
+import { InterfaceType, UnionType, isAstType } from 'langium/grammar';
 
 describe('isAstType', () => {
 
@@ -74,4 +75,73 @@ describe('isAstType', () => {
         })).toBeFalsy();
     });
 
+    test('Should return false for primitive union type with cycles', () => {
+        const unionType = new UnionType('Test');
+        const valueType: ValueType = {
+            value: unionType
+        };
+        unionType.type = {
+            types: [
+                {
+                    primitive: 'string'
+                },
+                valueType
+            ]
+        };
+        expect(isAstType(unionType.type)).toBeFalsy();
+    });
+
+    test('Should return true for interface union type with cycles', () => {
+        const unionType = new UnionType('A');
+        const valueType: ValueType = {
+            value: unionType
+        };
+        unionType.type = {
+            types: [
+                {
+                    value: new InterfaceType('B', true, false)
+                },
+                valueType
+            ]
+        };
+        expect(isAstType(unionType.type)).toBeTruthy();
+    });
+
+    test('Should return true for nested duplicate union types', () => {
+        const unionType1 = new UnionType('A');
+        const unionType2 = new UnionType('B');
+        const interfaceType1 = new InterfaceType('C', true, false);
+        const interfaceType2 = new InterfaceType('D', true, false);
+        unionType1.type = {
+            types: [
+                {
+                    value: interfaceType1
+                },
+                {
+                    value: unionType2
+                }
+            ]
+        };
+        unionType2.type = {
+            types: [
+                {
+                    value: interfaceType1 // duplicate
+                },
+                {
+                    value: interfaceType2
+                }
+            ]
+        };
+        expect(isAstType(unionType1.type)).toBeTruthy();
+    });
+
+    test('Should return true for cyclic AST type', () => {
+        const unionType = new UnionType('Test');
+        const type: ValueType = {
+            value: unionType
+        };
+        unionType.type = type;
+        expect(isAstType(type)).toBeTruthy();
+    });
+
 });