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.

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

Lazily compute signature type predicates #17600

Merged
22 commits merged into from
Dec 5, 2017
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
66 changes: 43 additions & 23 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,11 @@ namespace ts {
const resolvingSignature = createSignature(undefined, undefined, undefined, emptyArray, anyType, /*typePredicate*/ undefined, 0, /*hasRestParameter*/ false, /*hasLiteralTypes*/ false);
const silentNeverSignature = createSignature(undefined, undefined, undefined, emptyArray, silentNeverType, /*typePredicate*/ undefined, 0, /*hasRestParameter*/ false, /*hasLiteralTypes*/ false);

const unresolvedTypePredicate: void & { __unresolvedTypePredicate: void } = (() => {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this type so needlessly complex? Just use TypePredicate.

Copy link
Author

Choose a reason for hiding this comment

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

I want to ensure that this is definitely not useable as a TypePredicate -- it's just one to prevent polymorphism. Otherwise it's too easy to access resolvedTypePredicate and think you're getting the right thing.

Copy link
Member

Choose a reason for hiding this comment

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

We generally don't have that issue with other cases like noConstraintType, especially if all access to the type predicate is gated through getTypePredicateOfSignature.

While I don't think it's strictly necessary, you could add another TypePredicateKind as a discriminant, but I think using an object whose shape (and hidden class) matches other valid values for that property will reduce polymorphism.

Copy link
Author

@ghost ghost Aug 9, 2017

Choose a reason for hiding this comment

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

I would disagree that it's not an issue -- it might not be an issue if you wrote the code itself, but coming from the outside, this was a barrier to my understanding this code, and it would have been easier had I realized that resolvedReturnType was not meant to be used directly as a Type. I hadn't come across noConstraintType yet, but I'm sure it wouldn't have been easy to realize that a variable of type Type should not actually be used as a Type because it might be a special sentinel value.

Copy link
Member

Choose a reason for hiding this comment

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

This one of many cases where we need better documentation in checker.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer we stick to our current pattern for these cases. We can discuss these specific concerns with the broader team following the 2.5 release.

const tp: IdentifierTypePredicate = { kind: TypePredicateKind.Identifier, parameterName: "<<unresolved>>", parameterIndex: 0, type: anyType };
return tp as any;
})();

const enumNumberIndexInfo = createIndexInfo(stringType, /*isReadonly*/ true);
const jsObjectLiteralIndexInfo = createIndexInfo(anyType, /*isReadonly*/ false);

Expand Down Expand Up @@ -2826,8 +2831,8 @@ namespace ts {
parameters.unshift(thisParameter);
}
let returnTypeNode: TypeNode;
if (signature.typePredicate) {
const typePredicate = signature.typePredicate;
const typePredicate = getTypePredicateOfSignature(signature);
if (typePredicate) {
const parameterName = typePredicate.kind === TypePredicateKind.Identifier ?
setEmitFlags(createIdentifier((<IdentifierTypePredicate>typePredicate).parameterName), EmitFlags.NoAsciiEscaping) :
createThisTypeNode();
Expand Down Expand Up @@ -3773,8 +3778,9 @@ namespace ts {
}
writeSpace(writer);

if (signature.typePredicate) {
buildTypePredicateDisplay(signature.typePredicate, writer, enclosingDeclaration, flags, symbolStack);
const typePredicate = getTypePredicateOfSignature(signature);
if (typePredicate) {
buildTypePredicateDisplay(typePredicate, writer, enclosingDeclaration, flags, symbolStack);
}
else {
buildTypeDisplay(returnType, writer, enclosingDeclaration, flags, symbolStack);
Expand Down Expand Up @@ -5431,14 +5437,14 @@ namespace ts {
}

Copy link
Member

Choose a reason for hiding this comment

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

Please be aware that if we call createSignature once with a TypePredicate and then call createSignature again later with "pending" that it will result in a V8 compiling a second copy of createSignature due to the parameter being polymorphic (string vs. object).

We also end up making Signature more polymorphic. This could result in degraded performance so I would recommend you run benchmarks before and after this change to ensure this does not cause a regression.

As an alternative, I would suggest that you define something like a const unresolvedTypePredicate = <IdentifierTypePredicate>{ kind: TypePredicateKind.Identifier, parameterName: "pending", parameterIndex: 0 }; to act as your "pending" sentinel value as it will reduce polymorphism.

Copy link
Author

Choose a reason for hiding this comment

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

Could we just use null for pending?

Copy link
Member

Choose a reason for hiding this comment

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

We try not to use null in the compiler at all, and it would be inconsistent.

function createSignature(declaration: SignatureDeclaration, typeParameters: TypeParameter[], thisParameter: Symbol | undefined, parameters: Symbol[],
resolvedReturnType: Type, typePredicate: TypePredicate, minArgumentCount: number, hasRestParameter: boolean, hasLiteralTypes: boolean): Signature {
resolvedReturnType: Type | undefined, resolvedTypePredicate: TypePredicate | void & { __unresolvedTypePredicate: void } | undefined, minArgumentCount: number, hasRestParameter: boolean, hasLiteralTypes: boolean): Signature {
const sig = new Signature(checker);
sig.declaration = declaration;
sig.typeParameters = typeParameters;
sig.parameters = parameters;
sig.thisParameter = thisParameter;
sig.resolvedReturnType = resolvedReturnType;
sig.typePredicate = typePredicate;
sig.resolvedTypePredicate = resolvedTypePredicate;
sig.minArgumentCount = minArgumentCount;
sig.hasRestParameter = hasRestParameter;
sig.hasLiteralTypes = hasLiteralTypes;
Expand All @@ -5447,7 +5453,7 @@ namespace ts {

function cloneSignature(sig: Signature): Signature {
return createSignature(sig.declaration, sig.typeParameters, sig.thisParameter, sig.parameters, sig.resolvedReturnType,
sig.typePredicate, sig.minArgumentCount, sig.hasRestParameter, sig.hasLiteralTypes);
sig.resolvedTypePredicate, sig.minArgumentCount, sig.hasRestParameter, sig.hasLiteralTypes);
}

function getDefaultConstructSignatures(classType: InterfaceType): Signature[] {
Expand Down Expand Up @@ -6543,6 +6549,17 @@ namespace ts {
}
}

function signatureHasTypePredicate(signature: Signature): boolean {
return signature.resolvedTypePredicate !== undefined;
}

function getTypePredicateOfSignature(signature: Signature): TypePredicate | undefined {
Copy link
Member

Choose a reason for hiding this comment

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

Another approach we could consider is the one used to resolve Type Parameter constraints. Instead of using undefined to indicate no type predicate and an unresolvedTypePredicate sentinel to indicate deferred resolution, use undefined to indicate deferred resolution and a noTypePredicate sentinel. See noConstraintType as an example.

Copy link
Author

Choose a reason for hiding this comment

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

Shouldn't undefined be used for the most common case though? Usually no type predicate exists.

Copy link
Member

Choose a reason for hiding this comment

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

It can also mean that we don't know the answer yet. This is consistent with other "resolve"-named properties used in checker.

Copy link
Author

Choose a reason for hiding this comment

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

I've inverted the representation, so undefined now represents a lazily-computed type predicate and noTypePredicate represents no type predicate existing.

if (signature.resolvedTypePredicate === unresolvedTypePredicate) {
signature.resolvedTypePredicate = instantiateTypePredicate(getTypePredicateOfSignature(signature.target), signature.mapper);
}
return signature.resolvedTypePredicate as TypePredicate | undefined;
}
Copy link
Member

Choose a reason for hiding this comment

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

Write this function in the same style as getConstraintFromTypeParameter.


function getReturnTypeOfSignature(signature: Signature): Type {
if (!signature.resolvedReturnType) {
if (!pushTypeResolution(signature, TypeSystemPropertyName.ResolvedReturnType)) {
Expand Down Expand Up @@ -8102,7 +8119,7 @@ namespace ts {
return result;
}

function cloneTypePredicate(predicate: TypePredicate, mapper: TypeMapper): ThisTypePredicate | IdentifierTypePredicate {
function instantiateTypePredicate(predicate: TypePredicate, mapper: TypeMapper): ThisTypePredicate | IdentifierTypePredicate {
if (isIdentifierTypePredicate(predicate)) {
return {
kind: TypePredicateKind.Identifier,
Expand All @@ -8121,7 +8138,6 @@ namespace ts {

function instantiateSignature(signature: Signature, mapper: TypeMapper, eraseTypeParameters?: boolean): Signature {
let freshTypeParameters: TypeParameter[];
let freshTypePredicate: TypePredicate;
if (signature.typeParameters && !eraseTypeParameters) {
// First create a fresh set of type parameters, then include a mapping from the old to the
// new type parameters in the mapper function. Finally store this mapper in the new type
Expand All @@ -8132,14 +8148,14 @@ namespace ts {
tp.mapper = mapper;
}
}
if (signature.typePredicate) {
freshTypePredicate = cloneTypePredicate(signature.typePredicate, mapper);
}
// Don't compute resolvedReturnType and resolvedTypePredicate now,
// because using `mapper` now could trigger inferences to become fixed. (See `createInferenceContext`.)
// See GH#17600.
const result = createSignature(signature.declaration, freshTypeParameters,
signature.thisParameter && instantiateSymbol(signature.thisParameter, mapper),
instantiateList(signature.parameters, mapper, instantiateSymbol),
/*resolvedReturnType*/ undefined,
freshTypePredicate,
/*resolvedTypePredicate*/ signatureHasTypePredicate(signature) ? unresolvedTypePredicate : undefined,
signature.minArgumentCount, signature.hasRestParameter, signature.hasLiteralTypes);
result.target = signature;
result.mapper = mapper;
Expand Down Expand Up @@ -8566,7 +8582,7 @@ namespace ts {
// similar to return values, callback parameters are output positions. This means that a Promise<T>,
// where T is used only in callback parameter positions, will be co-variant (as opposed to bi-variant)
// with respect to T.
const callbacks = sourceSig && targetSig && !sourceSig.typePredicate && !targetSig.typePredicate &&
const callbacks = sourceSig && targetSig && !signatureHasTypePredicate(sourceSig) && !signatureHasTypePredicate(targetSig) &&
(getFalsyFlags(sourceType) & TypeFlags.Nullable) === (getFalsyFlags(targetType) & TypeFlags.Nullable);
const related = callbacks ?
compareSignaturesRelated(targetSig, sourceSig, /*checkAsCallback*/ true, /*ignoreReturnTypes*/ false, reportErrors, errorReporter, compareTypes) :
Expand All @@ -8590,11 +8606,13 @@ namespace ts {
const sourceReturnType = getReturnTypeOfSignature(source);

// The following block preserves behavior forbidding boolean returning functions from being assignable to type guard returning functions
if (target.typePredicate) {
if (source.typePredicate) {
result &= compareTypePredicateRelatedTo(source.typePredicate, target.typePredicate, reportErrors, errorReporter, compareTypes);
const targetTypePredicate = getTypePredicateOfSignature(target);
if (targetTypePredicate) {
const sourceTypePredicate = getTypePredicateOfSignature(source);
if (sourceTypePredicate) {
result &= compareTypePredicateRelatedTo(sourceTypePredicate, targetTypePredicate, reportErrors, errorReporter, compareTypes);
}
else if (isIdentifierTypePredicate(target.typePredicate)) {
else if (isIdentifierTypePredicate(targetTypePredicate)) {
if (reportErrors) {
errorReporter(Diagnostics.Signature_0_must_be_a_type_predicate, signatureToString(source));
}
Expand Down Expand Up @@ -10639,8 +10657,10 @@ namespace ts {
function inferFromSignature(source: Signature, target: Signature) {
forEachMatchingParameterType(source, target, inferFromTypes);

if (source.typePredicate && target.typePredicate && source.typePredicate.kind === target.typePredicate.kind) {
inferFromTypes(source.typePredicate.type, target.typePredicate.type);
const sourceTypePredicate = getTypePredicateOfSignature(source);
const targetTypePredicate = getTypePredicateOfSignature(target);
if (sourceTypePredicate && targetTypePredicate && sourceTypePredicate.kind === targetTypePredicate.kind) {
inferFromTypes(sourceTypePredicate.type, targetTypePredicate.type);
}
else {
inferFromTypes(getReturnTypeOfSignature(source), getReturnTypeOfSignature(target));
Expand Down Expand Up @@ -11403,7 +11423,7 @@ namespace ts {
const apparentType = getApparentType(funcType);
if (apparentType !== unknownType) {
const callSignatures = getSignaturesOfType(apparentType, SignatureKind.Call);
return !!forEach(callSignatures, sig => sig.typePredicate);
return some(callSignatures, sig => signatureHasTypePredicate(sig));
Copy link
Member

Choose a reason for hiding this comment

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

Can you just use signatureHasTypePredicate directly instead?

Copy link
Author

Choose a reason for hiding this comment

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

👍

}
}
}
Expand Down Expand Up @@ -11957,7 +11977,7 @@ namespace ts {
return type;
}
const signature = getResolvedSignature(callExpression);
const predicate = signature.typePredicate;
const predicate = getTypePredicateOfSignature(signature);
if (!predicate) {
return type;
}
Expand Down Expand Up @@ -18122,7 +18142,7 @@ namespace ts {
return;
}

const typePredicate = getSignatureFromDeclaration(parent).typePredicate;
const typePredicate = getTypePredicateOfSignature(getSignatureFromDeclaration(parent));
if (!typePredicate) {
return;
}
Expand Down
11 changes: 8 additions & 3 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3369,7 +3369,14 @@ namespace ts {
/* @internal */
thisParameter?: Symbol; // symbol of this-type parameter
/* @internal */
resolvedReturnType: Type; // Resolved return type
// See comment in `instantiateSignature` for why these are set lazily.
resolvedReturnType: Type | undefined; // Lazily set by `getReturnTypeOfSignature`.
/* @internal */
// Lazily set by `getTypePredicateOfSignature`.
// Unlike `resolvedReturnType`, `undefined` is a valid value, so we use a special sentinel TypePredicate-like instead to indicate a type predicate that must still be computed.
// (See https://github.com/Microsoft/TypeScript/pull/17600#discussion_r132059173)
Copy link
Member

Choose a reason for hiding this comment

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

This link is a bet that our use of github will last as long as the typescript source. Probably a decent bet, but I personally think the explanation stands on its own as a valid justification.

(Same for the use of github bug numbers earlier in the review.)

// Uses a funny type signature to help ensure that this value is not accidentally used as a valid TypePredicate.
resolvedTypePredicate: TypePredicate | void & { __unresolvedTypePredicate: void } | undefined;
/* @internal */
minArgumentCount: number; // Number of non-optional parameters
/* @internal */
Expand All @@ -3387,8 +3394,6 @@ namespace ts {
/* @internal */
isolatedSignatureType?: ObjectType; // A manufactured type that just contains the signature for purposes of signature comparison
/* @internal */
typePredicate?: TypePredicate;
/* @internal */
instantiations?: Map<Signature>; // Generic signature instantiation cache
}

Expand Down
1 change: 1 addition & 0 deletions src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,7 @@ namespace ts {
parameters: Symbol[];
thisParameter: Symbol;
resolvedReturnType: Type;
resolvedTypePredicate: TypePredicate | void & { __unresolvedTypePredicate: void } | undefined;
minTypeArgumentCount: number;
minArgumentCount: number;
hasRestParameter: boolean;
Expand Down
9 changes: 9 additions & 0 deletions tests/baselines/reference/typeInferenceTypePredicate.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//// [typeInferenceTypePredicate.ts]
declare function f<T>(predicate: (x: {}) => x is T): T;
// 'res' should be of type 'number'.
const res = f((n): n is number => true);


//// [typeInferenceTypePredicate.js]
// 'res' should be of type 'number'.
var res = f(function (n) { return true; });
17 changes: 17 additions & 0 deletions tests/baselines/reference/typeInferenceTypePredicate.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
=== tests/cases/compiler/typeInferenceTypePredicate.ts ===
declare function f<T>(predicate: (x: {}) => x is T): T;
>f : Symbol(f, Decl(typeInferenceTypePredicate.ts, 0, 0))
>T : Symbol(T, Decl(typeInferenceTypePredicate.ts, 0, 19))
>predicate : Symbol(predicate, Decl(typeInferenceTypePredicate.ts, 0, 22))
>x : Symbol(x, Decl(typeInferenceTypePredicate.ts, 0, 34))
>x : Symbol(x, Decl(typeInferenceTypePredicate.ts, 0, 34))
>T : Symbol(T, Decl(typeInferenceTypePredicate.ts, 0, 19))
>T : Symbol(T, Decl(typeInferenceTypePredicate.ts, 0, 19))

// 'res' should be of type 'number'.
const res = f((n): n is number => true);
>res : Symbol(res, Decl(typeInferenceTypePredicate.ts, 2, 5))
>f : Symbol(f, Decl(typeInferenceTypePredicate.ts, 0, 0))
>n : Symbol(n, Decl(typeInferenceTypePredicate.ts, 2, 15))
>n : Symbol(n, Decl(typeInferenceTypePredicate.ts, 2, 15))

20 changes: 20 additions & 0 deletions tests/baselines/reference/typeInferenceTypePredicate.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
=== tests/cases/compiler/typeInferenceTypePredicate.ts ===
declare function f<T>(predicate: (x: {}) => x is T): T;
>f : <T>(predicate: (x: {}) => x is T) => T
>T : T
>predicate : (x: {}) => x is T
>x : {}
>x : any
>T : T
>T : T

// 'res' should be of type 'number'.
const res = f((n): n is number => true);
>res : number
>f((n): n is number => true) : number
>f : <T>(predicate: (x: {}) => x is T) => T
>(n): n is number => true : (n: {}) => n is number
>n : {}
>n : any
>true : true

3 changes: 3 additions & 0 deletions tests/cases/compiler/typeInferenceTypePredicate.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
declare function f<T>(predicate: (x: {}) => x is T): T;
// 'res' should be of type 'number'.
Copy link
Member

Choose a reason for hiding this comment

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

change this line to var res: number and start the next line with var res = ... and the compiler will enforce this assertion.

Copy link
Author

@ghost ghost Oct 20, 2017

Choose a reason for hiding this comment

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

I didn't want us taking any contextual type -- presumably we should be doing that on assignments if we're not already.
The baselines do enforce this assertion.

const res = f((n): n is number => true);