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 1 commit
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
61 changes: 38 additions & 23 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2825,8 +2825,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 @@ -3776,8 +3776,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 @@ -5434,14 +5435,14 @@ namespace ts {
}

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 | undefined | "pending", minArgumentCount: number, hasRestParameter: boolean, hasLiteralTypes: boolean): Signature {
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.

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 @@ -5450,7 +5451,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 @@ -6536,6 +6537,17 @@ namespace ts {
}
}

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

function getTypePredicateOfSignature(signature: Signature): TypePredicate {
if (signature.resolvedTypePredicate === "pending") {
Copy link
Member

Choose a reason for hiding this comment

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

why do we need a third state when resolvedReturnType doesn't?

Copy link
Member

Choose a reason for hiding this comment

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

oh, because all signatures have a return type (eventually), but not all signatures will have a typePredicate. So "pending" for resolvedReturnType is the equivalent of undefined for resolvedTypePredicate.

Copy link
Member

Choose a reason for hiding this comment

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

oh, this is documented in the definition in types.ts

signature.resolvedTypePredicate = instantiateTypePredicate(getTypePredicateOfSignature(signature.target), signature.mapper);
}
return signature.resolvedTypePredicate;
}

function getReturnTypeOfSignature(signature: Signature): Type {
if (!signature.resolvedReturnType) {
if (!pushTypeResolution(signature, TypeSystemPropertyName.ResolvedReturnType)) {
Expand Down Expand Up @@ -8042,7 +8054,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 @@ -8061,7 +8073,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 @@ -8072,14 +8083,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) ? "pending" : undefined,
signature.minArgumentCount, signature.hasRestParameter, signature.hasLiteralTypes);
result.target = signature;
result.mapper = mapper;
Expand Down Expand Up @@ -8506,7 +8517,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 @@ -8530,11 +8541,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 @@ -10546,8 +10559,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 @@ -11321,7 +11336,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));
}
}
}
Expand Down Expand Up @@ -11875,7 +11890,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 @@ -18028,7 +18043,7 @@ namespace ts {
return;
}

const typePredicate = getSignatureFromDeclaration(parent).typePredicate;
const typePredicate = getTypePredicateOfSignature(getSignatureFromDeclaration(parent));
if (!typePredicate) {
return;
}
Expand Down
9 changes: 6 additions & 3 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3363,7 +3363,12 @@ 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 "pending" instead to indicate a type predicate that must still be computed.
resolvedTypePredicate: TypePredicate | undefined | "pending";
/* @internal */
minArgumentCount: number; // Number of non-optional parameters
/* @internal */
Expand All @@ -3381,8 +3386,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 | undefined | "pending";
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);