Merge pull request microsoft#31480 from andrewbranch/bug/25487

andrewbranch · web-flow · commit 871bdeeec405 · 2019-06-26T10:13:42.000-07:00
Fix invalid JSXExpressions having identifier-ish things in their trivia, improve error messages for comma expressions in JSX
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -20033,6 +20033,7 @@ namespace ts {
         }
 
         function checkJsxExpression(node: JsxExpression, checkMode?: CheckMode) {
+            checkGrammarJsxExpression(node);
             if (node.expression) {
                 const type = checkExpression(node.expression, checkMode);
                 if (node.dotDotDotToken && type !== anyType && !isArrayType(type)) {
@@ -31912,6 +31913,12 @@ namespace ts {
             }
         }
 
+        function checkGrammarJsxExpression(node: JsxExpression) {
+            if (node.expression && isCommaSequence(node.expression)) {
+                return grammarErrorOnNode(node.expression, Diagnostics.JSX_expressions_may_not_use_the_comma_operator_Did_you_mean_to_write_an_array);
+            }
+        }
+
         function checkGrammarForInOrForOfStatement(forInOrOfStatement: ForInOrOfStatement): boolean {
             if (checkGrammarStatementInAmbientContext(forInOrOfStatement)) {
                 return true;
diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json
@@ -5009,5 +5009,9 @@
     "Classes may not have a field named 'constructor'.": {
         "category": "Error",
         "code": 18006
+    },
+    "JSX expressions may not use the comma operator. Did you mean to write an array?": {
+        "category": "Error",
+        "code": 18007
     }
 }
diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts
@@ -4430,14 +4430,18 @@ namespace ts {
 
             if (token() !== SyntaxKind.CloseBraceToken) {
                 node.dotDotDotToken = parseOptionalToken(SyntaxKind.DotDotDotToken);
-                node.expression = parseAssignmentExpressionOrHigher();
+                // Only an AssignmentExpression is valid here per the JSX spec,
+                // but we can unambiguously parse a comma sequence and provide
+                // a better error message in grammar checking.
+                node.expression = parseExpression();
             }
             if (inExpressionContext) {
                 parseExpected(SyntaxKind.CloseBraceToken);
             }
             else {
-                parseExpected(SyntaxKind.CloseBraceToken, /*message*/ undefined, /*shouldAdvance*/ false);
-                scanJsxText();
+                if (parseExpected(SyntaxKind.CloseBraceToken, /*message*/ undefined, /*shouldAdvance*/ false)) {
+                    scanJsxText();
+                }
             }
 
             return finishNode(node);
diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts
@@ -583,6 +583,21 @@ namespace FourSlash {
             });
         }
 
+        public verifyErrorExistsAtRange(range: Range, code: number, expectedMessage?: string) {
+            const span = ts.createTextSpanFromRange(range);
+            const hasMatchingError = ts.some(
+                this.getDiagnostics(range.fileName),
+                ({ code, messageText, start, length }) =>
+                    code === code &&
+                    (!expectedMessage || expectedMessage === messageText) &&
+                    ts.isNumber(start) && ts.isNumber(length) &&
+                    ts.textSpansEqual(span, { start, length }));
+
+            if (!hasMatchingError) {
+                this.raiseError(`No error with code ${code} found at provided range.`);
+            }
+        }
+
         public verifyNumberOfErrorsInCurrentFile(expected: number) {
             const errors = this.getDiagnostics(this.activeFile.fileName);
             const actual = errors.length;
@@ -4009,6 +4024,10 @@ namespace FourSlashInterface {
             this.state.verifyNoErrors();
         }
 
+        public errorExistsAtRange(range: FourSlash.Range, code: number, message?: string) {
+            this.state.verifyErrorExistsAtRange(range, code, message);
+        }
+
         public numberOfErrorsInCurrentFile(expected: number) {
             this.state.verifyNumberOfErrorsInCurrentFile(expected);
         }
diff --git a/tests/baselines/reference/jsxAndTypeAssertion.js b/tests/baselines/reference/jsxAndTypeAssertion.js
@@ -28,18 +28,21 @@ var foo = /** @class */ (function () {
     return foo;
 }());
 var x;
-x = <any> {test} <any></any> };
+x = <any> {test}: <any></any> };
 
 x = <any><any></any>;
  
-x = <foo>hello {<foo>} </foo>}
+x = <foo>hello {<foo>} </foo>};
 
 x = <foo test={<foo>}>hello</foo>}/>
 
