Unify JSX And Normal Call Checking Codepaths #27627

weswigham · 2018-10-09T00:04:20Z

So. A long time ago, there existed only JSX class elements. Those elements were typechecked with special rules and behaved only slightly like calls - they were handled like any other expression with their own type checking code and everything. Then came stateless function components - these components behaved a lot more like function calls and merited even more special code to typecheck. Then the requests for correct behavior with respect to overloads, generic defaults, and type inference came, along with type arguments for both of the above, and by every metric, both class and stateless components acted very much like calls; but still, they were not checked as function calls (and indeed, they were not even treated similarly to one another).

Now, they are. This greatly simplifies how we check JSX. No longer does JSX have its own unique call; resolution, overload resolution, excess property checking, and language service codepaths. Now all of the normal variants of those (getResolvedSignature, chooseOverload, isRelatedTo, and getContextualType) all handle a JSX tag (or JSX-style type/signature) as one might expect (that the tag as a whole is like a function call, while the attributes are like an object literal).

This fixes a few outstanding bugs (listed above) by unifying the class and sfc checking codepaths with the more general signature checking codepaths, however this was not without sacrifice (though warranted). SFCs had this unique ability that, given a tag of type (props: A) => SFCComponent1 | (props: B) => SFCComponent2, we would check the passed attributes against every signature, rather than issue an error on the dissimilar signatures (as we do for other kinds of calls). The functionality was removed, and I believe rightfully so. In the context of contextually typed parameters, this had great potential for breakage, as contextual parameter types are fixed to the nodes once inference is completed. rechecking the same "signature" under multiple types would undoubtedly fail to append any types beyond those from the first signature (see checkFunctionExpressionOrObjectLiteralMethod and how it stores a checked flag and fixes types). This functionality should be created via some means like a general solution to #7294 instead, rather than the ad-hoc union-breakdown we were doing before.

Generally, this should be backwards compatible in almost every way; but because I've swapped from ad-hoc-psuedo-overload-resolution to bonafide overload resolution, it's possible that some overload signatures may need to be reordered or modified to continue to function in the intended fashion (I doubt this affects many beyond our test suite, since anyone writing signatures affected by this would be confused as to why writing their SFC as a function call behaved differently than it did as a tag). Previously JSX both skipped the subtype pass and had a strange arity-check-esque rule that wholly overlapped with excess property checking.

Additionally, this process uncovered a small bug in how weak types were checked that impacted some JSX tests once their checking was done correctly which required restructuring how the flag which skips the weak type check was passed down through isRelatedTo.

On the minor change list:

This is technically a breaking API change because I've removed getAllAttributesTypeFromJsxOpeningLikeElement from the API, and it was publicly exposed (why?). Anyone using it should just be using getContextualType instead, and, if the exact same behavior is needed, if they witness a union return type, convert it into an intersection instead (or, like our language service, handle unions intelligently).
- The changes associated with this break should also allow prop types' completions to be discriminated, so this should improve the completion experience for JSX, too.
SFC signature validity checking, by virtue of now using the normal call resolution paths, now uses checkTypeRelatedToAndOptionallyElaborate, meaning it can now have deep elaborations appear on SFC argument errors. Previously this'd only happen for class components.
SFC target types once again include IntrinsicAttributes in error messages. They were removed by a small change a few weeks ago that moved where the SFC error was reported within the SFC checking codepath. If we want them gone again, we should implement the more general "skip intersections of only weak types when elaborating" kinda rule we were thinking about before.

…ture call

weswigham · 2018-10-09T03:35:39Z

Fun fact: Excluding test changes and new tests, this PR now sits at 227 lines added and 569 lines removed. 😄

weswigham · 2018-10-09T03:38:23Z

@typescript-bot test this

typescript-bot · 2018-10-09T03:38:25Z

Heya @weswigham, I've started to run the extended test suite on this PR at c51c496. You can monitor the build here. It should now contribute to this PR's status checks.

sandersn · 2018-10-09T19:57:27Z

Once you fix the JSX excess property checking, it might help #27355 too? I'll try it out.

weswigham · 2018-10-09T20:16:21Z

Nah, #27355 is less an excess property error and more "Property 'b' will always be overridden by the following spread.", IMO. The spread type entirely comes from the declared parameter type in that issue, so an excess property error doesn't make too much sense. In any case, I've fixed excess property errors on spreads (namely: they no longer trigger them, as the intent in #19775 indicates).

weswigham · 2018-10-09T20:17:00Z

@typescript-bot test this - the baseline changes show now just be additional error lines (from the intersection returning) or moved errors from improved elaboration.

