Skip to content

Commit

Permalink
PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@weswigham
weswigham committed Feb 6, 2020
1 parent ff7532c commit 3af84d4
Show file tree
Hide file tree
Showing 8 changed files with 169 additions and 67 deletions.
41 changes: 25 additions & 16 deletions src/compiler/checker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24309,10 +24309,7 @@ namespace ts {
// can be specified by users through attributes property.
const paramType = getEffectiveFirstArgumentForJsxSignature(signature, node);
const attributesType = checkExpressionWithContextualType(node.attributes, paramType, /*inferenceContext*/ undefined, checkMode);
if (!checkTagNameDoesNotExpectTooManyArguments()) {
return false;
}
return checkTypeRelatedToAndOptionallyElaborate(
return checkTagNameDoesNotExpectTooManyArguments() && checkTypeRelatedToAndOptionallyElaborate(
attributesType,
paramType,
relation,
Expand Down Expand Up @@ -24346,34 +24343,46 @@ namespace ts {
return true;
}

let hasFirstparamSignatures = false;
// Check that _some_ first parameter expects and SFC-like thing, and that some overload of the SFC expects an acceptable number of arguments
let hasFirstParamSignatures = false;
let maxParamCount = 0;
// Check that _some_ first parameter expects a FC-like thing, and that some overload of the SFC expects an acceptable number of arguments
for (const sig of callSignatures) {
const firstparam = getTypeAtPosition(sig, 0);
const signaturesOfParam = getSignaturesOfType(firstparam, SignatureKind.Call);
if (!length(signaturesOfParam)) continue;
for (const paramSig of signaturesOfParam) {
hasFirstparamSignatures = true;
hasFirstParamSignatures = true;
if (hasEffectiveRestParameter(paramSig)) {
return true; // some signature has a rest param, so function components can have an aritrary number of arguments
return true; // some signature has a rest param, so function components can have an arbitrary number of arguments
}
const paramCount = getParameterCount(paramSig);
for (const tagSig of tagCallSignatures) {
const tagParamCount = getParameterCount(tagSig);
if (tagParamCount <= paramCount) {
return true; // some signature accepts the number of arguments the function component provides
}
if (paramCount > maxParamCount) {
maxParamCount = paramCount;
}
}
}
if (!hasFirstparamSignatures) {
if (!hasFirstParamSignatures) {
// Not a single signature had a first parameter which expected a signature - for back compat, and
// to guard against generic factories which won't have signatures directly, return true
// to guard against generic factories which won't have signatures directly, do not error
return true;
}
let absoluteMinArgCount = Infinity;
for (const tagSig of tagCallSignatures) {
const tagRequiredArgCount = getMinArgumentCount(tagSig);
if (tagRequiredArgCount < absoluteMinArgCount) {
absoluteMinArgCount = tagRequiredArgCount;
}
}
if (absoluteMinArgCount <= maxParamCount) {
return true; // some signature accepts the number of arguments the function component provides
}

if (reportErrors) {
const diag = createDiagnosticForNode(node.tagName, Diagnostics.Function_like_tag_expects_more_arguments_than_the_JSX_factory_can_provide);
const diag = createDiagnosticForNode(node.tagName, Diagnostics.Tag_0_expects_at_least_1_arguments_but_the_JSX_factory_2_provides_at_most_3, entityNameToString(node.tagName), absoluteMinArgCount, entityNameToString(factory), maxParamCount);
const tagNameDeclaration = getSymbolAtLocation(node.tagName)?.valueDeclaration;
if (tagNameDeclaration) {
addRelatedInfo(diag, createDiagnosticForNode(tagNameDeclaration, Diagnostics._0_is_declared_here, entityNameToString(node.tagName)));
}
if (errorOutputContainer && errorOutputContainer.skipLogging) {
(errorOutputContainer.errors || (errorOutputContainer.errors = [])).push(diag);
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/diagnosticMessages.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4280,7 +4280,7 @@
"category": "Message",
"code": 6228
},
"Function-like tag expects more arguments than the JSX factory can provide.": {
"Tag '{0}' expects at least '{1}' arguments, but the JSX factory '{2}' provides at most '{3}'.": {
"category": "Error",
"code": 6229
},
Expand Down
7 changes: 5 additions & 2 deletions src/compiler/utilities.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -864,14 +864,17 @@ namespace ts {
}
}

export function entityNameToString(name: EntityNameOrEntityNameExpression): string {
export function entityNameToString(name: EntityNameOrEntityNameExpression | JsxTagNameExpression | PrivateIdentifier): string {
switch (name.kind) {
case SyntaxKind.ThisKeyword:
return "this";
case SyntaxKind.PrivateIdentifier:
case SyntaxKind.Identifier:
return getFullWidth(name) === 0 ? idText(name) : getTextOfNode(name);
case SyntaxKind.QualifiedName:
return entityNameToString(name.left) + "." + entityNameToString(name.right);
case SyntaxKind.PropertyAccessExpression:
if (isIdentifier(name.name)) {
if (isIdentifier(name.name) || isPrivateIdentifier(name.name)) {
return entityNameToString(name.expression) + "." + entityNameToString(name.name);
}
else {
Expand Down
29 changes: 20 additions & 9 deletions tests/baselines/reference/jsxIssuesErrorWhenTagExpectsTooManyArguments.errors.txt
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
tests/cases/compiler/jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx(13,12): error TS6229: Function-like tag expects more arguments than the JSX factory can provide.
tests/cases/compiler/jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx(19,12): error TS6229: Tag 'MyComp4' expects at least '4' arguments, but the JSX factory 'React.createElement' provides at most '2'.
tests/cases/compiler/jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx(20,12): error TS6229: Tag 'MyComp3' expects at least '3' arguments, but the JSX factory 'React.createElement' provides at most '2'.


==== tests/cases/compiler/jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx (1 errors) ====
==== tests/cases/compiler/jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx (2 errors) ====
/// <reference path="/.lib/react16.d.ts" />

import * as React from "react";
Expand All @@ -10,15 +11,25 @@ tests/cases/compiler/jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx(13,12): er
x: number;
}

function MyComp(props: MyProps, context: any, bad: any, verybad: any) {
function MyComp4(props: MyProps, context: any, bad: any, verybad: any) {
return <div></div>;
}
function MyComp3(props: MyProps, context: any, bad: any) {
return <div></div>;
}

const a = <MyComp x={2}/>; // using `MyComp` as a component should error - it expects more arguments than react provides
~~~~~~
!!! error TS6229: Function-like tag expects more arguments than the JSX factory can provide.

function MyComp2(props: MyProps, context: any) {
return <div></div>
}
const b = <MyComp2 x={2}/>; // Should be OK, `context` is allowed, per react rules

const a = <MyComp4 x={2}/>; // using `MyComp` as a component should error - it expects more arguments than react provides
~~~~~~~
!!! error TS6229: Tag 'MyComp4' expects at least '4' arguments, but the JSX factory 'React.createElement' provides at most '2'.
!!! related TS2728 tests/cases/compiler/jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx:9:10: 'MyComp4' is declared here.
const b = <MyComp3 x={2}/>; // using `MyComp` as a component should error - it expects more arguments than react provides
~~~~~~~
!!! error TS6229: Tag 'MyComp3' expects at least '3' arguments, but the JSX factory 'React.createElement' provides at most '2'.
!!! related TS2728 tests/cases/compiler/jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx:12:10: 'MyComp3' is declared here.
const c = <MyComp2 x={2}/>; // Should be OK, `context` is allowed, per react rules

declare function MyTagWithOptionalNonJSXBits(props: MyProps, context: any, nonReactArg?: string): JSX.Element;
const d = <MyTagWithOptionalNonJSXBits x={2} />; // Technically OK, but probably questionable
27 changes: 19 additions & 8 deletions tests/baselines/reference/jsxIssuesErrorWhenTagExpectsTooManyArguments.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,27 +7,38 @@ interface MyProps {
x: number;
}

function MyComp(props: MyProps, context: any, bad: any, verybad: any) {
function MyComp4(props: MyProps, context: any, bad: any, verybad: any) {
return <div></div>;
}
function MyComp3(props: MyProps, context: any, bad: any) {
return <div></div>;
}

const a = <MyComp x={2}/>; // using `MyComp` as a component should error - it expects more arguments than react provides

function MyComp2(props: MyProps, context: any) {
return <div></div>
}
const b = <MyComp2 x={2}/>; // Should be OK, `context` is allowed, per react rules

const a = <MyComp4 x={2}/>; // using `MyComp` as a component should error - it expects more arguments than react provides
const b = <MyComp3 x={2}/>; // using `MyComp` as a component should error - it expects more arguments than react provides
const c = <MyComp2 x={2}/>; // Should be OK, `context` is allowed, per react rules
declare function MyTagWithOptionalNonJSXBits(props: MyProps, context: any, nonReactArg?: string): JSX.Element;
const d = <MyTagWithOptionalNonJSXBits x={2} />; // Technically OK, but probably questionable
//// [jsxIssuesErrorWhenTagExpectsTooManyArguments.js]
"use strict";
/// <reference path="react16.d.ts" />
exports.__esModule = true;
var React = require("react");
function MyComp(props, context, bad, verybad) {
function MyComp4(props, context, bad, verybad) {
return React.createElement("div", null);
}
function MyComp3(props, context, bad) {
return React.createElement("div", null);
}
var a = React.createElement(MyComp, { x: 2 }); // using `MyComp` as a component should error - it expects more arguments than react provides
function MyComp2(props, context) {
return React.createElement("div", null);
}
var b = React.createElement(MyComp2, { x: 2 }); // Should be OK, `context` is allowed, per react rules
var a = React.createElement(MyComp4, { x: 2 }); // using `MyComp` as a component should error - it expects more arguments than react provides
var b = React.createElement(MyComp3, { x: 2 }); // using `MyComp` as a component should error - it expects more arguments than react provides
var c = React.createElement(MyComp2, { x: 2 }); // Should be OK, `context` is allowed, per react rules
var d = React.createElement(MyTagWithOptionalNonJSXBits, { x: 2 }); // Technically OK, but probably questionable
62 changes: 46 additions & 16 deletions tests/baselines/reference/jsxIssuesErrorWhenTagExpectsTooManyArguments.symbols
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,31 @@ interface MyProps {
>x : Symbol(MyProps.x, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 4, 19))
}

function MyComp(props: MyProps, context: any, bad: any, verybad: any) {
>MyComp : Symbol(MyComp, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 6, 1))
>props : Symbol(props, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 8, 16))
function MyComp4(props: MyProps, context: any, bad: any, verybad: any) {
>MyComp4 : Symbol(MyComp4, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 6, 1))
>props : Symbol(props, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 8, 17))
>MyProps : Symbol(MyProps, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 2, 31))
>context : Symbol(context, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 8, 31))
>bad : Symbol(bad, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 8, 45))
>verybad : Symbol(verybad, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 8, 55))
>context : Symbol(context, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 8, 32))
>bad : Symbol(bad, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 8, 46))
>verybad : Symbol(verybad, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 8, 56))

return <div></div>;
>div : Symbol(JSX.IntrinsicElements.div, Decl(react16.d.ts, 2420, 114))
>div : Symbol(JSX.IntrinsicElements.div, Decl(react16.d.ts, 2420, 114))
}
function MyComp3(props: MyProps, context: any, bad: any) {
>MyComp3 : Symbol(MyComp3, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 10, 1))
>props : Symbol(props, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 11, 17))
>MyProps : Symbol(MyProps, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 2, 31))
>context : Symbol(context, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 11, 32))
>bad : Symbol(bad, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 11, 46))

