Lazily compute signature type predicates · Pull Request #17600 · microsoft/TypeScript

ghost · 2017-08-03T22:25:05Z

Fixes #17451
EDIT: And #19640 and #19642 and #20186.

To summarize the problem:

During the first quickInfo, we start from a blank slate.

In chooseOverload:

First time around, we set excludedArgument to [true], meaning that we exclude the sole argument.
Obviously, we get no type inferences from this. So the inferred type argument is {}.
Then in the next round, excludeArgument will be undefined. This means we enter the body of checkFunctionExpressionOrObjectLiteralMethod; particularly where we call instantiateSignature(contextualSignature, contextualMapper);.
In instantiating the contextual signature, we end up calling an innocent-looking function called cloneTypePredicate. This uses the contextual mapper to call map the contextual type predicate type to the actual type predicate type.
By calling the mapper (created by createInferenceContext), we cause a side-effect of fixing the inference -- so it gets stuck at {} forever.

The statefulness in the original issue is because by the second quickInfo, we've already checked the function, and its return type is (n: {}) => n is number. The explicit : n is number annotation overrides the contextual type. So getTypeOfFuncClassEnumModule returns a completed full type just an empty new one, and we can infer types correctly. During the first quickInfo we didn't have a type for the function yet, so type inference problems only showed up then.

We had already solved this sort of problem for contextual return types: compute them lazily so that we don't call the contextual mapper until necessary. So the solution is simply to do the same for the type predicate.

ghost · 2017-08-03T22:28:37Z

For posterity, the test I was using to reproduce #17451 was:

///<reference path="fourslash.ts"/>

// @noLib: true

////declare function f<T>(predicate: (t: {}) => t is T): T;
////const y = /**/f((n): n is number => true);

verify.quickInfoAt("", "function f<{}>(predicate: (t: {}) => t is {}): {}"); // OK

console.log("First succeeded...");

verify.quickInfoAt("", "function f<{}>(predicate: (t: {}) => t is {}): {}"); // Fails

sandersn

I like the delay -- it should have always been this way to mirror the way return types are treated -- but I wish there were a more uniform way to represent 'pending' vs 'missing'. The problem is that for efficiency, the common case should be 'undefined', which for type predicates is 'missing' and for return types is 'pending'.

sandersn · 2017-08-03T22:53:22Z

