Skip to content
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

Jump to bottom

Instantiate earlier inferred constraints in conditional types #57362

Merged
merged 1 commit into from Feb 16, 2024
Merged

Instantiate earlier inferred constraints in conditional types #57362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 9 additions & 25 deletions src/compiler/checker.ts
View file
Open in desktop
Expand Up @@ -18636,11 +18636,6 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return type;
}

function maybeCloneTypeParameter(p: TypeParameter) {
const constraint = getConstraintOfTypeParameter(p);
return constraint && (isGenericObjectType(constraint) || isGenericIndexType(constraint)) ? cloneTypeParameter(p) : p;
}

function isSimpleTupleType(node: TypeNode): boolean {
return isTupleTypeNode(node) && length(node.elements) > 0 &&
!some(node.elements, e => isOptionalTypeNode(e) || isRestTypeNode(e) || isNamedTupleMember(e) && !!(e.questionToken || e.dotDotDotToken));
Expand Down Expand Up @@ -18683,44 +18678,33 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
let combinedMapper: TypeMapper | undefined;
if (root.inferTypeParameters) {
// When we're looking at making an inference for an infer type, when we get its constraint, it'll automagically be
// instantiated with the context, so it doesn't need the mapper for the inference contex - however the constraint
// instantiated with the context, so it doesn't need the mapper for the inference context - however the constraint
// may refer to another _root_, _uncloned_ `infer` type parameter [1], or to something mapped by `mapper` [2].
// [1] Eg, if we have `Foo<T, U extends T>` and `Foo<number, infer B>` - `B` is constrained to `T`, which, in turn, has been instantiated
// as `number`
// Conversely, if we have `Foo<infer A, infer B>`, `B` is still constrained to `T` and `T` is instantiated as `A`
// [2] Eg, if we have `Foo<T, U extends T>` and `Foo<Q, infer B>` where `Q` is mapped by `mapper` into `number` - `B` is constrained to `T`
// which is in turn instantiated as `Q`, which is in turn instantiated as `number`.
// So we need to:
// * Clone the type parameters so their constraints can be instantiated in the context of `mapper` (otherwise theyd only get inference context information)
// * Set the clones to both map the conditional's enclosing `mapper` and the original params
// * instantiate the extends type with the clones
// * combine `context.nonFixingMapper` with `mapper` so their constraints can be instantiated in the context of `mapper` (otherwise they'd only get inference context information)
// * incorporate all of the component mappers into the combined mapper for the true and false members
// This means we have three mappers that need applying:
// This means we have two mappers that need applying:
// * The original `mapper` used to create this conditional
// * The mapper that maps the old root type parameter to the clone (`freshMapper`)
// * The mapper that maps the clone to its inference result (`context.mapper`)
const freshParams = sameMap(root.inferTypeParameters, maybeCloneTypeParameter);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR - in its current form - removes the context of those fresh params.

They were introduced in this PR to address a type parameter leak. From what I understand, the leak was caused by non-instantiated constraints of infer type parameters, and those fresh params were introduced to "wire up" the mapper so it would partake in the instantiation of those constraints. Those instantiations are made using context.nonFixingMapper so I figured out that maybe this can be simplified by combining that with the mapper.

The problem with maybeCloneTypeParameter that was used to create those freshParams is that the check inside it is non-exhaustive:

isGenericObjectType(constraint) || isGenericIndexType(constraint)

When it comes to the added test case, this check doesn't cover Klass<V> - that's neither of the tested things. (when the inferred Klass<number> gets checked against that constraint it fails the assignability check and the inferred result gets replaced by the constraint which in turn is instantiated by the combinedMapper)

So the alternative fix would be to extend this check to cover more situations. That can't be done exhaustively and cheaply. The closest check to that is couldContainTypeVariables and that's not supposed to be used in situations like this, it's just an optimization.

So given the fact above, one extra alternative solution comes to mind... freshParams could always be created. I verified that it would fix the issue and that it wouldn't break any existing case.

const freshMapper = freshParams !== root.inferTypeParameters ? createTypeMapper(root.inferTypeParameters, freshParams) : undefined;
const context = createInferenceContext(freshParams, /*signature*/ undefined, InferenceFlags.None);
if (freshMapper) {
const freshCombinedMapper = combineTypeMappers(mapper, freshMapper);
for (let i = 0; i < freshParams.length; i++) {
if (freshParams[i] !== root.inferTypeParameters[i]) {
freshParams[i].mapper = freshCombinedMapper;
}
}
// * The mapper that maps the infer type parameter to its inference result (`context.mapper`)
const context = createInferenceContext(root.inferTypeParameters, /*signature*/ undefined, InferenceFlags.None);
if (mapper) {
context.nonFixingMapper = combineTypeMappers(context.nonFixingMapper, mapper);
}
if (!checkTypeDeferred) {
// We don't want inferences from constraints as they may cause us to eagerly resolve the
// conditional type instead of deferring resolution. Also, we always want strict function
// types rules (i.e. proper contravariance) for inferences.
inferTypes(context.inferences, checkType, instantiateType(extendsType, freshMapper), InferencePriority.NoConstraints | InferencePriority.AlwaysStrict);
inferTypes(context.inferences, checkType, extendsType, InferencePriority.NoConstraints | InferencePriority.AlwaysStrict);
}
const innerMapper = combineTypeMappers(freshMapper, context.mapper);
// It's possible for 'infer T' type paramteters to be given uninstantiated constraints when the
// those type parameters are used in type references (see getInferredTypeParameterConstraint). For
// that reason we need context.mapper to be first in the combined mapper. See #42636 for examples.
combinedMapper = mapper ? combineTypeMappers(innerMapper, mapper) : innerMapper;
combinedMapper = mapper ? combineTypeMappers(context.mapper, mapper) : context.mapper;
}
// Instantiate the extends type including inferences for 'infer T' type parameters
const inferredExtendsType = combinedMapper ? instantiateType(root.extendsType, combinedMapper) : extendsType;
Expand Down
54 changes: 54 additions & 0 deletions tests/baselines/reference/inferTypeParameterConstraints.js
View file
Open in desktop
Expand Up @@ -18,8 +18,62 @@ type Constrain<T extends C, C> = unknown;
type Foo<A> = A extends Constrain<infer X, A> ? X : never;

type T0 = Foo<string>; // string

// https://github.com/microsoft/TypeScript/issues/57286#issuecomment-1927920336

class BaseClass<V> {
protected fake(): V {
throw new Error("");
}
}

class Klass<V> extends BaseClass<V> {
child = true;
}

type Constructor<V, P extends BaseClass<V>> = new () => P;
type inferTest<V, T> = T extends Constructor<V, infer P> ? P : never;

type U = inferTest<number, Constructor<number, Klass<number>>>;

declare let m: U;
m.child; // ok


//// [inferTypeParameterConstraints.js]
"use strict";
// Repro from #42636
var __extends = (this && this.__extends) || (function () {
var extendStatics = function (d, b) {
extendStatics = Object.setPrototypeOf ||
({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
function (d, b) { for (var p in b) if (Object.prototype.hasOwnProperty.call(b, p)) d[p] = b[p]; };
return extendStatics(d, b);
};
return function (d, b) {
if (typeof b !== "function" && b !== null)
throw new TypeError("Class extends value " + String(b) + " is not a constructor or null");
extendStatics(d, b);
function __() { this.constructor = d; }
d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};
})();
// https://github.com/microsoft/TypeScript/issues/57286#issuecomment-1927920336
var BaseClass = /** @class */ (function () {
function BaseClass() {
}
BaseClass.prototype.fake = function () {
throw new Error("");
};
return BaseClass;
}());
var Klass = /** @class */ (function (_super) {
__extends(Klass, _super);
function Klass() {
var _this = _super !== null && _super.apply(this, arguments) || this;
_this.child = true;
return _this;
}
return Klass;
}(BaseClass));
m.child; // ok
58 changes: 58 additions & 0 deletions tests/baselines/reference/inferTypeParameterConstraints.symbols
View file
Open in desktop
Expand Up @@ -51,3 +51,61 @@ type T0 = Foo<string>; // string
>T0 : Symbol(T0, Decl(inferTypeParameterConstraints.ts, 14, 58))
>Foo : Symbol(Foo, Decl(inferTypeParameterConstraints.ts, 12, 41))

// https://github.com/microsoft/TypeScript/issues/57286#issuecomment-1927920336

class BaseClass<V> {
>BaseClass : Symbol(BaseClass, Decl(inferTypeParameterConstraints.ts, 16, 22))
>V : Symbol(V, Decl(inferTypeParameterConstraints.ts, 20, 16))

protected fake(): V {
>fake : Symbol(BaseClass.fake, Decl(inferTypeParameterConstraints.ts, 20, 20))
>V : Symbol(V, Decl(inferTypeParameterConstraints.ts, 20, 16))

throw new Error("");
>Error : Symbol(Error, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
}
}

class Klass<V> extends BaseClass<V> {
>Klass : Symbol(Klass, Decl(inferTypeParameterConstraints.ts, 24, 1))
>V : Symbol(V, Decl(inferTypeParameterConstraints.ts, 26, 12))
>BaseClass : Symbol(BaseClass, Decl(inferTypeParameterConstraints.ts, 16, 22))
>V : Symbol(V, Decl(inferTypeParameterConstraints.ts, 26, 12))

child = true;
>child : Symbol(Klass.child, Decl(inferTypeParameterConstraints.ts, 26, 37))
}

type Constructor<V, P extends BaseClass<V>> = new () => P;
>Constructor : Symbol(Constructor, Decl(inferTypeParameterConstraints.ts, 28, 1))
>V : Symbol(V, Decl(inferTypeParameterConstraints.ts, 30, 17))
>P : Symbol(P, Decl(inferTypeParameterConstraints.ts, 30, 19))
>BaseClass : Symbol(BaseClass, Decl(inferTypeParameterConstraints.ts, 16, 22))
>V : Symbol(V, Decl(inferTypeParameterConstraints.ts, 30, 17))
>P : Symbol(P, Decl(inferTypeParameterConstraints.ts, 30, 19))

type inferTest<V, T> = T extends Constructor<V, infer P> ? P : never;
>inferTest : Symbol(inferTest, Decl(inferTypeParameterConstraints.ts, 30, 58))
>V : Symbol(V, Decl(inferTypeParameterConstraints.ts, 31, 15))
>T : Symbol(T, Decl(inferTypeParameterConstraints.ts, 31, 17))
>T : Symbol(T, Decl(inferTypeParameterConstraints.ts, 31, 17))
>Constructor : Symbol(Constructor, Decl(inferTypeParameterConstraints.ts, 28, 1))
>V : Symbol(V, Decl(inferTypeParameterConstraints.ts, 31, 15))
>P : Symbol(P, Decl(inferTypeParameterConstraints.ts, 31, 53))
>P : Symbol(P, Decl(inferTypeParameterConstraints.ts, 31, 53))

type U = inferTest<number, Constructor<number, Klass<number>>>;
>U : Symbol(U, Decl(inferTypeParameterConstraints.ts, 31, 69))
>inferTest : Symbol(inferTest, Decl(inferTypeParameterConstraints.ts, 30, 58))
>Constructor : Symbol(Constructor, Decl(inferTypeParameterConstraints.ts, 28, 1))
>Klass : Symbol(Klass, Decl(inferTypeParameterConstraints.ts, 24, 1))

declare let m: U;
>m : Symbol(m, Decl(inferTypeParameterConstraints.ts, 35, 11))
>U : Symbol(U, Decl(inferTypeParameterConstraints.ts, 31, 69))

m.child; // ok
>m.child : Symbol(Klass.child, Decl(inferTypeParameterConstraints.ts, 26, 37))
>m : Symbol(m, Decl(inferTypeParameterConstraints.ts, 35, 11))
>child : Symbol(Klass.child, Decl(inferTypeParameterConstraints.ts, 26, 37))

41 changes: 41 additions & 0 deletions tests/baselines/reference/inferTypeParameterConstraints.types
View file
Open in desktop
Expand Up @@ -26,3 +26,44 @@ type Foo<A> = A extends Constrain<infer X, A> ? X : never;
type T0 = Foo<string>; // string
>T0 : string

// https://github.com/microsoft/TypeScript/issues/57286#issuecomment-1927920336

class BaseClass<V> {
>BaseClass : BaseClass<V>

protected fake(): V {
>fake : () => V

throw new Error("");
>new Error("") : Error
>Error : ErrorConstructor
>"" : ""
}
}

class Klass<V> extends BaseClass<V> {
>Klass : Klass<V>
>BaseClass : BaseClass<V>

child = true;
>child : boolean
>true : true
}

type Constructor<V, P extends BaseClass<V>> = new () => P;
>Constructor : Constructor<V, P>

type inferTest<V, T> = T extends Constructor<V, infer P> ? P : never;
>inferTest : inferTest<V, T>

type U = inferTest<number, Constructor<number, Klass<number>>>;
>U : Klass<number>

declare let m: U;
>m : Klass<number>

m.child; // ok
>m.child : boolean
>m : Klass<number>
>child : boolean

20 changes: 20 additions & 0 deletions tests/cases/compiler/inferTypeParameterConstraints.ts
View file
Open in desktop
Expand Up @@ -17,3 +17,23 @@ type Constrain<T extends C, C> = unknown;
type Foo<A> = A extends Constrain<infer X, A> ? X : never;

type T0 = Foo<string>; // string

// https://github.com/microsoft/TypeScript/issues/57286#issuecomment-1927920336

class BaseClass<V> {
protected fake(): V {
throw new Error("");
}
}

class Klass<V> extends BaseClass<V> {
child = true;
}

type Constructor<V, P extends BaseClass<V>> = new () => P;
type inferTest<V, T> = T extends Constructor<V, infer P> ? P : never;

type U = inferTest<number, Constructor<number, Klass<number>>>;

declare let m: U;
m.child; // ok