const a = <MyComp x={2}/>; // using `MyComp` as a component should error - it expects more arguments than react provides
>a : Symbol(a, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 12, 5))
>MyComp : Symbol(MyComp, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 6, 1))
>x : Symbol(x, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 12, 17))

return <div></div>;
>div : Symbol(JSX.IntrinsicElements.div, Decl(react16.d.ts, 2420, 114))
>div : Symbol(JSX.IntrinsicElements.div, Decl(react16.d.ts, 2420, 114))
}
function MyComp2(props: MyProps, context: any) {
>MyComp2 : Symbol(MyComp2, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 12, 26))
>MyComp2 : Symbol(MyComp2, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 13, 1))
>props : Symbol(props, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 14, 17))
>MyProps : Symbol(MyProps, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 2, 31))
>context : Symbol(context, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 14, 32))
Expand All @@ -39,8 +44,33 @@ function MyComp2(props: MyProps, context: any) {
>div : Symbol(JSX.IntrinsicElements.div, Decl(react16.d.ts, 2420, 114))
>div : Symbol(JSX.IntrinsicElements.div, Decl(react16.d.ts, 2420, 114))
}
const b = <MyComp2 x={2}/>; // Should be OK, `context` is allowed, per react rules
>b : Symbol(b, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 17, 5))
>MyComp2 : Symbol(MyComp2, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 12, 26))
>x : Symbol(x, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 17, 19))

