fix(jsii-diff): handle recursive types (#558)

Structural type checking would recurse endlessly if a struct had a
member with the same type as itself, or the same type as a struct that
contained itself.

Add a cache to prevent infinite recursion.

Also in this commit: `tryFindFqn` no longer explodes if the referenced
assembly couldn't be found, but simply returns `undefined` as it should.

Author: rix0rrr

packages/jsii-diff/lib/type-analysis.ts

@@ -13,6 +13,9 @@ export function isSuperType(a: reflect.TypeReference, b: reflect.TypeReference,
   if (a.void || b.void) { throw new Error('isSuperType() does not handle voids'); }
   if (a.isAny) { return { success: true }; }
 
+  // Assume true if we're currently already checking this.
+  if (CURRENTLY_CHECKING.has(a, b)) { return { success: true }; }
+
   if (a.primitive !== undefined) {
     if (a.primitive === b.primitive) { return { success: true }; }
     return failure(`${b} is not assignable to ${a}`);
@@ -52,32 +55,39 @@ export function isSuperType(a: reflect.TypeReference, b: reflect.TypeReference,
     );
   }
 
-  // For named types, we'll always do a nominal typing relationship.
-  // That is, if in the updated typesystem someone were to use the type name
-  // from the old assembly, do they have a typing relationship that's accepted
-  // by a nominal type system. (That check also rules out enums)
-  const nominalCheck = isNominalSuperType(a, b, updatedSystem);
-  if (nominalCheck.success === false) { return nominalCheck; }
-
-  // At this point, the types are legal in the updated assembly's type system.
-  // However, for structs we also structurally check the fields between the OLD
-  // and the NEW type system.
-  // We could do more complex analysis on typing of methods, but it doesn't seem
-  // worth it.
+  // We have two named types, recursion might happen so protect against it.
+  CURRENTLY_CHECKING.add(a, b);
   try {
-    const A = a.type!; // Note: lookup in old type system!
-    const B = b.type!;
-    if (A.isInterfaceType() && A.isDataType() && B.isInterfaceType() && B.datatype) {
-      return isStructuralSuperType(A, B, updatedSystem);
+
+    // For named types, we'll always do a nominal typing relationship.
+    // That is, if in the updated typesystem someone were to use the type name
+    // from the old assembly, do they have a typing relationship that's accepted
+    // by a nominal type system. (That check also rules out enums)
+    const nominalCheck = isNominalSuperType(a, b, updatedSystem);
+    if (nominalCheck.success === false) { return nominalCheck; }
+
+    // At this point, the types are legal in the updated assembly's type system.
+    // However, for structs we also structurally check the fields between the OLD
+    // and the NEW type system.
+    // We could do more complex analysis on typing of methods, but it doesn't seem
+    // worth it.
+    try {
+      const A = a.type!; // Note: lookup in old type system!
+      const B = b.type!;
+      if (A.isInterfaceType() && A.isDataType() && B.isInterfaceType() && B.datatype) {
+        return isStructuralSuperType(A, B, updatedSystem);
+      }
+    } catch (e) {
+      // We might get an exception if the type is supposed to come from a different
+      // assembly and the lookup fails.
+      return failure(e.message);
     }
-  } catch (e) {
-    // We might get an exception if the type is supposed to come from a different
-    // assembly and the lookup fails.
-    return { success: false, reasons: [e.message] };
-  }
 
-  // All seems good
-  return { success: true };
+    // All seems good
+    return { success: true };
+  } finally {
+    CURRENTLY_CHECKING.remove(a, b);
+  }
 }
 
 /**
@@ -147,7 +157,7 @@ function isStructuralSuperType(a: reflect.InterfaceType, b: reflect.InterfaceTyp
     }
 
     const ana = isSuperType(aProp.type, bProp.type, updatedSystem);
-    if (!ana.success) { return ana; }
+    if (!ana.success) { return failure(`property ${name}`, ...ana.reasons); }
   }
 
   return { success: true };
@@ -166,3 +176,49 @@ export function prependReason(analysis: Analysis, message: string): Analysis {
   if (analysis.success) { return analysis; }
   return failure(message, ...analysis.reasons);
 }
+
+/**
+ * Keep a set of type pairs.
+ *
+ * Needs more code than it should because JavaScript has an anemic runtime.
+ *
+ * (The ideal type would have been Set<[string, string]> but different
+ * instances of those pairs wouldn't count as "the same")
+ */
+class TypePairs {
+  private readonly pairs = new Map<string, Set<string>>();
+
+  public add(a: reflect.TypeReference, b: reflect.TypeReference): void {
+    if (!a.fqn || !b.fqn) { return; } // Only for user-defined types
+
+    let image = this.pairs.get(a.fqn);
+    if (!image) {
+      this.pairs.set(a.fqn, image = new Set<string>());
+    }
+    image.add(b.fqn);
+  }
+
+  public remove(a: reflect.TypeReference, b: reflect.TypeReference): void {
+    if (!a.fqn || !b.fqn) { return; } // Only for user-defined types
+
+    const image = this.pairs.get(a.fqn);
+    if (image) {
+      image.delete(b.fqn);
+    }
+  }
+
+  public has(a: reflect.TypeReference, b: reflect.TypeReference): boolean {
+    if (!a.fqn || !b.fqn) { return false; } // Only for user-defined types
+
+    const image = this.pairs.get(a.fqn);
+    return !!image && image.has(b.fqn);
+  }
+}
+
+/**
+ * To avoid recursion, we keep a set of relationships we're *currently*
+ * checking, so that if we're recursing we can just assume the subtyping
+ * relationship holds (and let the other struct members falsify that
+ * statement).
+ */
+const CURRENTLY_CHECKING = new TypePairs();

packages/jsii-diff/test/test.structs.ts

@@ -249,4 +249,76 @@ export = {
 
     test.done();
   },
+
+  // ----------------------------------------------------------------------
+
+  async 'can handle recursive type references'(test: Test) {
+    await expectNoError(test,
+      `
+      export interface LinkedList {
+        readonly name: string;
+        readonly next?: LinkedList;
+      }
+
+      export class UseIt {
+        public main(list: LinkedList) {
+          Array.isArray(list);
+        }
+      }
+    `, `
+      export interface LinkedList {
+        readonly name: string;
+        readonly next?: LinkedList;
+      }
+
+      export class UseIt {
+        public main(list: LinkedList) {
+          Array.isArray(list);
+        }
+      }
+    `);
+
+    test.done();
+  },
+
+  // ----------------------------------------------------------------------
+
+  async 'can handle mutually recursive type references'(test: Test) {
+    await expectNoError(test,
+      `
+      export interface A {
+        readonly name: string;
+        readonly next?: B;
+      }
+
+      export interface B {
+        readonly name: string;
+        readonly next?: A;
+      }
+
+      export class UseIt {
+        public main(list: A) {
+          Array.isArray(list);
+        }
+      }
+    `, `
+      export interface A {
+        readonly name: string;
+        readonly next?: B;
+      }
+
+      export interface B {
+        readonly name: string;
+        readonly next?: A;
+      }
+
+      export class UseIt {
+        public main(list: A) {
+          Array.isArray(list);
+        }
+      }
+    `);
+
+    test.done();
+  },
 };

packages/jsii-reflect/lib/type-system.ts

@@ -142,9 +142,14 @@ export class TypeSystem {
   }
 
   public findAssembly(name: string) {
-    if (!(name in this._assemblyLookup)) {
+    const ret = this.tryFindAssembly(name);
+    if (!ret) {
       throw new Error(`Assembly "${name}" not found`);
     }
+    return ret;
+  }
+
+  public tryFindAssembly(name: string): Assembly | undefined {
     return this._assemblyLookup[name];
   }
 
@@ -156,8 +161,8 @@ export class TypeSystem {
 
   public tryFindFqn(fqn: string): Type | undefined {
     const [ assembly ] = fqn.split('.');
-    const asm = this.findAssembly(assembly);
-    return asm.tryFindType(fqn);
+    const asm = this.tryFindAssembly(assembly);
+    return asm && asm.tryFindType(fqn);
   }
 
   public findClass(fqn: string): ClassType {