Address code review

Kanchalai Tanglertsampan · Kanchalai Tanglertsampan · commit aea551c3b874 · 2017-02-02T15:00:35.000-08:00
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -11398,7 +11398,7 @@ namespace ts {
             // When we trying to resolve JsxOpeningLikeElement as a stateless function element, we will already give its attributes a contextual type
             // which is a type of the parameter of the signature we are trying out.
             // If there is no contextual type (e.g. we are trying to resolve stateful component), get attributes type from resolving element's tagName
-            const attributesType = getContextualType(<Expression>attribute.parent) || getAttributesTypeFromJsxOpeningLikeElement(<JsxOpeningLikeElement>attribute.parent.parent);
+            const attributesType = getContextualType(<Expression>attribute.parent);
 
             if (isJsxAttribute(attribute)) {
                 if (!attributesType || isTypeAny(attributesType)) {
@@ -12071,42 +12071,6 @@ namespace ts {
             return createJsxAttributesTypeFromAttributesProperty(node.parent as JsxOpeningLikeElement, /*filter*/ undefined, contextualMapper);
         }
 
-        /**
-         * Check whether the given attributes of JSX opening-like element is assignable to the tagName attributes.
-         *      Get the attributes type of the opening-like element through resolving the tagName, "target attributes"
-         *      Check assignablity between given attributes property, "source attributes", and the "target attributes"
-         * @param openingLikeElement an opening-like JSX element to check its JSXAttributes
-         */
-        function checkJsxAttributesAssignableToTagNameAttributes(openingLikeElement: JsxOpeningLikeElement) {
-            // The function involves following steps:
-            //      1. Figure out expected attributes type by resolving tagName of the JSX opening-like element, targetAttributesType.
-            //         During these steps, we will try to resolve the tagName as intrinsic name, stateless function, stateful component (in the order)
-            //      2. Solved JSX attributes type given by users, sourceAttributesType, which is by resolving "attributes" property of the JSX opening-like element.
-            //      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.
-            const targetAttributesType = isJsxIntrinsicIdentifier(openingLikeElement.tagName) ?
-                getIntrinsicAttributesTypeFromJsxOpeningLikeElement(openingLikeElement) :
-                getCustomJsxElementAttributesType(openingLikeElement, /*shouldIncludeAllStatelessAttributesType*/ false);
-
-            // sourceAttributesType is a type of an attributes properties.
-            // i.e <div attr1={10} attr2="string" />
-            //     attr1 and attr2 are treated as JSXAttributes attached in the JsxOpeningLikeElement as "attributes".
-            const sourceAttributesType = createJsxAttributesTypeFromAttributesProperty(openingLikeElement,
-                attribute => {
-                    return isUnhyphenatedJsxName(attribute.name) || !!(getPropertyOfType(targetAttributesType, attribute.name));
-                });
-
-            // If the targetAttributesType is an emptyObjectType, indicating that there is no property named 'props' on this instance type.
-            // but there exists a sourceAttributesType, we need to explicitly give an error as normal assignability check allow excess properties and will pass.
-            if (targetAttributesType === emptyObjectType && (isTypeAny(sourceAttributesType) || (<ResolvedType>sourceAttributesType).properties.length > 0)) {
-                error(openingLikeElement, Diagnostics.JSX_element_class_does_not_support_attributes_because_it_does_not_have_a_0_property, getJsxElementPropertiesName());
-            }
-            else {
-                checkTypeAssignableTo(sourceAttributesType, targetAttributesType, openingLikeElement.attributes.properties.length > 0 ? openingLikeElement.attributes : openingLikeElement);
-            }
-        }
-
         function getJsxType(name: string) {
             let jsxType = jsxTypes.get(name);
             if (jsxType === undefined) {
@@ -12462,6 +12426,7 @@ namespace ts {
          * Get attributes type of the given custom opening-like JSX element.
          * This function is intended to be called from a caller that handles intrinsic JSX element already.
          * @param node a custom JSX opening-like element
+         * @param shouldIncludeAllStatelessAttributesType a boolean value used by language service to get all possible attributes type from an overload stateless function component
          */
         function getCustomJsxElementAttributesType(node: JsxOpeningLikeElement, shouldIncludeAllStatelessAttributesType: boolean): Type {
             const links = getNodeLinks(node);
@@ -12489,7 +12454,7 @@ namespace ts {
         }
 
         /**
-         * Get the attributes type which is the type that indicate which attributes are valid on the given JSXOpeningLikeElement.
+         * Get the attributes type, which indicates the attributes that are valid on the given JSXOpeningLikeElement. 
          * @param node a JSXOpeningLikeElement node
          * @return an attributes type of the given node
          */
@@ -12520,7 +12485,9 @@ namespace ts {
             return jsxElementClassType;
         }
 
-        // Returns all the properties of the Jsx.IntrinsicElements interface
+        /**
+         * Returns all the properties of the Jsx.IntrinsicElements interface
+         */
         function getJsxIntrinsicTagNames(): Symbol[] {
             const intrinsics = getJsxType(JsxNames.IntrinsicElements);
             return intrinsics ? getPropertiesOfType(intrinsics) : emptyArray;
@@ -12561,6 +12528,42 @@ namespace ts {
             checkJsxAttributesAssignableToTagNameAttributes(node);
         }
 
+         /**
+         * Check whether the given attributes of JSX opening-like element is assignable to the tagName attributes.
+         *      Get the attributes type of the opening-like element through resolving the tagName, "target attributes"
+         *      Check assignablity between given attributes property, "source attributes", and the "target attributes"
+         * @param openingLikeElement an opening-like JSX element to check its JSXAttributes
+         */
+        function checkJsxAttributesAssignableToTagNameAttributes(openingLikeElement: JsxOpeningLikeElement) {
+            // The function involves following steps:
+            //      1. Figure out expected attributes type by resolving tagName of the JSX opening-like element, targetAttributesType.
+            //         During these steps, we will try to resolve the tagName as intrinsic name, stateless function, stateful component (in the order)
+            //      2. Solved JSX attributes type given by users, sourceAttributesType, which is by resolving "attributes" property of the JSX opening-like element.
+            //      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.
+            const targetAttributesType = isJsxIntrinsicIdentifier(openingLikeElement.tagName) ?
+                getIntrinsicAttributesTypeFromJsxOpeningLikeElement(openingLikeElement) :
+                getCustomJsxElementAttributesType(openingLikeElement, /*shouldIncludeAllStatelessAttributesType*/ false);
+
+            // sourceAttributesType is a type of an attributes properties.
+            // i.e <div attr1={10} attr2="string" />
+            //     attr1 and attr2 are treated as JSXAttributes attached in the JsxOpeningLikeElement as "attributes".
+            const sourceAttributesType = createJsxAttributesTypeFromAttributesProperty(openingLikeElement,
+                attribute => {
+                    return isUnhyphenatedJsxName(attribute.name) || !!(getPropertyOfType(targetAttributesType, attribute.name));
+                });
+
+            // If the targetAttributesType is an emptyObjectType, indicating that there is no property named 'props' on this instance type.
+            // but there exists a sourceAttributesType, we need to explicitly give an error as normal assignability check allow excess properties and will pass.
+            if (targetAttributesType === emptyObjectType && (isTypeAny(sourceAttributesType) || (<ResolvedType>sourceAttributesType).properties.length > 0)) {
+                error(openingLikeElement, Diagnostics.JSX_element_class_does_not_support_attributes_because_it_does_not_have_a_0_property, getJsxElementPropertiesName());
+            }
+            else {
+                checkTypeAssignableTo(sourceAttributesType, targetAttributesType, openingLikeElement.attributes.properties.length > 0 ? openingLikeElement.attributes : openingLikeElement);
+            }
+        }
+
         function checkJsxExpression(node: JsxExpression, contextualMapper?: TypeMapper) {
             if (node.expression) {
                 const type = checkExpression(node.expression, contextualMapper);
@@ -13021,9 +13024,7 @@ namespace ts {
             let spreadArgIndex = -1;
 
             if (isJsxOpeningLikeElement(node)) {
-                // For JSX opening-like element, we will ignore regular arity check (which is what is done here).
-                // Instead, the arity check will be done in "checkApplicableSignatureForJsxOpeningLikeElement" as we are required to figure out
-                // all property inside the given attributes.
+                // The arity check will be done in "checkApplicableSignatureForJsxOpeningLikeElement".
                 return true;
             }
 
@@ -13250,14 +13251,14 @@ namespace ts {
             // However "context" and "updater" are implicit and can't be specify by users. Only the first parameter, props,
             // can be specified by users through attributes property.
             const paramType = getTypeAtPosition(signature, 0);
-            const argType = checkExpressionWithContextualType(node.attributes, paramType, /*contextualMapper*/ undefined);
-            const argProperties = getPropertiesOfType(argType);
+            const attributesType = checkExpressionWithContextualType(node.attributes, paramType, /*contextualMapper*/ undefined);
+            const argProperties = getPropertiesOfType(attributesType);
             for (const arg of argProperties) {
                 if (!getPropertyOfType(paramType, arg.name) && isUnhyphenatedJsxName(arg.name)) {
                     return false;
                 }
             }
-            if (checkTypeRelatedTo(argType, paramType, relation, /*errorNode*/ undefined, headMessage)) {
+            if (checkTypeRelatedTo(attributesType, paramType, relation, /*errorNode*/ undefined, headMessage)) {
                 return true;
             }
             return false;
@@ -13744,16 +13745,15 @@ namespace ts {
             // If candidate is undefined, it means that no candidates had a suitable arity. In that case,
             // skip the checkApplicableSignature check.
             if (candidateForArgumentError) {
+                if (isJsxOpeningOrSelfClosingElement) {
+                    // We do not report any error here because any error will be handled in "resolveCustomJsxElementAttributesType".
+                    return candidateForArgumentError;
+                }
                 // excludeArgument is undefined, in this case also equivalent to [undefined, undefined, ...]
                 // The importance of excludeArgument is to prevent us from typing function expression parameters
                 // in arguments too early. If possible, we'd like to only type them once we know the correct
                 // overload. However, this matters for the case where the call is correct. When the call is
                 // an error, we don't need to exclude any arguments, although it would cause no harm to do so.
-                if (isJsxOpeningOrSelfClosingElement) {
-                    // If there is not result, just return the last one we try as a candidate.
-                    // We do not report any error here because any error will be handled in "resolveCustomJsxElementAttributesType".
-                    return candidateForArgumentError;
-                }
                 checkApplicableSignature(node, args, candidateForArgumentError, assignableRelation, /*excludeArgument*/ undefined, /*reportErrors*/ true);
             }
             else if (candidateForTypeArgumentError) {
diff --git a/src/services/goToDefinition.ts b/src/services/goToDefinition.ts
@@ -86,15 +86,13 @@ namespace ts.GoToDefinition {
         }
 
         if (isJsxOpeningLikeElement(node.parent)) {
-            // For JSX opening-like element, the tag can be resolved either as stateful component (e.g class) or stateless function component.
-            // Because if it is a stateless function component with an error while trying to resolve the signature, we don't want to return all
-            // possible overloads but just the first one.
+            // If there are errors when trying to figure out stateless component function, just return the first declaration
             // For example:
-            //      /*firstSource*/declare function MainButton(buttonProps: ButtonProps): JSX.Element;
-            //      /*secondSource*/declare function MainButton(linkProps: LinkProps): JSX.Element;
-            //      /*thirdSource*/declare function MainButton(props: ButtonProps | LinkProps): JSX.Element;
-            //      let opt = <Main/*firstTarget*/Button />;  // We get undefined for resolved signature indicating an error, then just return the first declaration
-            const {symbolName, symbolKind, containerName}  = getSymbolInfo(typeChecker, symbol, node);
+            //      declare function /*firstSource*/iMainButton(buttonProps: ButtonProps): JSX.Element;
+            //      declare function /*secondSource*/MainButton(linkProps: LinkProps): JSX.Element;
+            //      declare function /*thirdSource*/MainButton(props: ButtonProps | LinkProps): JSX.Element;
+            //      let opt = <Main/*firstTarget*/Button />;  // Error - We get undefined for resolved signature indicating an error, then just return the first declaration
+            const {symbolName, symbolKind, containerName} = getSymbolInfo(typeChecker, symbol, node);
             return [createDefinitionInfo(symbol.valueDeclaration, symbolKind, symbolName, containerName)];
         }
 
diff --git a/src/services/symbolDisplay.ts b/src/services/symbolDisplay.ts
@@ -136,13 +136,7 @@ namespace ts.SymbolDisplay {
                         signature = candidateSignatures[0];
                     }
 
-                    let useConstructSignatures = false;
-                    if (callExpressionLike.kind === SyntaxKind.NewExpression) {
-                        useConstructSignatures = true;
-                    }
-                    else if (isCallExpression(callExpressionLike) && callExpressionLike.expression.kind === SyntaxKind.SuperKeyword) {
-                        useConstructSignatures = true;
-                    }
+                    const useConstructSignatures = callExpressionLike.kind === SyntaxKind.NewExpression || (isCallExpression(callExpressionLike) && callExpressionLike.expression.kind === SyntaxKind.SuperKeyword);
 
                     const allSignatures = useConstructSignatures ? type.getConstructSignatures() : type.getCallSignatures();
 