-x = <foo test={<foo>}>hello{<foo>}</foo>}
+x = <foo test={<foo>}>hello{<foo>}</foo>};
 
 x = <foo>x</foo>, x = <foo />;
 
 <foo>{<foo><foo>{/foo/.test(x) ? <foo><foo></foo> : <foo><foo></foo>}</foo>}</foo>
     :
-}</></>}</></>}/></></></>;
+}
+
+    
+</></>}</></>}/></></></>;
diff --git a/tests/baselines/reference/jsxInvalidEsprimaTestSuite.js b/tests/baselines/reference/jsxInvalidEsprimaTestSuite.js
@@ -123,7 +123,7 @@ var x = <div>one</div>, <div>two</div>;
 var x = <div>one</div> /* intervening comment */, /* intervening comment */ <div>two</div>;
 ;
 //// [20.jsx]
-<a>{"str"}}</a>;
+<a>{"str"};}</a>;
 //// [21.jsx]
 <span className="a" id="b"/>;
 //// [22.jsx]
diff --git a/tests/baselines/reference/jsxParsingError1.errors.txt b/tests/baselines/reference/jsxParsingError1.errors.txt
@@ -1,9 +1,8 @@
-tests/cases/conformance/jsx/file.tsx(11,36): error TS1005: '}' expected.
-tests/cases/conformance/jsx/file.tsx(11,44): error TS1003: Identifier expected.
-tests/cases/conformance/jsx/file.tsx(11,46): error TS1161: Unterminated regular expression literal.
+tests/cases/conformance/jsx/file.tsx(11,30): error TS2695: Left side of comma operator is unused and has no side effects.
+tests/cases/conformance/jsx/file.tsx(11,30): error TS18007: JSX expressions may not use the comma operator. Did you mean to write an array?
 
 
-==== tests/cases/conformance/jsx/file.tsx (3 errors) ====
+==== tests/cases/conformance/jsx/file.tsx (2 errors) ====
     declare module JSX {
     	interface Element { }
     	interface IntrinsicElements {
@@ -15,10 +14,8 @@ tests/cases/conformance/jsx/file.tsx(11,46): error TS1161: Unterminated regular
     const class1 = "foo";
     const class2 = "bar";
     const elem = <div className={class1, class2}/>;
-                                       ~
-!!! error TS1005: '}' expected.
-                                               ~
-!!! error TS1003: Identifier expected.
-                                                 
-!!! error TS1161: Unterminated regular expression literal.
+                                 ~~~~~~
+!!! error TS2695: Left side of comma operator is unused and has no side effects.
+                                 ~~~~~~~~~~~~~~
+!!! error TS18007: JSX expressions may not use the comma operator. Did you mean to write an array?
     
diff --git a/tests/baselines/reference/jsxParsingError1.js b/tests/baselines/reference/jsxParsingError1.js
@@ -16,5 +16,4 @@ const elem = <div className={class1, class2}/>;
 // This should be a parse error
 var class1 = "foo";
 var class2 = "bar";
-var elem = <div className={class1} class2/>;
-/>;;
+var elem = <div className={class1, class2}/>;
diff --git a/tests/baselines/reference/jsxParsingError1.symbols b/tests/baselines/reference/jsxParsingError1.symbols
@@ -25,5 +25,5 @@ const elem = <div className={class1, class2}/>;
 >div : Symbol(JSX.IntrinsicElements, Decl(file.tsx, 1, 22))
 >className : Symbol(className, Decl(file.tsx, 10, 17))
 >class1 : Symbol(class1, Decl(file.tsx, 8, 5))
->class2 : Symbol(class2, Decl(file.tsx, 10, 36))
+>class2 : Symbol(class2, Decl(file.tsx, 9, 5))
 
diff --git a/tests/baselines/reference/jsxParsingError1.types b/tests/baselines/reference/jsxParsingError1.types
@@ -18,10 +18,10 @@ const class2 = "bar";
 
 const elem = <div className={class1, class2}/>;
 >elem : JSX.Element
-><div className={class1, class2 : JSX.Element
+><div className={class1, class2}/> : JSX.Element
 >div : any
 >className : string
+>class1, class2 : "bar"
 >class1 : "foo"
->class2 : true
->/>; : RegExp
+>class2 : "bar"
 
diff --git a/tests/baselines/reference/tsxErrorRecovery1.errors.txt b/tests/baselines/reference/tsxErrorRecovery1.errors.txt
@@ -1,26 +1,14 @@
-tests/cases/conformance/jsx/file.tsx(4,11): error TS17008: JSX element 'div' has no corresponding closing tag.
 tests/cases/conformance/jsx/file.tsx(4,19): error TS1109: Expression expected.
-tests/cases/conformance/jsx/file.tsx(7,11): error TS2304: Cannot find name 'a'.
-tests/cases/conformance/jsx/file.tsx(7,12): error TS1005: '}' expected.
-tests/cases/conformance/jsx/file.tsx(8,1): error TS1005: '</' expected.
 
 
-==== tests/cases/conformance/jsx/file.tsx (5 errors) ====
+==== tests/cases/conformance/jsx/file.tsx (1 errors) ====
     declare namespace JSX { interface Element { } }
     
     function foo() {
     	var x = <div>  { </div>
-    	         ~~~
-!!! error TS17008: JSX element 'div' has no corresponding closing tag.
     	                 ~~
 !!! error TS1109: Expression expected.
     }
     // Shouldn't see any errors down here
     var y = { a: 1 };
-              ~
-!!! error TS2304: Cannot find name 'a'.
-               ~
-!!! error TS1005: '}' expected.
-    
-    
-!!! error TS1005: '</' expected.
+    
diff --git a/tests/baselines/reference/tsxErrorRecovery1.js b/tests/baselines/reference/tsxErrorRecovery1.js
@@ -10,9 +10,7 @@ var y = { a: 1 };
 
 //// [file.jsx]
 function foo() {
-    var x = <div>  {}div>
-}
-// Shouldn't see any errors down here
-var y = {a} 1 };
-    </>;
+    var x = <div>  {} </div>;
 }
+// Shouldn't see any errors down here
+var y = { a: 1 };
diff --git a/tests/baselines/reference/tsxErrorRecovery1.symbols b/tests/baselines/reference/tsxErrorRecovery1.symbols
@@ -11,4 +11,6 @@ function foo() {
 }
 // Shouldn't see any errors down here
 var y = { a: 1 };
+>y : Symbol(y, Decl(file.tsx, 6, 3))
+>a : Symbol(a, Decl(file.tsx, 6, 9))
 
diff --git a/tests/baselines/reference/tsxErrorRecovery1.types b/tests/baselines/reference/tsxErrorRecovery1.types
@@ -6,13 +6,15 @@ function foo() {
 
 	var x = <div>  { </div>
 >x : JSX.Element
-><div>  { </div>}// Shouldn't see any errors down herevar y = { a: 1 }; : JSX.Element
+><div>  { </div> : JSX.Element
 >div : any
 > : any
+>div : any
 }
 // Shouldn't see any errors down here
 var y = { a: 1 };
->a : any
-
-> : any
+>y : { a: number; }
+>{ a: 1 } : { a: number; }
+>a : number
+>1 : 1
 
diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts
@@ -42,6 +42,8 @@
 //
 // TODO: figure out a better solution to the API exposure problem.
 
+/// <reference path="../../../src/compiler/diagnosticInformationMap.generated.ts" />
+
 declare module ts {
     export type MapKey = string | number;
     export interface Map<T> {
@@ -70,6 +72,21 @@ declare module ts {
         text: string;
     }
 
+    enum DiagnosticCategory {
+        Warning,
+        Error,
+        Suggestion,
+        Message
+    }
+
+    interface DiagnosticMessage {
+        key: string;
+        category: DiagnosticCategory;
+        code: number;
+        message: string;
+        reportsUnnecessary?: {};
+    }
+
     function flatMap<T, U>(array: ReadonlyArray<T>, mapfn: (x: T, i: number) => U | ReadonlyArray<U> | undefined): U[];
 }
 
@@ -238,6 +255,7 @@ declare namespace FourSlashInterface {
         signatureHelp(...options: VerifySignatureHelpOptions[], ): void;
         // Checks that there are no compile errors.
         noErrors(): void;
+        errorExistsAtRange(range: Range, code: number, message?: string): void;
         numberOfErrorsInCurrentFile(expected: number): void;
         baselineCurrentFileBreakpointLocations(): void;
         baselineCurrentFileNameOrDottedNameSpans(): void;
diff --git a/tests/cases/fourslash/jsxExpressionFollowedByIdentifier.ts b/tests/cases/fourslash/jsxExpressionFollowedByIdentifier.ts
@@ -0,0 +1,12 @@
+/// <reference path="fourslash.ts" />
+
+//@Filename: jsxExpressionFollowedByIdentifier.tsx
+////declare var React: any;
+////const a = <div>{<div />[|x|]}</div>
+////const b = <div x={<div />[|x|]} />
+
+test.ranges().forEach(range => {
+  verify.errorExistsAtRange(range, ts.Diagnostics._0_expected.code, "'}' expected.");
+  // This is just to ensure getting quick info doesn’t crash
+  verify.not.quickInfoExists();
+});
diff --git a/tests/cases/fourslash/jsxExpressionWithCommaExpression.ts b/tests/cases/fourslash/jsxExpressionWithCommaExpression.ts
@@ -0,0 +1,11 @@
+/// <reference path="fourslash.ts" />
+
+//@Filename: jsxExpressionWithCommaExpression.tsx
+//@jsx: react
+////declare var React: any;
+////declare var x: string;
+////const a = <div x={[|x, x|]} />
+////const b = <div>{[|x, x|]}</div>
+
+verify.getSyntacticDiagnostics([]);
+test.ranges().forEach(range => verify.errorExistsAtRange(range, 18006));

Original file line number	Diff line number	Diff line change
`@@ -20033,6 +20033,7 @@ namespace ts {`
`20033`	`20033`	`}`
`20034`	`20034`
`20035`	`20035`	`function checkJsxExpression(node: JsxExpression, checkMode?: CheckMode) {`
	`20036`	`+ checkGrammarJsxExpression(node);`
`20036`	`20037`	`if (node.expression) {`
`20037`	`20038`	`const type = checkExpression(node.expression, checkMode);`
`20038`	`20039`	`if (node.dotDotDotToken && type !== anyType && !isArrayType(type)) {`
`@@ -31912,6 +31913,12 @@ namespace ts {`
`31912`	`31913`	`}`
`31913`	`31914`	`}`
`31914`	`31915`
	`31916`	`+ function checkGrammarJsxExpression(node: JsxExpression) {`
	`31917`	`+ if (node.expression && isCommaSequence(node.expression)) {`
	`31918`	`+ return grammarErrorOnNode(node.expression, Diagnostics.JSX_expressions_may_not_use_the_comma_operator_Did_you_mean_to_write_an_array);`
	`31919`	`+ }`
	`31920`	`+ }`
	`31921`	`+`
`31915`	`31922`	`function checkGrammarForInOrForOfStatement(forInOrOfStatement: ForInOrOfStatement): boolean {`
`31916`	`31923`	`if (checkGrammarStatementInAmbientContext(forInOrOfStatement)) {`
`31917`	`31924`	`return true;`
Original file line number	Diff line number	Diff line change
`@@ -5009,5 +5009,9 @@`
`5009`	`5009`	`"Classes may not have a field named 'constructor'.": {`
`5010`	`5010`	`"category": "Error",`
`5011`	`5011`	`"code": 18006`
	`5012`	`+ },`
	`5013`	`+ "JSX expressions may not use the comma operator. Did you mean to write an array?": {`
	`5014`	`+ "category": "Error",`
	`5015`	`+ "code": 18007`
`5012`	`5016`	`}`
`5013`	`5017`	`}`
Original file line number	Diff line number	Diff line change
`@@ -123,7 +123,7 @@ var x = <div>one</div>, <div>two</div>;`
`123`	`123`	`var x = <div>one</div> /* intervening comment /, / intervening comment */ <div>two</div>;`
`124`	`124`	`;`
`125`	`125`	`//// [20.jsx]`
`126`		`-<a>{"str"}}</a>;`
	`126`	`+<a>{"str"};}</a>;`
`127`	`127`	`//// [21.jsx]`
`128`	`128`	`<span className="a" id="b"/>;`
`129`	`129`	`//// [22.jsx]`