typescript-bot · 2018-10-09T20:17:04Z

Heya @weswigham, I've started to run the extended test suite on this PR at 1ebbaa1. You can monitor the build here. It should now contribute to this PR's status checks.

weswigham · 2018-10-09T22:47:10Z

The RWC changes look known now. There's a few places where the extra line with the intersection of intrinsics was added back, one case where the error was moved to the containing tag name, and one case where someone was using the jsx-can-resolve-unions-of-signatures-unintentionally feature (which, as I stated, is now gone as it's very much not safe to do that way it was being done).

sandersn · 2018-10-10T16:32:39Z

src/compiler/checker.ts

-        function isTypeInvalidDueToUnionDiscriminant(contextualType: Type, obj: ObjectLiteralExpression): boolean {
-            return obj.properties.some(property => {
+        function isTypeInvalidDueToUnionDiscriminant(contextualType: Type, obj: ObjectLiteralExpression | JsxAttributes): boolean {
+            const list = obj.properties as NodeArray<ObjectLiteralElementLike | JsxAttributeLike>;


can ObjectLiteralElementLike be expanded to include JsxAttributeLike, or could we introduce yet another union to unify the two?

I only need to perform this cast because we can't resolve signatures of unions, so operating on a union of two arrays is impossible.

sandersn · 2018-10-10T20:46:54Z

src/compiler/checker.ts

@@ -11547,15 +11540,16 @@ namespace ts {
                        return hasExcessProperties(source, discriminant, /*discriminant*/ undefined, reportErrors);
                    }
                    for (const prop of getPropertiesOfObjectType(source)) {
-                        if (!isPropertyFromSpread(prop, source.symbol) && !isKnownProperty(target, prop.escapedName, isComparingJsxAttributes)) {
+                        if (shouldCheckAsExcessProp(prop, source.symbol) && !isKnownProperty(target, prop.escapedName, isComparingJsxAttributes)) {


I would name this Property

sandersn · 2018-10-10T20:52:09Z

src/compiler/checker.ts

@@ -11433,8 +11429,7 @@ namespace ts {

                let result = Ternary.False;
                const saveErrorInfo = errorInfo;
-                const saveIsIntersectionConstituent = isIntersectionConstituent;
-                isIntersectionConstituent = false;
+                let isIntersectionConstituent = !!isApparentIntersectionConstituent;


I think the new distinction between isIntersectionConstituent and isApparentIntersectionConstituent is that isIntersection is not enclosed in checkTypeRelated, so it only propagates through passing to recursive calls. And isApparentIntersectionConstituent is enclosed in checkTypeRelated, but doesn't pass recursively.

Is that accurate? The distinction is subtle and warrants some explanation and/or renaming of variables because it'll be hard to reconstruct the design by reading the code later.

isIntersectionConstituent "propagates" through all calls, but is reset on every visit of isRelatedTo, while isApparentIntersectionConstituent is an argument used to prevent that reset

sandersn · 2018-10-10T22:34:24Z

src/compiler/checker.ts

+                declaration,
+                /*typeParameters*/ undefined,
+                /*thisParameter*/ undefined,
+                [parameterSymbol],


nitpick: This function has so many parameters, it would be easier for me to have a comment on every non-identifier argument. Well, at least 1 and typeSymbol ? ....

This function has so many parameters that it should just take a configuration object.

sandersn · 2018-10-10T22:42:25Z

src/compiler/checker.ts

@@ -17075,7 +17029,7 @@ namespace ts {
                    const hostClassType = getReturnTypeOfSignature(sig);
                    apparentAttributesType = intersectTypes(
                        typeParams
-                            ? createTypeReference(<GenericType>intrinsicClassAttribs, fillMissingTypeArguments([hostClassType], typeParams, getMinTypeArgumentCount(typeParams), isJs))
+                            ? createTypeReference(<GenericType>intrinsicClassAttribs, fillMissingTypeArguments([hostClassType], typeParams, getMinTypeArgumentCount(typeParams), isInJSFile(context)))


this line is too long!

fillMissingTypeArguments call lines are always long.

sandersn · 2018-10-10T22:44:11Z

src/compiler/checker.ts


            if (compilerOptions.jsx === JsxEmit.React && (compilerOptions.jsxFactory || getSourceFileOfNode(node).pragmas.has("jsx"))) {
                error(node, compilerOptions.jsxFactory
                    ? Diagnostics.JSX_fragment_is_not_supported_when_using_jsxFactory
                    : Diagnostics.JSX_fragment_is_not_supported_when_using_an_inline_JSX_factory_pragma);
            }

+            checkJsxChildren(node);


so, uhh, we weren't checking children before?

If the attributes type is non-any, they can get checked via their contributions to the attributes type; however we need to ensure they get visited even in the any/error cases, so we do that here.

sandersn · 2018-10-10T22:48:57Z

src/compiler/checker.ts

                    const intrinsicProp = getPropertyOfType(intrinsicElementsType, escapeLeadingUnderscores(stringLiteralTypeName));
                    if (intrinsicProp) {
-                        return getTypeOfSymbol(intrinsicProp);
+                        const target = getTypeOfSymbol(intrinsicProp);


you can collapse this back to one line, I think.

sandersn · 2018-10-10T23:16:20Z

src/compiler/checker.ts

@@ -19637,6 +19263,7 @@ namespace ts {
            const isTaggedTemplate = node.kind === SyntaxKind.TaggedTemplateExpression;
            const isDecorator = node.kind === SyntaxKind.Decorator;
            const isJsxOpeningOrSelfClosingElement = isJsxOpeningLikeElement(node);
+            const reportErrors = !candidatesOutArray;


I know it's redundant, but it might be more readable to add a reportErrors parameter.

It already has an optional fallbackError parameter that I'd then need to start providing (as undefined) at almost every callsite.

sandersn · 2018-10-10T23:17:35Z

src/compiler/checker.ts

+            // TODO: Since this builds some nodes for the fake'd up signature, this might be worth caching on the node
+            const namespace = getJsxNamespaceAt(node);
+            const exports = namespace && getExportsOfSymbol(namespace);
+            // TODO: We fake up a SFC signature for each intrinsic, however a more specific per-element signature drawn from the JSX declaration


I would decide what to do about these TODOs and remove them. (I suspect the best thing to do for the second one is just to note that the more-specific signature would be preferable.)

The TODOs are here to gather some remarks on what y'all think is appropriate. If I had a solid stance I'd have done it. See, I don't know if the nodes need to be cached, since the signature is already cached when this is called via resolveSignature and since that signature should be the only handle to those generated nodes, it might not be necessary.

sandersn · 2018-10-10T23:27:28Z

Looks good so far. I need to look at the tests still.

sandersn · 2018-10-11T17:11:30Z

tests/baselines/reference/checkJsxChildrenProperty14.errors.txt

+!!! error TS2322: Type '{ children: Element[]; a: number; b: string; }' is not assignable to type 'IntrinsicAttributes & SingleChildProp'.
+!!! error TS2322:   Type '{ children: Element[]; a: number; b: string; }' is not assignable to type 'SingleChildProp'.


I think it was special-case code that got rid of this line in the first place. Is it possible that it just needs to be moved or updated to work again?

As I mentioned in person, this is a result of bringing SFC checking back into line and doing the right thing - there was no "special case" removing the jsx types from the errors; they just straight up weren't being checked against!

sandersn · 2018-10-11T17:12:57Z

tests/baselines/reference/contextuallyTypedStringLiteralsInJsxAttributes02.errors.txt

@@ -37,26 +41,30 @@ tests/cases/conformance/types/contextualTypes/jsxAttributes/file.tsx(36,44): err

    const b0 = <MainButton {...{onClick: (k) => {console.log(k)}}} extra />;  // k has type "left" | "right"
                ~~~~~~~~~~
-!!! error TS2322: Type '{ extra: true; onClick: (k: "left" | "right") => void; }' is not assignable to type 'LinkProps'.
-!!! error TS2322:   Property 'goTo' is missing in type '{ extra: true; onClick: (k: "left" | "right") => void; }'.
+!!! error TS2322: Type '{ extra: true; onClick: (k: "left" | "right") => void; }' is not assignable to type 'IntrinsicAttributes & LinkProps'.


same comment here, although I'm not as sure there actually was special-case code.

sandersn · 2018-10-11T17:32:47Z

tests/baselines/reference/tsxElementResolution10.errors.txt

@@ -18,9 +16,6 @@ tests/cases/conformance/jsx/file.tsx(19,2): error TS2322: Type '{ x: number; ren
    }
    var Obj1: Obj1type;
    <Obj1 x={10} />; // Error, no render member


should this still be an error? The comment says so.

The argument assignability error suppresses the construction error now. Specifically, since no signature matched the arguments provided, an error signature is returned with an anyish return type, which is assignable to ElementClass.

sandersn · 2018-10-11T17:33:48Z

tests/baselines/reference/tsxElementResolution3.errors.txt

The directionality of this error changed, meaning that maybe source swapped with target. Is that correct?

Ehhhhh, not quite. Previous JSX didn't use normal excess-property-checking, and so the normal assignability error would be reported. Excess property errors in the usual codepath are reported at a higher priority than typical assignability errors, while in the old JSX code, excess properties only got checked if the type was otherwise assignable.

sandersn · 2018-10-11T17:40:17Z

tests/baselines/reference/tsxGenericAttributesType2.errors.txt

-    const decorator4 = function <T extends { x: number }>(Component: React.StatelessComponent<T>): React.StatelessComponent<T> {
-        return (props) => <Component {...props} y={"blah"} ></Component>
-                                                ~~~~~~~~~~
-!!! error TS2339: Property 'y' does not exist on type 'IntrinsicAttributes & { x: number; } & { children?: ReactNode; }'.


shouldn't this still be an excess property error?

The source is T & { y: string }, so not by our normal excess property checking rules, no!

sandersn · 2018-10-11T19:54:33Z

tests/cases/fourslash/tsxGoToDefinitionStatelessFunction2.ts

    thirdTarget: "firstSource",
    fourthTarget: "firstSource",
    fifthTarget: "secondSource",
-    sixthTarget: "thirdSource"
+    sixthTarget: "firstSource"


I'm probably super confused, but I thought normal overload resolution chose the last overload in error cases. Why does this switch to the first overload here?

Nope - when overload resolution completely fails we say the declaration is the first signature - see createUnionOfSignaturesForOverloadFailure.

sandersn

I some questions on test changes before signing off.

And after this PR, I'd like to see one that improves error reporting against intersections that contain weak types so that we can get back our nice-ish error messages.

weswigham added 5 commits October 8, 2018 16:25

Unify JSX Call Checking Codepaths

3240f41

Add tests for fixed issues

071b91b

Fix lint, move all error checking into the only-run-once resolveSigna…

f280b6b

…ture call

Remove unused (unreachable?) code path

97df58e

Consolidate a little more duplicated logic into signature checking

c51c496

weswigham requested review from ahejlsberg, sandersn and RyanCavanaugh October 9, 2018 03:36

Fix microsoft#19775 a bit more

1ebbaa1

sandersn self-assigned this Oct 9, 2018

sandersn reviewed Oct 10, 2018

View reviewed changes

sandersn reviewed Oct 11, 2018

View reviewed changes

sandersn requested changes Oct 11, 2018

View reviewed changes

Cosmetic changes from CR

d83f463

sandersn approved these changes Oct 16, 2018

View reviewed changes

weswigham merged commit 7b5ef64 into microsoft:master Oct 17, 2018

weswigham deleted the tsx-rewrite-number-five branch October 17, 2018 00:16

weswigham added the Breaking Change Would introduce errors in existing code label Oct 17, 2018

weswigham added this to the TypeScript 3.2 milestone Oct 17, 2018

weswigham added the Domain: JSX/TSX Relates to the JSX parser and emitter label Oct 17, 2018

weswigham mentioned this pull request Oct 17, 2018

TSX and React Context API broke on nightly #27948

Closed

Jessidhia mentioned this pull request Oct 24, 2018

Add definitions for React 16.6's React.memo DefinitelyTyped/DefinitelyTyped#29990

Closed

8 tasks

johnnyreilly mentioned this pull request Nov 2, 2018

Add type definitions for the proposed React Hooks API DefinitelyTyped/DefinitelyTyped#30057

Merged

8 tasks

sabberworm mentioned this pull request Dec 18, 2018

Compilation failures with TypeScript 3.2 Swizz/snabbdom-pragma#40

Open

		!!! error TS2322: Type '{ children: Element[]; a: number; b: string; }' is not assignable to type 'IntrinsicAttributes & SingleChildProp'.
		!!! error TS2322: Type '{ children: Element[]; a: number; b: string; }' is not assignable to type 'SingleChildProp'.

Unify JSX And Normal Call Checking Codepaths #27627

Unify JSX And Normal Call Checking Codepaths #27627

Conversation

weswigham commented Oct 9, 2018 • edited Loading

weswigham commented Oct 9, 2018

weswigham commented Oct 9, 2018

typescript-bot commented Oct 9, 2018 • edited Loading

sandersn commented Oct 9, 2018

weswigham commented Oct 9, 2018

weswigham commented Oct 9, 2018

typescript-bot commented Oct 9, 2018 • edited Loading

weswigham commented Oct 9, 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

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

sandersn commented Oct 10, 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

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 Oct 9, 2018 •

edited

Loading

typescript-bot commented Oct 9, 2018 •

edited

Loading

typescript-bot commented Oct 9, 2018 •

edited

Loading

weswigham commented Oct 9, 2018 •

edited

Loading