Skip to content

Improve recursion depth checks #46599

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 2, 2021
Merged

Improve recursion depth checks #46599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ddc106b
Decrease recursion depth limit to 3 + smarter check for recursion
ahejlsberg Oct 29, 2021
86185ad
Accept new baselines
ahejlsberg Oct 29, 2021
52e10d3
Always set last type id
ahejlsberg Oct 30, 2021
5f37d89
Keep indexed access recursion depth check
ahejlsberg Oct 31, 2021
9df07a8
Less expensive and corrected check for broadest equivalent keys
ahejlsberg Oct 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 61 additions & 48 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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)) {
Expand All @@ -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;
}
}
Expand Down Expand Up @@ -20261,47 +20259,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) ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been wondering this for awhile and worked on a change for a bit, but can/should we expand this detailed id generation to generic type-alias'd types as well (and not just type references), so type alias relations can also benefit from the broadest-equivalent-id check?

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
Expand Down Expand Up @@ -20355,27 +20361,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;
}
}
}
Expand Down
12 changes: 6 additions & 6 deletions tests/baselines/reference/invariantGenericErrorElaboration.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>'.


Expand All @@ -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>'.
Expand Down