Fix JSX contextual types to not eagerly become apparent, use 2-pass inference for JSX #21383

weswigham · 2018-01-24T01:56:31Z

Fixes the issue I talked about in #21381, which is this:

In JSX, this pattern is presently broken because our JSX type calculation code aggressively gets the apparent type of types it uses to construct the attributes type - contrary to what we'd like for appropriate contextual typing (since the apparent type replaces type parameters with their constraints, preventing us from drawing inferences). I'll have a PR up to fix that shortly.

While I was there, I also generally did a cleanup and conceptual unification pass on the JSX inference code (most of it was actually necessary just to ensure contextual typing didn't trigger inference in a loop when calculating the inferred type for a parameter). Now jsx always gets a 2-pass inference (no 3rd pass for return type inference, since right now jsx expressions have a static type of JSX.Element), just like a normal function call, and is structured much more like inferTypeArguments is in general (including reevaluating the type of the argument expression between each pass). The lack of a second pass could have prevented context sensitive generic functions from acquiring the correct type (which are now represented in the test in this PR).

While working on the test case for this, I encountered #21382 (which I thought was an inference issue until I checked the type baselines and compiler output), which as it turns out is not a JSX-specific issue, so I'm opening this PR despite knowing that quickinfo in a file like the test (or like will be in formik's user test, once they update their type definitions to be inferrable) will be wrong.

weswigham · 2018-01-24T02:02:58Z

Note that this undoes the fix in #21108, but still fixes the underlying issue, since divorcing the codepaths used by checking vs contextual typing for determining the correct signatures to look at accomplishes the same thing (the contextual typing version never instantiates and doesn't get the apparent type unless it has to, the check version continues to behave as it does today).

RyanCavanaugh · 2018-01-24T18:37:02Z

src/compiler/checker.ts

+
+            return mapType(valueType, signturesToParameterTypes);
+
+            function signturesToParameterTypes(valueType: Type) {


Spelling (signatures)

RyanCavanaugh · 2018-01-24T18:45:15Z

src/compiler/checker.ts

-            const paramType = getTypeAtPosition(signature, 0);
-            inferTypes(context.inferences, sourceAttributesType, paramType);
+        function inferJsxTypeArguments(signature: Signature, node: JsxOpeningLikeElement, context: InferenceContext): Type[] {
+            {


If using a block like this isn't a lint violation already, it should be.

Is it actually unsafe to reuse paramType ?

The blocks are just there for organization (as in inferTypeArguments each pass is an identifiable logical block thanks to each cycle of argument iteration), not to allow reuse of the same name safely. I'll remove them, if you'd prefer.

RyanCavanaugh · 2018-01-24T18:47:16Z

src/compiler/checker.ts

+            for (const signature of signatures) {
+                if (signature.typeParameters) {
+                    const isJavascript = isInJavaScriptFile(node);
+                    const inferenceContext = createInferenceContext(signature, /*flags*/ isJavascript ? InferenceFlags.AnyDefault : 0);


We should add InferenceFlags.None so that this ternary can be made typesafe (as-is it'd be possible to pass the wrong kind of enum to createInferenceContext)

RyanCavanaugh · 2018-01-24T18:47:59Z

src/compiler/checker.ts

+                        if (intrinsicClassAttribs !== unknownType) {
+                            const typeParams = getLocalTypeParametersOfClassOrInterfaceOrTypeAlias(intrinsicClassAttribs.symbol);
+                            if (typeParams) {
+                                if (typeParams.length === 1) {


nit: && with the above condition to reduce nesting

RyanCavanaugh · 2018-01-24T18:49:16Z

src/compiler/checker.ts

+                        // There is no property named 'props' on this instance type
+                        return emptyObjectType;
+                    }
+                    else if (isTypeAny(attributesType) || (attributesType === unknownType)) {


isTypeAny(unknownType) is already true

RyanCavanaugh

LGTM after minor fixes 👍

weswigham · 2018-01-24T20:58:55Z

@RyanCavanaugh Done - I've also consolidated a few pieces of functionality.

sandersn

Mostly questions about how things work.

sandersn · 2018-02-01T22:13:51Z

src/compiler/checker.ts

-                return undefined;
-            }
+        function getContextualTypeForChildJsxExpression(node: JsxElement) {
+            const attributesType = getApparentTypeOfContextualType(node.openingElement.tagName);


Is this function tested by the new test? Maybe an existing test?

Added test, it was broken, then I fixed jsx children contextual typing - the issue was that I was checking the jsx attributes, but the jsx children aren't in the subtree of the attributes (despite contributing to its type), so didn't have the contextual type or mapper set by checkExpressionWithContextualType in their heirarchy.

So now it is tested. 🌞 Thanks.

sandersn · 2018-02-01T22:22:16Z

src/compiler/checker.ts

+        function getContextualTypeForChildJsxExpression(node: JsxElement) {
+            const attributesType = getApparentTypeOfContextualType(node.openingElement.tagName);
+            // JSX expression is in children of JSX Element, we will look for an "children" atttribute (we get the name from JSX.ElementAttributesProperty)
+            const jsxChildrenPropertyName = getJsxElementChildrenPropertyname();


Propertyname should capitalize Name

sandersn · 2018-02-01T22:45:19Z

src/compiler/checker.ts

+                return anyType;
+            }
+
+            return mapType(valueType, signaturesToParameterTypes);


Personally I would inline signaturesToParameterTypes

sandersn · 2018-02-01T23:32:28Z

src/compiler/checker.ts

@@ -15298,16 +15331,16 @@ namespace ts {
            //      3. Check if the two are assignable to each other


+            // targetAttributesType is a type of an attributes from resolving tagName of an opening-like JSX element.


Typo:attribute

sandersn · 2018-02-01T23:38:37Z

src/compiler/checker.ts

+        function inferJsxTypeArguments(signature: Signature, node: JsxOpeningLikeElement, context: InferenceContext): Type[] {
+            // Skip context sensitive pass
+            const skipContextParamType = getTypeAtPosition(signature, 0);
+            const checkAttrTypeSkipContextSensitive = checkExpressionWithContextualType(node.attributes, skipContextParamType, identityMapper);


I don’t understand this pass, or at least the name of this pass. We grab the type of the first parameter, then use it as the contextual type against node.attributes, instead of just taking the type from outside this function. But then we call getTypeAtPosition again — do we expect something different this time?

Yep, we do expect something different. Namely we expect contextual parameter types to have been assigned from the first pass. We do the same thing in normal inference where we call checkExpression separately in each pass on an argument.

Oh, right, because inferTypes is side-effecting. Or is is checkExpressionWithContextualType.

I still don't understand how this skips the context param though. Or does it skip the context[ual type] of the param?

checkExpressionWithContextualType is the sideffecting one. It can cause assignments to cached parameter types when called with an inference context as the contextual mapper. (so the first call isn't side-effecting, but is inference generating, while the second is side-effecting and inference generating)

sandersn · 2018-02-01T23:47:21Z

src/compiler/checker.ts

+                    if (intrinsicClassAttribs !== unknownType) {
+                        const typeParams = getLocalTypeParametersOfClassOrInterfaceOrTypeAlias(intrinsicClassAttribs.symbol);
+                        apparentAttributesType = intersectTypes(
+                            length(typeParams)


In the previous version if typeParams had length 0, then apparentAttributesType doesn’t change. Now it gets intersected with intrinsicClassAttribs. Is that OK?

A length of 0 for type params isn't possible; we don't make a type param list, then, I don't think. HOWEVER, the bigger issue that you've called to attention is that if the intrinsic class attributes type has more than one parameter (not correct, but I don't think there's an error for it), this now intersects a class type reference that's missing arguments (instead of nothing), I think it should do our normal type parameter-filling behavior using fillMissingTypeArguments. Meaning you should be able to write interface ElementClass<T, Props = {}> { /*...*/ } and actually have it work. I'll add that now. 🌞

Done. I think this is now much more consistent with how we handle type arguments and generics elsewhere.

sandersn · 2018-02-01T23:50:36Z

src/compiler/checker.ts

+                    // There is no property named 'props' on this instance type
+                    return emptyObjectType;
+                }
+                else if (isTypeAny(attributesType)) {


Why drop the unknownType check? Or does isTypeAny work for unknownType too?

Yes. @RyanCavanaugh pointed that out:

const unknownType = createIntrinsicType(TypeFlags.Any, "unknown"); //... function isTypeAny(type: Type) { return type && (type.flags & TypeFlags.Any) !== 0; }

sandersn · 2018-02-01T23:53:26Z

src/compiler/types.ts

@@ -3846,6 +3846,7 @@ namespace ts {
    }

    export const enum InferenceFlags {
+        None            =      0,  // No special inference behaviors


I kind of like 0. It’s pretty succinct.

But we have None for so many other enums - plus, it also clarifies what 0 actually does at the callsite to someone reading the code.

weswigham · 2018-02-03T02:07:38Z

ping @sandersn I'm done implementing your feedback; wanna take another look when you get a chance?

chrisbenti · 2018-02-05T19:36:58Z

this fixes my repo @ lyft

chrisbenti · 2018-02-05T19:37:05Z

Thanks @weswigham !

sandersn · 2018-02-05T21:48:48Z

src/compiler/checker.ts

@@ -14788,7 +14788,7 @@ namespace ts {
                    }
                }
                else {
-                    childrenTypes.push(checkExpression(child, checkMode));
+                    childrenTypes.push(checkExpressionForMutableLocation(child, checkMode));


Why mutable location?

jsxchildren are like a jsxattribute are like object members and should be checked the same way.

sandersn · 2018-02-05T21:56:19Z

src/compiler/checker.ts

@@ -14095,7 +14104,7 @@ namespace ts {
                }
            }

-            return getUnionType(map(signatures, ctor ? getJsxPropsTypeFromConstructSignature : getJsxPropsTypeFromCallSignature), UnionReduction.None);
+            return getUnionType(map(signatures, ctor ? isJs ? getJsxPropsTypeFromConstructSignatureJs : getJsxPropsTypeFromConstructSignature : getJsxPropsTypeFromCallSignature), UnionReduction.None);


It seems a lot easier to read to write this as x => getJsxPropsTypeFromConstructSignature(x, isJs) — and everywhere else you added function pairs.

But that'd introduce a lambda on every invocation here, and this is called in contextual typing (which is done often), whereas this way only two functions get made ever. I do agree that that looks better, I just think it'd be detrimental to perf. :(

[1] Did you measure? You’re still doing dynamic dispatch here so I can’t imagine the numbers would be that different (though I am not sure how good the VMs are at lifting lambdas).
[2] At least extract the double-nested conditional to a local. That’s just egregious. :]

sandersn

Looks good after you [1] add a test for fillMissingTypeArguments [2] make sure that DefinitelyTyped doesn’t break too much from the improved inferences.

weswigham · 2018-02-05T23:08:48Z

@sandersn Test added. (And a small change to actually-do-the-right-thing-and-not-throw). Thanks. 🐱

weswigham · 2018-02-06T00:32:40Z

@sandersn I ran the dt tests and this PR doesn't affect any baselines, when compared to master. 🌞 (I somehow feel like this is a failing of the tests on dt, because it definitely should - leaflet, for example, definitely has generic jsx elements in it - the tests must not use them extensively)

…nference for JSX (microsoft#21383) * Fix JSX contextual types to not eagerly become apparent * Apply changes from code review, unify common code * Fix jsx children contextual typing * Light code review feedback * Use fillMissingTypeArguments * Accept nonliteral jsx child type * Add test for the fillMissingTypeArguments case

…nference for JSX (#21383) (#21749) * Fix JSX contextual types to not eagerly become apparent * Apply changes from code review, unify common code * Fix jsx children contextual typing * Light code review feedback * Use fillMissingTypeArguments * Accept nonliteral jsx child type * Add test for the fillMissingTypeArguments case

Fix JSX contextual types to not eagerly become apparent

8cd9d9d

weswigham requested review from sandersn, DanielRosenwasser, RyanCavanaugh and mhegazy January 24, 2018 01:56

weswigham mentioned this pull request Jan 24, 2018

Add formik to the user suite #21381

Merged

This was referenced Jan 24, 2018

Generic JSX props don't work properly when props contain generic functions #20891

Closed

Change layout of generics in main Formik class to allow better inference jaredpalmer/formik#370

Merged

RyanCavanaugh reviewed Jan 24, 2018

View reviewed changes

RyanCavanaugh approved these changes Jan 24, 2018

View reviewed changes

Apply changes from code review, unify common code

0a08644

sandersn requested changes Feb 1, 2018

View reviewed changes

weswigham mentioned this pull request Feb 2, 2018

Accept new user test baselines #21576

Merged

weswigham added 3 commits February 2, 2018 17:19

Fix jsx children contextual typing

7c63c49

Light code review feedback

122a7e3

Use fillMissingTypeArguments

59fc025

weswigham added 2 commits February 5, 2018 13:30

Merge branch 'master' into fix-jsx-contextual-types

c1df4ce

Accept nonliteral jsx child type

774aec8

sandersn reviewed Feb 5, 2018

View reviewed changes

sandersn approved these changes Feb 5, 2018

View reviewed changes

Add test for the fillMissingTypeArguments case

f1c9736

weswigham merged commit 17554ff into master Feb 6, 2018

weswigham mentioned this pull request Feb 6, 2018

Updated formik baselines #21660

Merged

weswigham mentioned this pull request Feb 6, 2018

Fix JSX contextual types to not eagerly become apparent, use 2-pass inference for JSX #21661

Closed

weswigham deleted the fix-jsx-contextual-types branch February 6, 2018 01:03

weswigham mentioned this pull request Feb 7, 2018

Fix JSX contextual types to not eagerly become apparent, use 2-pass i… #21749

Merged

microsoft locked and limited conversation to collaborators Jul 3, 2018


		return mapType(valueType, signturesToParameterTypes);

		function signturesToParameterTypes(valueType: Type) {

		@@ -15298,16 +15331,16 @@ namespace ts {
		// 3. Check if the two are assignable to each other


		// targetAttributesType is a type of an attributes from resolving tagName of an opening-like JSX element.

Fix JSX contextual types to not eagerly become apparent, use 2-pass inference for JSX #21383

Fix JSX contextual types to not eagerly become apparent, use 2-pass inference for JSX #21383

Conversation

weswigham commented Jan 24, 2018

weswigham commented Jan 24, 2018

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

Choose a reason for hiding this comment

RyanCavanaugh left a comment

Choose a reason for hiding this comment

weswigham commented Jan 24, 2018

sandersn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham Feb 3, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weswigham commented Feb 3, 2018

chrisbenti commented Feb 5, 2018

chrisbenti commented Feb 5, 2018

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

weswigham commented Feb 5, 2018

weswigham commented Feb 6, 2018

weswigham Feb 3, 2018 •

edited

Loading

weswigham Feb 2, 2018 •

edited

Loading

weswigham Feb 3, 2018 •

edited

Loading

weswigham Feb 2, 2018 •

edited

Loading