Skip to content

Commit

Permalink
Improve recursion depth checks (microsoft#46599)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@ahejlsberg @mprobst
ahejlsberg authored and mprobst committed Jan 10, 2022
1 parent 06617b2 commit fc3a158
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 54 deletions.
109 changes: 61 additions & 48 deletions src/compiler/checker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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)) {
Expand All @@ -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;
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
}
}
Expand Down
12 changes: 6 additions & 6 deletions tests/baselines/reference/invariantGenericErrorElaboration.errors.txt
View file
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

0 comments on commit fc3a158

Please sign in to comment.