const a = <MyComp4 x={2}/>; // using `MyComp` as a component should error - it expects more arguments than react provides
>a : Symbol(a, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 18, 5))
>MyComp4 : Symbol(MyComp4, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 6, 1))
>x : Symbol(x, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 18, 18))

const b = <MyComp3 x={2}/>; // using `MyComp` as a component should error - it expects more arguments than react provides
>b : Symbol(b, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 19, 5))
>MyComp3 : Symbol(MyComp3, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 10, 1))
>x : Symbol(x, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 19, 18))

const c = <MyComp2 x={2}/>; // Should be OK, `context` is allowed, per react rules
>c : Symbol(c, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 20, 5))
>MyComp2 : Symbol(MyComp2, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 13, 1))
>x : Symbol(x, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 20, 19))

declare function MyTagWithOptionalNonJSXBits(props: MyProps, context: any, nonReactArg?: string): JSX.Element;
>MyTagWithOptionalNonJSXBits : Symbol(MyTagWithOptionalNonJSXBits, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 20, 28))
>props : Symbol(props, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 22, 45))
>MyProps : Symbol(MyProps, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 2, 31))
>context : Symbol(context, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 22, 60))
>nonReactArg : Symbol(nonReactArg, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 22, 74))
>JSX : Symbol(JSX, Decl(react16.d.ts, 2367, 12))
>Element : Symbol(JSX.Element, Decl(react16.d.ts, 2368, 23))