+        }
+
+        function getTypePredicateOfSignature(signature: Signature): TypePredicate {
+            if (signature.resolvedTypePredicate === "pending") {


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

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.

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

rbuckton · 2017-08-08T23:34:24Z


        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 {


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.

Could we just use null for pending?

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

… type predicate

rbuckton · 2017-08-09T15:02:56Z

        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 } = (() => {


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

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.

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.

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.

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

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.

rbuckton · 2017-08-09T15:07:36Z

                    if (apparentType !== unknownType) {
                        const callSignatures = getSignaturesOfType(apparentType, SignatureKind.Call);
-                        return !!forEach(callSignatures, sig => sig.typePredicate);
+                        return some(callSignatures, sig => signatureHasTypePredicate(sig));


Can you just use signatureHasTypePredicate directly instead?

rbuckton · 2017-08-09T15:12:55Z

+            return signature.resolvedTypePredicate !== undefined;
+        }
+
+        function getTypePredicateOfSignature(signature: Signature): TypePredicate | undefined {


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.

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

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

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

rbuckton · 2017-08-09T17:26:58Z

Can you take a look at the build failures?

…edicate` instead of `undefined` when in all `createSignature` calls

ghost · 2017-08-29T14:48:04Z

@rbuckton @ahejlsberg Any more comments?

ghost · 2017-09-13T20:26:48Z

Bump -- @rbuckton @ahejlsberg Is anything blocking this?

ghost · 2017-09-22T21:09:02Z

@rbuckton Good to go?

ahejlsberg · 2017-10-04T21:07:04Z

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


Write this function in the same style as getConstraintFromTypeParameter.

ghost · 2017-10-06T15:40:33Z

New commit contains fixes to the union half of #17757. Intersecting type predicates is marked as a TODO.
Previously we considered signatures identical even if they had different type predicates; this meant a union of two different type predicates would just contain the first one.
@sandersn @ahejlsberg Needs new review for these new changes.

ghost · 2017-10-20T14:49:02Z

@sandersn @rbuckton @ahejlsberg Anyone?

sandersn

One test assertion in a comment can be checked by the compiler. Otherwise looks good.

sandersn · 2017-10-20T16:08:26Z

@@ -0,0 +1,3 @@
+declare function f<T>(predicate: (x: {}) => x is T): T;
+// 'res' should be of type 'number'.


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

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.

sandersn · 2017-10-20T16:08:47Z

+type Or = A | B;
+
+function f(o: Or, x: {}, y: {}) {
+    if (o.pred(x, y)) {


interesting.. why doesn't this match?

oh, it's because you can't make a single type predicate that narrows two variables.

sandersn · 2017-10-20T16:22:48Z

+        // 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)


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.)

sandersn · 2017-10-20T16:27:46Z

        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);
+            return createSignature(sig.declaration, sig.typeParameters, sig.thisParameter, sig.parameters, /*resolvedReturnType*/ undefined,


odd, I thought cloneSignature already zeroed out resolvedReturnType.

oh, right, this is from that analysis you did which showed that all callers of cloneSignature immediately zeroed it out themselves.

sandersn · 2017-10-20T17:16:55Z

+        function getUnionTypePredicate(signatures: ReadonlyArray<Signature>): TypePredicate {
+            let first: TypePredicate | undefined;
+            const types: Type[] = [];
+            for (let i = 0; i < signatures.length; i++) {


why not for-of here?

If we did #19309 we could enable prefer-for-of to catch this kind of error.

ahejlsberg · 2017-10-25T22:00:18Z

+                getIndexTypeOfStructuredType,
+                getConstraintFromTypeParameter,
+                getFirstIdentifier,
+            ),


Did anything change here? Otherwise, just leave it alone.

ahejlsberg · 2017-10-25T22:02:21Z

+                hasRestParameter,
+                hasLiteralTypes) {
+                return createSignature(declaration, typeParameters, thisParameter, parameters, resolvedReturnType, resolvedTypePredicate || noTypePredicate, minArgumentCount, hasRestParameter, hasLiteralTypes);
+            },


Why the change here?

ahejlsberg · 2017-10-25T22:03:42Z

            sig.thisParameter = thisParameter;
            sig.resolvedReturnType = resolvedReturnType;
-            sig.typePredicate = typePredicate;
+            sig.resolvedTypePredicate = resolvedTypePredicate;


I would just do typePredicate || noTypePredicate here and then you won't need all the other changes.

ahejlsberg · 2017-10-25T23:03:33Z

I just pushed a branch with some suggested changes:

https://github.com/Microsoft/TypeScript/tree/typePredicateChanges

This makes the noTypePredicate sentinel purely an implementation detail of getTypePredicateOfSignature and aligns better with how getReturnTypeOfSignature works.

…tateOfSignature

ghost · 2017-11-01T14:35:03Z

@ahejlsberg Good to go?

ahejlsberg · 2017-11-02T17:22:20Z

@Andy-MS Also, check to see if this fixes #19676.

ghost · 2017-11-21T16:25:12Z

#19676 is fixed, tested for by typeInferenceTypePredicate.ts. (And tested with local build.)

ghost · 2017-11-21T16:26:09Z


    isArray = ('isArray' in Array) ?
->isArray = ('isArray' in Array) ?        Array.isArray : function (value) { return Object.prototype.toString.call(value) === '[object Array]'; } : (arg: any) => arg is any[]
+>isArray = ('isArray' in Array) ?        Array.isArray : function (value) { return Object.prototype.toString.call(value) === '[object Array]'; } : (value: any) => boolean


I think this change is correct because only the left hand side of the conditional is a type predicate. @ahejlsberg could you verify, then merge

Something seems fishy here. If you reverse the order of the operands you get the old result. So apparently both types are subtypes of each other, or otherwise subtype reduction would consistently pick one of them. I'm not sure why this is the case, since (x: any) => x is any[] should be a subtype of (x: any) => boolean. You may want to investigate.

Added a test -- the return type seems to be boolean the other way around as well.

Ok, looks good.

ahejlsberg · 2017-12-03T02:04:14Z

I think this PR is good to go.

Note: This includes a workaround to a type inferring issue fixed by microsoft/TypeScript#17600. When Typescript 2.7 is released, these forms can be removed.

Lazily compute signature type predicates

bf6a3f7

ghost requested review from ahejlsberg and sandersn August 3, 2017 22:25

msftclas added the cla-not-required label Aug 3, 2017

sandersn approved these changes Aug 3, 2017

View reviewed changes

rbuckton suggested changes Aug 8, 2017

View reviewed changes

Andy Hanson added 2 commits August 9, 2017 07:49

Use an instance of IdentifierTypePredicate to represent an unresolved…

b2a29de

… type predicate

Merge branch 'master' into typepredicate

a072a64

rbuckton suggested changes Aug 9, 2017

View reviewed changes

Andy Hanson added 2 commits August 9, 2017 08:41

Simplify getMaybeTypePredicate

2d0dd82

Invert representation of resolvedTypePredicate

304665a

Andy Hanson added 2 commits August 9, 2017 10:45

Remove __unresolvedTypePredicate type and remember to use `noTypePr…

7350faa

…edicate` instead of `undefined` when in all `createSignature` calls

Merge branch 'master' into typepredicate

2bc75c0

msftgits removed the cla-not-required label Sep 26, 2017

ahejlsberg reviewed Oct 4, 2017

View reviewed changes

Andy Hanson added 3 commits October 4, 2017 14:56

Merge branch 'master' into typepredicate

9fe2a59

Fix style of getTypePredicateOfSignature

cadf250

Use in createGetSymbolWalker

7001e71

ghost mentioned this pull request Oct 6, 2017

Unions and intersections of type predicates produce wrong type #17757

Closed

Fix bugs for unions of type predicates

d52812f

ghost force-pushed the typepredicate branch from 7372208 to d52812f Compare October 6, 2017 16:51

ghost mentioned this pull request Oct 20, 2017

Type inference fails with immediate type guards #18562

Closed

Merge branch 'master' into typepredicate

b384c9c

ghost force-pushed the typepredicate branch from 3d9ad82 to b384c9c Compare October 20, 2017 15:42

sandersn approved these changes Oct 20, 2017

View reviewed changes

Code review

e72f847

ahejlsberg reviewed Oct 25, 2017

View reviewed changes

ahejlsberg and others added 4 commits October 30, 2017 11:17

Make noTypePredicate purely an implementation detail of getTypePredic…

aaa6928

…tateOfSignature

Merge branch 'master' into typepredicate

3e1b337

Add test

8b6fbd7

Merge branch 'master' into typepredicate

267c3a9

This was referenced Nov 1, 2017

Array/Observable filter does not infer chained type #19640

Closed

Can't use is type asserting function as predicate parameter #19642

Closed

Add test for #19642

8e0b1e9

ahejlsberg approved these changes Nov 2, 2017

View reviewed changes

trxcllnt mentioned this pull request Nov 21, 2017

Narrowed types from user defined type guards are lost in subsequent method calls #20186

Closed

Merge branch 'master' into typepredicate

7c5718c

ghost commented Nov 21, 2017

View reviewed changes

Add test with reversed order

4a8b449

ahejlsberg added this to the TypeScript 2.7 milestone Dec 3, 2017

Andy Hanson added 2 commits December 4, 2017 08:36

Merge branch 'master' into typepredicate

85d70e4

Merge branch 'master' into typepredicate

8d2ea56

ghost merged commit 8153397 into master Dec 5, 2017

ghost deleted the typepredicate branch December 5, 2017 16:32

microsoft locked and limited conversation to collaborators Jun 14, 2018

This pull request was closed.

		@@ -0,0 +1,3 @@
		declare function f<T>(predicate: (x: {}) => x is T): T;
		// 'res' should be of type 'number'.

Conversation

ghost commented Aug 3, 2017 • edited by ahejlsberg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 3, 2017

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Aug 9, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton commented Aug 9, 2017

Uh oh!

ghost commented Aug 29, 2017

Uh oh!

ghost commented Sep 13, 2017

Uh oh!

ghost commented Sep 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 6, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 20, 2017

Uh oh!

sandersn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Oct 20, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ghost commented Aug 3, 2017 •

edited by ahejlsberg

Loading

ghost Aug 9, 2017 •

edited by ghost

Loading

ghost commented Oct 6, 2017 •

edited by ghost

Loading

ghost Oct 20, 2017 •

edited by ghost

Loading

ahejlsberg commented Oct 25, 2017 •

edited

Loading