-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Changes from 7 commits
bf6a3f7
b2a29de
a072a64
2d0dd82
304665a
7350faa
2bc75c0
9fe2a59
cadf250
7001e71
d52812f
b384c9c
e72f847
aaa6928
3e1b337
8b6fbd7
267c3a9
8e0b1e9
7c5718c
4a8b449
85d70e4
8d2ea56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -275,10 +275,12 @@ namespace ts { | |
const noConstraintType = createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, undefined, undefined); | ||
const circularConstraintType = createAnonymousType(undefined, emptySymbols, emptyArray, emptyArray, undefined, undefined); | ||
|
||
const anySignature = createSignature(undefined, undefined, undefined, emptyArray, anyType, /*typePredicate*/ undefined, 0, /*hasRestParameter*/ false, /*hasLiteralTypes*/ false); | ||
const unknownSignature = createSignature(undefined, undefined, undefined, emptyArray, unknownType, /*typePredicate*/ undefined, 0, /*hasRestParameter*/ false, /*hasLiteralTypes*/ false); | ||
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 noTypePredicate: IdentifierTypePredicate = { kind: TypePredicateKind.Identifier, parameterName: "<<unresolved>>", parameterIndex: 0, type: anyType }; | ||
|
||
const anySignature = createSignature(undefined, undefined, undefined, emptyArray, anyType, /*typePredicate*/ noTypePredicate, 0, /*hasRestParameter*/ false, /*hasLiteralTypes*/ false); | ||
const unknownSignature = createSignature(undefined, undefined, undefined, emptyArray, unknownType, /*typePredicate*/ noTypePredicate, 0, /*hasRestParameter*/ false, /*hasLiteralTypes*/ false); | ||
const resolvingSignature = createSignature(undefined, undefined, undefined, emptyArray, anyType, /*typePredicate*/ noTypePredicate, 0, /*hasRestParameter*/ false, /*hasLiteralTypes*/ false); | ||
const silentNeverSignature = createSignature(undefined, undefined, undefined, emptyArray, silentNeverType, /*typePredicate*/ noTypePredicate, 0, /*hasRestParameter*/ false, /*hasLiteralTypes*/ false); | ||
|
||
const enumNumberIndexInfo = createIndexInfo(stringType, /*isReadonly*/ true); | ||
const jsObjectLiteralIndexInfo = createIndexInfo(anyType, /*isReadonly*/ false); | ||
|
@@ -2826,8 +2828,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(); | ||
|
@@ -3773,8 +3775,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); | ||
|
@@ -5431,14 +5434,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, minArgumentCount: number, hasRestParameter: boolean, hasLiteralTypes: boolean): Signature { | ||
const sig = new Signature(checker); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please be aware that if we call We also end up making As an alternative, I would suggest that you define something like a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we just use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
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; | ||
|
@@ -5447,14 +5450,14 @@ 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[] { | ||
const baseConstructorType = getBaseConstructorTypeOfClass(classType); | ||
const baseSignatures = getSignaturesOfType(baseConstructorType, SignatureKind.Construct); | ||
if (baseSignatures.length === 0) { | ||
return [createSignature(undefined, classType.localTypeParameters, undefined, emptyArray, classType, /*typePredicate*/ undefined, 0, /*hasRestParameter*/ false, /*hasLiteralTypes*/ false)]; | ||
return [createSignature(undefined, classType.localTypeParameters, undefined, emptyArray, classType, /*typePredicate*/ noTypePredicate, 0, /*hasRestParameter*/ false, /*hasLiteralTypes*/ false)]; | ||
} | ||
const baseTypeNode = getBaseTypeNodeOfClass(classType); | ||
const isJavaScript = isInJavaScriptFile(baseTypeNode); | ||
|
@@ -6418,7 +6421,7 @@ namespace ts { | |
const returnType = getSignatureReturnTypeFromDeclaration(declaration, isJSConstructSignature, classType); | ||
const typePredicate = declaration.type && declaration.type.kind === SyntaxKind.TypePredicate ? | ||
createTypePredicateFromTypePredicateNode(declaration.type as TypePredicateNode) : | ||
undefined; | ||
noTypePredicate; | ||
// JS functions get a free rest parameter if they reference `arguments` | ||
let hasRestLikeParameter = hasRestParameter(declaration); | ||
if (!hasRestLikeParameter && isInJavaScriptFile(declaration) && containsArgumentsReference(declaration)) { | ||
|
@@ -6543,6 +6546,20 @@ namespace ts { | |
} | ||
} | ||
|
||
function signatureHasTypePredicate(signature: Signature): boolean { | ||
return signature.resolvedTypePredicate !== noTypePredicate; | ||
} | ||
|
||
function getTypePredicateOfSignature(signature: Signature): TypePredicate | undefined { | ||
if (signature.resolvedTypePredicate === noTypePredicate) { | ||
return undefined; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've inverted the representation, so |
||
if (signature.resolvedTypePredicate === undefined) { | ||
signature.resolvedTypePredicate = instantiateTypePredicate(getTypePredicateOfSignature(signature.target), signature.mapper); | ||
} | ||
return signature.resolvedTypePredicate as TypePredicate; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Write this function in the same style as |
||
|
||
function getReturnTypeOfSignature(signature: Signature): Type { | ||
if (!signature.resolvedReturnType) { | ||
if (!pushTypeResolution(signature, TypeSystemPropertyName.ResolvedReturnType)) { | ||
|
@@ -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, | ||
|
@@ -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 | ||
|
@@ -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) ? undefined : noTypePredicate, | ||
signature.minArgumentCount, signature.hasRestParameter, signature.hasLiteralTypes); | ||
result.target = signature; | ||
result.mapper = mapper; | ||
|
@@ -8567,7 +8583,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) : | ||
|
@@ -8591,11 +8607,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, source.declaration, target.declaration, reportErrors, errorReporter, compareTypes); | ||
const targetTypePredicate = getTypePredicateOfSignature(target); | ||
if (targetTypePredicate) { | ||
const sourceTypePredicate = getTypePredicateOfSignature(source); | ||
if (sourceTypePredicate) { | ||
result &= compareTypePredicateRelatedTo(sourceTypePredicate, targetTypePredicate, source.declaration, target.declaration, 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)); | ||
} | ||
|
@@ -10645,8 +10663,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)); | ||
|
@@ -11407,10 +11427,7 @@ namespace ts { | |
const funcType = checkNonNullExpression(node.expression); | ||
if (funcType !== silentNeverType) { | ||
const apparentType = getApparentType(funcType); | ||
if (apparentType !== unknownType) { | ||
const callSignatures = getSignaturesOfType(apparentType, SignatureKind.Call); | ||
return !!forEach(callSignatures, sig => sig.typePredicate); | ||
} | ||
return apparentType !== unknownType && some(getSignaturesOfType(apparentType, SignatureKind.Call), signatureHasTypePredicate); | ||
} | ||
} | ||
return false; | ||
|
@@ -11963,7 +11980,7 @@ namespace ts { | |
return type; | ||
} | ||
const signature = getResolvedSignature(callExpression); | ||
const predicate = signature.typePredicate; | ||
const predicate = getTypePredicateOfSignature(signature); | ||
if (!predicate) { | ||
return type; | ||
} | ||
|
@@ -18140,7 +18157,7 @@ namespace ts { | |
return; | ||
} | ||
|
||
const typePredicate = getSignatureFromDeclaration(parent).typePredicate; | ||
const typePredicate = getTypePredicateOfSignature(getSignatureFromDeclaration(parent)); | ||
if (!typePredicate) { | ||
return; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3392,7 +3392,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`. | ||
// `undefined` indicates a type predicate that has not yet been computed. | ||
// Uses a special `noTypePredicate` sentinel value to indicate that there is no type predicate. This looks like a TypePredicate at runtime to avoid polymorphism. | ||
// (See https://github.com/Microsoft/TypeScript/pull/17600#discussion_r132059173) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.) |
||
resolvedTypePredicate: TypePredicate | undefined; | ||
/* @internal */ | ||
minArgumentCount: number; // Number of non-optional parameters | ||
/* @internal */ | ||
|
@@ -3410,8 +3417,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 | ||
} | ||
|
||
|
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; }); |
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)) | ||
|
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 | ||
|
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'. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. change this line to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
const res = f((n): n is number => true); |
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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 accessresolvedTypePredicate
and think you're getting the right thing.There was a problem hiding this comment.
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 throughgetTypePredicateOfSignature
.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.
There was a problem hiding this comment.
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 aType
. I hadn't come acrossnoConstraintType
yet, but I'm sure it wouldn't have been easy to realize that a variable of typeType
should not actually be used as aType
because it might be a special sentinel value.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.