const d = <MyTagWithOptionalNonJSXBits x={2} />; // Technically OK, but probably questionable
>d : Symbol(d, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 23, 5))
>MyTagWithOptionalNonJSXBits : Symbol(MyTagWithOptionalNonJSXBits, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 20, 28))
>x : Symbol(x, Decl(jsxIssuesErrorWhenTagExpectsTooManyArguments.tsx, 23, 38))

52 changes: 42 additions & 10 deletions tests/baselines/reference/jsxIssuesErrorWhenTagExpectsTooManyArguments.types
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ interface MyProps {
>x : number
}

function MyComp(props: MyProps, context: any, bad: any, verybad: any) {
>MyComp : (props: MyProps, context: any, bad: any, verybad: any) => JSX.Element
function MyComp4(props: MyProps, context: any, bad: any, verybad: any) {
>MyComp4 : (props: MyProps, context: any, bad: any, verybad: any) => JSX.Element
>props : MyProps
>context : any
>bad : any
Expand All @@ -21,14 +21,17 @@ function MyComp(props: MyProps, context: any, bad: any, verybad: any) {
>div : any
>div : any
}
function MyComp3(props: MyProps, context: any, bad: any) {
>MyComp3 : (props: MyProps, context: any, bad: any) => JSX.Element
>props : MyProps
>context : any
>bad : any

const a = <MyComp x={2}/>; // using `MyComp` as a component should error - it expects more arguments than react provides
>a : JSX.Element
><MyComp x={2}/> : JSX.Element
>MyComp : (props: MyProps, context: any, bad: any, verybad: any) => JSX.Element
>x : number
>2 : 2

return <div></div>;
><div></div> : JSX.Element
>div : any
>div : any
}
function MyComp2(props: MyProps, context: any) {
>MyComp2 : (props: MyProps, context: any) => JSX.Element
>props : MyProps
Expand All @@ -39,10 +42,39 @@ function MyComp2(props: MyProps, context: any) {
>div : any
>div : any
}
const b = <MyComp2 x={2}/>; // Should be OK, `context` is allowed, per react rules

const a = <MyComp4 x={2}/>; // using `MyComp` as a component should error - it expects more arguments than react provides
>a : JSX.Element
><MyComp4 x={2}/> : JSX.Element
>MyComp4 : (props: MyProps, context: any, bad: any, verybad: any) => JSX.Element
>x : number
>2 : 2

const b = <MyComp3 x={2}/>; // using `MyComp` as a component should error - it expects more arguments than react provides
>b : JSX.Element
><MyComp3 x={2}/> : JSX.Element
>MyComp3 : (props: MyProps, context: any, bad: any) => JSX.Element
>x : number
>2 : 2

const c = <MyComp2 x={2}/>; // Should be OK, `context` is allowed, per react rules
>c : JSX.Element
><MyComp2 x={2}/> : JSX.Element
>MyComp2 : (props: MyProps, context: any) => JSX.Element
>x : number
>2 : 2

declare function MyTagWithOptionalNonJSXBits(props: MyProps, context: any, nonReactArg?: string): JSX.Element;
>MyTagWithOptionalNonJSXBits : (props: MyProps, context: any, nonReactArg?: string) => JSX.Element
>props : MyProps
>context : any
>nonReactArg : string
>JSX : any

const d = <MyTagWithOptionalNonJSXBits x={2} />; // Technically OK, but probably questionable
>d : JSX.Element
><MyTagWithOptionalNonJSXBits x={2} /> : JSX.Element
>MyTagWithOptionalNonJSXBits : (props: MyProps, context: any, nonReactArg?: string) => JSX.Element
>x : number
>2 : 2

Loading

0 comments on commit 3af84d4

Please sign in to comment.