Make getContextualTypeOfApparentType mapType over unions #17668

weswigham · 2017-08-07T23:19:32Z

This allows us to actually find member types for union-typed object members in the same way we do for non-unions.

sandersn

Had a couple of questions on my first pass:

sandersn · 2017-08-08T18:23:01Z

src/compiler/checker.ts

+         */
+        function getInstantiatedContextualType(node: Expression, checkMode?: CheckMode): Type {
+            if (checkMode === CheckMode.Inferential && node.contextualType && node.contextualMapper) {
+                return instantiateType(node.contextualType, node.contextualMapper);


don't you still need to get an apparent type in case the result is still a type parameter?

Absolutely. I used to be doing it at use-site, but forgot to add it back when I decided not to. Good catch; that's actually why there were baseline changes, and why the build was broken. It's a much neater fix now. 🐱

sandersn · 2017-08-08T18:23:39Z

src/compiler/checker.ts

@@ -16844,7 +16855,7 @@ namespace ts {

            // Check if function expression is contextually typed and assign parameter types if so.
            if (!(links.flags & NodeCheckFlags.ContextChecked)) {
-                const contextualSignature = getContextualSignature(node);
+                const contextualSignature = getContextualSignature(node); // Intentionally do not pass check mode; resolve without instantiation


why? Because it's not NodeCheckFlags.ContextChecked?

That's what I would assume; experimentally, the real issue is that we fail to make a ton of inferences when this code path does propagate the check mode.

sandersn

Looks good to me, although it would be nice to have @ahejlsberg take a look at a structural change like this.

rbuckton

@ahejlsberg, should we be caching the types for any CheckMode other than Normal? It seems we cache the type in checkExpressionCached as well regardless of the provided CheckMode value.

rbuckton · 2017-08-09T01:03:32Z

src/compiler/checker.ts

@@ -4515,7 +4515,7 @@ namespace ts {
                }
                // Handle export default expressions
                if (declaration.kind === SyntaxKind.ExportAssignment) {
-                    return links.type = checkExpression((<ExportAssignment>declaration).expression);
+                    return links.type = checkExpression((<ExportAssignment>declaration).expression, checkMode);


It doesn't seem like we should be holding on to the type here if this is not a normal check mode.

Will this ever be anything apart from normal?

Inferential. The reason its even being threaded through here is to catch when it is set to inferential.

DanielRosenwasser · 2017-09-09T08:09:36Z

src/compiler/checker.ts

            return type && getApparentType(type);
        }

        /**
+         * This must be called when a contextual type function is attempting to dig into a type to contextually type a subexpression
+         * Otherwise, contextual typing stops when a type argument is encountered, and `any` is ued instead.


DanielRosenwasser · 2017-09-19T20:37:32Z

Do you happen to know if this fixes #16136?

DanielRosenwasser · 2017-10-14T00:13:37Z

Can you add the following simple test case since, as you mentioned, this isn't necessarily tied to optionality?

interface Bar {
    bar: (a: number) => void;
}

declare function foo<T extends Bar>(x: string | T): T

let x = foo({
    bar(x) {
    }
})

weswigham · 2017-10-18T20:41:48Z

@DanielRosenwasser Synced with master and updated tests. Also I've checked; this does not fix #16136 (though it may not be possible to fix that without this change done in conjunction with it).

weswigham · 2017-10-18T22:57:38Z

I've modified the solution to work for array literals in addition to object literals. Also theoretically JSX, but JSX apparently never does an inferential pass because it's "overload resolution" is not real overload resolution (but I've included a test showcasing the issue).

ahejlsberg · 2017-10-20T14:23:50Z

I'm concerned with the cost of instantiating contextual types. We don't cache contextual types, yet obtaining a contextual type is assumed to be cheap and we do it at will in a number of places. Furthermore, if we're going to instantiate contextual types, it seems like we should always do it and not just when obtaining a member of a contextual object type. Indeed, with the approach in this PR I think we potentially end up instantiating types that were already instantiated (e.g. if we have nested object literals).

ahejlsberg · 2017-10-20T15:51:12Z

I should add also that instantiation of contextual types is deferred by design such that we don't prematurely "pull" on type parameters for which we are currently performing inference. For example, when assigning contextual types to function expression or arrow function arguments, we do instantiate the contextual type, but only once we have drilled down to the corresponding part of the contextual type. Thus, we only fix type parameters if they are actually referenced in the (part of the) contextual type for the function.

After much deliberation and inspection, much simpler fix Undo Redo

weswigham · 2017-10-21T00:27:32Z

@ahejlsberg Implemented the waaaaay simpler fix we looked at today; which interestingly, still doesn't fix the JSX case.

weswigham requested review from sandersn, rbuckton and ahejlsberg August 7, 2017 23:19

msftclas added the cla-already-signed label Aug 7, 2017

sandersn reviewed Aug 8, 2017

View reviewed changes

weswigham added 3 commits August 8, 2017 12:40

Instantiate contextual types while in an inferrential context

87063a6

Limit scope of instantiation to only when likely needed

c874409

Still get aparent type

9ccdf04

weswigham force-pushed the contextual-type-of-optional-members branch from 33fe6f9 to 9ccdf04 Compare August 8, 2017 19:45

sandersn approved these changes Aug 8, 2017

View reviewed changes

rbuckton reviewed Aug 9, 2017

View reviewed changes

DanielRosenwasser reviewed Sep 9, 2017

View reviewed changes

msftgits removed the cla-already-signed label Sep 26, 2017

DanielRosenwasser approved these changes Oct 14, 2017

View reviewed changes

weswigham added 3 commits October 18, 2017 13:25

Merge branch 'master' into contextual-type-of-optional-members

ce23fb0

Expand test

0561810

Fix nit

522df7b

weswigham added 2 commits October 18, 2017 15:06

Handle JSX and array

746ce5a

Tests for the JSX and Array cases

a4b2000

weswigham changed the title ~~Sometimes instantiate the contextual type of optional members~~ Sometimes instantiate the contextual type of members during inferential typing Oct 19, 2017

weswigham mentioned this pull request Oct 19, 2017

Retain literal types in some contextual unions #19322

Closed

Merge branch 'master' into contextual-type-of-optional-members

829944f

weswigham changed the title ~~Sometimes instantiate the contextual type of members during inferential typing~~ Make getContextualTypeOfApparentType mapType over unions Oct 21, 2017

weswigham force-pushed the contextual-type-of-optional-members branch from 09c2693 to 0864f7d Compare October 21, 2017 00:23

After much deliberation and inspection, much simpler fix

0d3a4ae

After much deliberation and inspection, much simpler fix Undo Redo

weswigham force-pushed the contextual-type-of-optional-members branch from 0864f7d to 0d3a4ae Compare October 21, 2017 00:24

ahejlsberg approved these changes Nov 6, 2017

View reviewed changes

weswigham merged commit 0593ba2 into microsoft:master Nov 6, 2017

weswigham deleted the contextual-type-of-optional-members branch November 6, 2017 20:52

weswigham mentioned this pull request Nov 8, 2017

Fails to get contextual type for string literal in array #19837

Closed

microsoft locked and limited conversation to collaborators Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make getContextualTypeOfApparentType mapType over unions #17668

Make getContextualTypeOfApparentType mapType over unions #17668

weswigham commented Aug 7, 2017 •

edited

Loading

sandersn left a comment

sandersn Aug 8, 2017

weswigham Aug 8, 2017

sandersn Aug 8, 2017

weswigham Aug 8, 2017

sandersn left a comment

rbuckton left a comment

rbuckton Aug 9, 2017

DanielRosenwasser Sep 9, 2017

weswigham Sep 9, 2017

DanielRosenwasser Sep 9, 2017

DanielRosenwasser commented Sep 19, 2017

DanielRosenwasser commented Oct 14, 2017

weswigham commented Oct 18, 2017

weswigham commented Oct 18, 2017

ahejlsberg commented Oct 20, 2017

ahejlsberg commented Oct 20, 2017

weswigham commented Oct 21, 2017

Make getContextualTypeOfApparentType mapType over unions #17668

Make getContextualTypeOfApparentType mapType over unions #17668

Conversation

weswigham commented Aug 7, 2017 • edited Loading

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn left a comment

Choose a reason for hiding this comment

rbuckton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielRosenwasser commented Sep 19, 2017

DanielRosenwasser commented Oct 14, 2017

weswigham commented Oct 18, 2017

weswigham commented Oct 18, 2017

ahejlsberg commented Oct 20, 2017

ahejlsberg commented Oct 20, 2017

weswigham commented Oct 21, 2017

weswigham commented Aug 7, 2017 •

edited

Loading