Improve recursion depth checks (#46599)

ahejlsberg · web-flow · commit 373accf28f2d · 2021-11-02T14:05:17.000-07:00
* Decrease recursion depth limit to 3 + smarter check for recursion

* Accept new baselines

* Always set last type id

* Keep indexed access recursion depth check

* Less expensive and corrected check for broadest equivalent keys
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -17804,7 +17804,7 @@ namespace ts {
                 if (source.flags & TypeFlags.Singleton) return true;
             }
             if (source.flags & TypeFlags.Object && target.flags & TypeFlags.Object) {
-                const related = relation.get(getRelationKey(source, target, IntersectionState.None, relation));
+                const related = relation.get(getRelationKey(source, target, IntersectionState.None, relation, /*ignoreConstraints*/ false));
                 if (related !== undefined) {
                     return !!(related & RelationComparisonResult.Succeeded);
                 }
@@ -18704,7 +18704,8 @@ namespace ts {
                 if (overflow) {
                     return Ternary.False;
                 }
-                const id = getRelationKey(source, target, intersectionState | (inPropertyCheck ? IntersectionState.InPropertyCheck : 0), relation);
+                const keyIntersectionState = intersectionState | (inPropertyCheck ? IntersectionState.InPropertyCheck : 0);
+                const id = getRelationKey(source, target, keyIntersectionState, relation, /*ingnoreConstraints*/ false);
                 const entry = relation.get(id);
                 if (entry !== undefined) {
                     if (reportErrors && entry & RelationComparisonResult.Failed && !(entry & RelationComparisonResult.Reported)) {
@@ -18731,16 +18732,13 @@ namespace ts {
                     targetStack = [];
                 }
                 else {
-                    // generate a key where all type parameter id positions are replaced with unconstrained type parameter ids
-                    // this isn't perfect - nested type references passed as type arguments will muck up the indexes and thus
-                    // prevent finding matches- but it should hit up the common cases
-                    const broadestEquivalentId = id.split(",").map(i => i.replace(/-\d+/g, (_match, offset: number) => {
-                        const index = length(id.slice(0, offset).match(/[-=]/g) || undefined);
-                        return `=${index}`;
-                    })).join(",");
+                    // A key that starts with "*" is an indication that we have type references that reference constrained
+                    // type parameters. For such keys we also check against the key we would have gotten if all type parameters
+                    // were unconstrained.
+                    const broadestEquivalentId = id.startsWith("*") ? getRelationKey(source, target, keyIntersectionState, relation, /*ignoreConstraints*/ true) : undefined;
                     for (let i = 0; i < maybeCount; i++) {
                         // If source and target are already being compared, consider them related with assumptions
-                        if (id === maybeKeys[i] || broadestEquivalentId === maybeKeys[i]) {
+                        if (id === maybeKeys[i] || broadestEquivalentId && broadestEquivalentId === maybeKeys[i]) {
                             return Ternary.Maybe;
                         }
                     }
@@ -20295,47 +20293,55 @@ namespace ts {
             return isNonDeferredTypeReference(type) && some(getTypeArguments(type), t => !!(t.flags & TypeFlags.TypeParameter) || isTypeReferenceWithGenericArguments(t));
         }
 
-        /**
-         * getTypeReferenceId(A<T, number, U>) returns "111=0-12=1"
-         *   where A.id=111 and number.id=12
-         */
-        function getTypeReferenceId(type: TypeReference, typeParameters: Type[], depth = 0) {
-            let result = "" + type.target.id;
-            for (const t of getTypeArguments(type)) {
-                if (isUnconstrainedTypeParameter(t)) {
-                    let index = typeParameters.indexOf(t);
-                    if (index < 0) {
-                        index = typeParameters.length;
-                        typeParameters.push(t);
+        function getGenericTypeReferenceRelationKey(source: TypeReference, target: TypeReference, postFix: string, ignoreConstraints: boolean) {
+            const typeParameters: Type[] = [];
+            let constraintMarker = "";
+            const sourceId = getTypeReferenceId(source, 0);
+            const targetId = getTypeReferenceId(target, 0);
+            return `${constraintMarker}${sourceId},${targetId}${postFix}`;
+            // getTypeReferenceId(A<T, number, U>) returns "111=0-12=1"
+            // where A.id=111 and number.id=12
+            function getTypeReferenceId(type: TypeReference, depth = 0) {
+                let result = "" + type.target.id;
+                for (const t of getTypeArguments(type)) {
+                    if (t.flags & TypeFlags.TypeParameter) {
+                        if (ignoreConstraints || isUnconstrainedTypeParameter(t)) {
+                            let index = typeParameters.indexOf(t);
+                            if (index < 0) {
+                                index = typeParameters.length;
+                                typeParameters.push(t);
+                            }
+                            result += "=" + index;
+                            continue;
+                        }
+                        // We mark type references that reference constrained type parameters such that we know to obtain
+                        // and look for a "broadest equivalent key" in the cache.
+                        constraintMarker = "*";
+                    }
+                    else if (depth < 4 && isTypeReferenceWithGenericArguments(t)) {
+                        result += "<" + getTypeReferenceId(t as TypeReference, depth + 1) + ">";
+                        continue;
                     }
-                    result += "=" + index;
-                }
-                else if (depth < 4 && isTypeReferenceWithGenericArguments(t)) {
-                    result += "<" + getTypeReferenceId(t as TypeReference, typeParameters, depth + 1) + ">";
-                }
-                else {
                     result += "-" + t.id;
                 }
+                return result;
             }
-            return result;
         }
 
         /**
          * To improve caching, the relation key for two generic types uses the target's id plus ids of the type parameters.
          * For other cases, the types ids are used.
          */
-        function getRelationKey(source: Type, target: Type, intersectionState: IntersectionState, relation: ESMap<string, RelationComparisonResult>) {
+        function getRelationKey(source: Type, target: Type, intersectionState: IntersectionState, relation: ESMap<string, RelationComparisonResult>, ignoreConstraints: boolean) {
             if (relation === identityRelation && source.id > target.id) {
                 const temp = source;
                 source = target;
                 target = temp;
             }
             const postFix = intersectionState ? ":" + intersectionState : "";
-            if (isTypeReferenceWithGenericArguments(source) && isTypeReferenceWithGenericArguments(target)) {
-                const typeParameters: Type[] = [];
-                return getTypeReferenceId(source as TypeReference, typeParameters) + "," + getTypeReferenceId(target as TypeReference, typeParameters) + postFix;
-            }
-            return source.id + "," + target.id + postFix;
+            return isTypeReferenceWithGenericArguments(source) && isTypeReferenceWithGenericArguments(target) ?
+                getGenericTypeReferenceRelationKey(source as TypeReference, target as TypeReference, postFix, ignoreConstraints) :
+                `${source.id},${target.id}${postFix}`;
         }
 
         // Invoke the callback for each underlying property symbol of the given symbol and return the first
@@ -20389,27 +20395,34 @@ namespace ts {
         }
 
         // Return true if the given type is deeply nested. We consider this to be the case when structural type comparisons
-        // for 5 or more occurrences or instantiations of the type have been recorded on the given stack. It is possible,
+        // for maxDepth or more occurrences or instantiations of the type have been recorded on the given stack. It is possible,
         // though highly unlikely, for this test to be true in a situation where a chain of instantiations is not infinitely
-        // expanding. Effectively, we will generate a false positive when two types are structurally equal to at least 5
+        // expanding. Effectively, we will generate a false positive when two types are structurally equal to at least maxDepth
         // levels, but unequal at some level beyond that.
-        // In addition, this will also detect when an indexed access has been chained off of 5 or more times (which is essentially
-        // the dual of the structural comparison), and likewise mark the type as deeply nested, potentially adding false positives
-        // for finite but deeply expanding indexed accesses (eg, for `Q[P1][P2][P3][P4][P5]`).
-        // It also detects when a recursive type reference has expanded 5 or more times, eg, if the true branch of
+        // In addition, this will also detect when an indexed access has been chained off of maxDepth more times (which is
+        // essentially the dual of the structural comparison), and likewise mark the type as deeply nested, potentially adding
+        // false positives for finite but deeply expanding indexed accesses (eg, for `Q[P1][P2][P3][P4][P5]`).
+        // It also detects when a recursive type reference has expanded maxDepth or more times, e.g. if the true branch of
         // `type A<T> = null extends T ? [A<NonNullable<T>>] : [T]`
-        // has expanded into `[A<NonNullable<NonNullable<NonNullable<NonNullable<NonNullable<T>>>>>>]`
-        // in such cases we need to terminate the expansion, and we do so here.
-        function isDeeplyNestedType(type: Type, stack: Type[], depth: number, maxDepth = 5): boolean {
+        // has expanded into `[A<NonNullable<NonNullable<NonNullable<NonNullable<NonNullable<T>>>>>>]`. In such cases we need
+        // to terminate the expansion, and we do so here.
+        function isDeeplyNestedType(type: Type, stack: Type[], depth: number, maxDepth = 3): boolean {
             if (depth >= maxDepth) {
                 const identity = getRecursionIdentity(type);
                 let count = 0;
+                let lastTypeId = 0;
                 for (let i = 0; i < depth; i++) {
-                    if (getRecursionIdentity(stack[i]) === identity) {
-                        count++;
-                        if (count >= maxDepth) {
-                            return true;
+                    const t = stack[i];
+                    if (getRecursionIdentity(t) === identity) {
+                        // We only count occurrences with a higher type id than the previous occurrence, since higher
+                        // type ids are an indicator of newer instantiations caused by recursion.
+                        if (t.id >= lastTypeId) {
+                            count++;
+                            if (count >= maxDepth) {
+                                return true;
+                            }
                         }
+                        lastTypeId = t.id;
                     }
                 }
             }
diff --git a/tests/baselines/reference/invariantGenericErrorElaboration.errors.txt b/tests/baselines/reference/invariantGenericErrorElaboration.errors.txt
@@ -1,7 +1,7 @@
 tests/cases/compiler/invariantGenericErrorElaboration.ts(3,7): error TS2322: Type 'Num' is not assignable to type 'Runtype<any>'.
-  The types of 'constraint.constraint.constraint' are incompatible between these types.
-    Type 'Constraint<Constraint<Constraint<Num>>>' is not assignable to type 'Constraint<Constraint<Constraint<Runtype<any>>>>'.
-      Type 'Constraint<Runtype<any>>' is not assignable to type 'Constraint<Num>'.
+  The types of 'constraint.constraint' are incompatible between these types.
+    Type 'Constraint<Constraint<Num>>' is not assignable to type 'Constraint<Constraint<Runtype<any>>>'.
+      Type 'Runtype<any>' is not assignable to type 'Num'.
 tests/cases/compiler/invariantGenericErrorElaboration.ts(4,19): error TS2322: Type 'Num' is not assignable to type 'Runtype<any>'.
 
 
@@ -11,9 +11,9 @@ tests/cases/compiler/invariantGenericErrorElaboration.ts(4,19): error TS2322: Ty
     const wat: Runtype<any> = Num;
           ~~~
 !!! error TS2322: Type 'Num' is not assignable to type 'Runtype<any>'.
-!!! error TS2322:   The types of 'constraint.constraint.constraint' are incompatible between these types.
-!!! error TS2322:     Type 'Constraint<Constraint<Constraint<Num>>>' is not assignable to type 'Constraint<Constraint<Constraint<Runtype<any>>>>'.
-!!! error TS2322:       Type 'Constraint<Runtype<any>>' is not assignable to type 'Constraint<Num>'.
+!!! error TS2322:   The types of 'constraint.constraint' are incompatible between these types.
+!!! error TS2322:     Type 'Constraint<Constraint<Num>>' is not assignable to type 'Constraint<Constraint<Runtype<any>>>'.
+!!! error TS2322:       Type 'Runtype<any>' is not assignable to type 'Num'.
     const Foo = Obj({ foo: Num })
                       ~~~
 !!! error TS2322: Type 'Num' is not assignable to type 'Runtype<any>'.
