From ddc106b854cb9244b7a12c3a5fd1e3eaf565c62e Mon Sep 17 00:00:00 2001 From: Anders Hejlsberg Date: Fri, 29 Oct 2021 16:40:11 -0700 Subject: [PATCH 1/5] Decrease recursion depth limit to 3 + smarter check for recursion --- src/compiler/checker.ts | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 8d77440dc009e..dc40f7b15a042 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -20355,27 +20355,25 @@ 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 - // `type A = null extends T ? [A>] : [T]` - // has expanded into `[A>>>>>]` - // 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 { + 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) { + const t = stack[i]; + // We only count occurrences with higher type ids than the previous occurrences, since higher + // type ids are an indicator of newer instantiations caused by recursion. + if (getRecursionIdentity(t) === identity && t.id >= lastTypeId) { count++; if (count >= maxDepth) { return true; } + lastTypeId = t.id; } } } @@ -20410,13 +20408,6 @@ namespace ts { if (type.flags & TypeFlags.TypeParameter) { return type.symbol; } - if (type.flags & TypeFlags.IndexedAccess) { - // Identity is the leftmost object type in a chain of indexed accesses, eg, in A[P][Q] it is A - do { - type = (type as IndexedAccessType).objectType; - } while (type.flags & TypeFlags.IndexedAccess); - return type; - } if (type.flags & TypeFlags.Conditional) { // The root object represents the origin of the conditional type return (type as ConditionalType).root; From 86185ad96bb7b6af188c483d9b44269e49c39426 Mon Sep 17 00:00:00 2001 From: Anders Hejlsberg Date: Fri, 29 Oct 2021 16:40:29 -0700 Subject: [PATCH 2/5] Accept new baselines --- .../invariantGenericErrorElaboration.errors.txt | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/baselines/reference/invariantGenericErrorElaboration.errors.txt b/tests/baselines/reference/invariantGenericErrorElaboration.errors.txt index 7ab83c61bcdd5..1113d3e0eae79 100644 --- 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'. - The types of 'constraint.constraint.constraint' are incompatible between these types. - Type 'Constraint>>' is not assignable to type 'Constraint>>>'. - Type 'Constraint>' is not assignable to type 'Constraint'. + The types of 'constraint.constraint' are incompatible between these types. + Type 'Constraint>' is not assignable to type 'Constraint>>'. + Type 'Runtype' is not assignable to type 'Num'. tests/cases/compiler/invariantGenericErrorElaboration.ts(4,19): error TS2322: Type 'Num' is not assignable to type 'Runtype'. @@ -11,9 +11,9 @@ tests/cases/compiler/invariantGenericErrorElaboration.ts(4,19): error TS2322: Ty const wat: Runtype = Num; ~~~ !!! error TS2322: Type 'Num' is not assignable to type 'Runtype'. -!!! error TS2322: The types of 'constraint.constraint.constraint' are incompatible between these types. -!!! error TS2322: Type 'Constraint>>' is not assignable to type 'Constraint>>>'. -!!! error TS2322: Type 'Constraint>' is not assignable to type 'Constraint'. +!!! error TS2322: The types of 'constraint.constraint' are incompatible between these types. +!!! error TS2322: Type 'Constraint>' is not assignable to type 'Constraint>>'. +!!! error TS2322: Type 'Runtype' is not assignable to type 'Num'. const Foo = Obj({ foo: Num }) ~~~ !!! error TS2322: Type 'Num' is not assignable to type 'Runtype'. From 52e10d3a980f2ebb390f589dc8cc5c62613e73ca Mon Sep 17 00:00:00 2001 From: Anders Hejlsberg Date: Sat, 30 Oct 2021 08:24:28 -0700 Subject: [PATCH 3/5] Always set last type id --- src/compiler/checker.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index dc40f7b15a042..daade99214eac 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -20366,12 +20366,14 @@ namespace ts { let lastTypeId = 0; for (let i = 0; i < depth; i++) { const t = stack[i]; - // We only count occurrences with higher type ids than the previous occurrences, since higher - // type ids are an indicator of newer instantiations caused by recursion. - if (getRecursionIdentity(t) === identity && t.id >= lastTypeId) { - count++; - if (count >= maxDepth) { - return true; + 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; } From 5f37d89c881dcbe3234f47b34cdd124fa779a912 Mon Sep 17 00:00:00 2001 From: Anders Hejlsberg Date: Sat, 30 Oct 2021 18:04:39 -0700 Subject: [PATCH 4/5] Keep indexed access recursion depth check --- src/compiler/checker.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index daade99214eac..e6a4c2d8c7c0e 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -20359,6 +20359,13 @@ namespace ts { // 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 maxDepth // levels, but unequal at some level beyond that. + // 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 = null extends T ? [A>] : [T]` + // has expanded into `[A>>>>>]`. 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); @@ -20410,6 +20417,13 @@ namespace ts { if (type.flags & TypeFlags.TypeParameter) { return type.symbol; } + if (type.flags & TypeFlags.IndexedAccess) { + // Identity is the leftmost object type in a chain of indexed accesses, eg, in A[P][Q] it is A + do { + type = (type as IndexedAccessType).objectType; + } while (type.flags & TypeFlags.IndexedAccess); + return type; + } if (type.flags & TypeFlags.Conditional) { // The root object represents the origin of the conditional type return (type as ConditionalType).root; From 9df07a84820585e2b05107b95e4887b7ff084335 Mon Sep 17 00:00:00 2001 From: Anders Hejlsberg Date: Sun, 31 Oct 2021 15:13:26 -0700 Subject: [PATCH 5/5] Less expensive and corrected check for broadest equivalent keys --- src/compiler/checker.ts | 76 ++++++++++++++++++++++------------------- 1 file changed, 41 insertions(+), 35 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index e6a4c2d8c7c0e..fa801d3161daf 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -17770,7 +17770,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); } @@ -18670,7 +18670,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)) { @@ -18697,16 +18698,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; } } @@ -20261,47 +20259,55 @@ namespace ts { return isNonDeferredTypeReference(type) && some(getTypeArguments(type), t => !!(t.flags & TypeFlags.TypeParameter) || isTypeReferenceWithGenericArguments(t)); } - /** - * getTypeReferenceId(A) 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) 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) { + function getRelationKey(source: Type, target: Type, intersectionState: IntersectionState, relation: ESMap, 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