Specialized parsing error for apparent JSX tag in .ts file #56101

JoshuaKGoldberg · 2023-10-13T18:28:53Z

Adds a specialized diagnostic for the case of a /> (slash and greater-than-sigh) after an identifier in a non-JSX-mode file.

JSX tags are not permitted in `.ts` files. Did you mean to change the file extension to `.tsx`?

I'm not at all confident in this PR's approach. It feels weird to work with a "previousNode" and then skip through tokens. Would love to know if there's a cleaner way of doing things, please!

This also doesn't capture every possible syntax variant that could appear to be from JSX syntax in a non-JSX file. I couldn't figure out any clean way to get more cases that came to mind.

Fixes #29375

RyanCavanaugh · 2023-10-13T21:46:19Z

A good fix might not exist. We really don't know something's syntactically invalid until way later; this code is legal (!):

declare const div: any, foo: any;
type div = {};
const a = <div>foo</div>/;

JoshuaKGoldberg · 2023-11-15T21:44:07Z

src/compiler/parser.ts

+        if (token() === SyntaxKind.SlashToken && scanner.scan() === SyntaxKind.GreaterThanToken) {
+            nextToken();
+            parseErrorAt(posLessThan - 1, getNodePos(), Diagnostics.JSX_tags_are_not_permitted_in_ts_files_Did_you_mean_to_change_the_file_extension_to_tsx, tokenToString(SyntaxKind.GreaterThanToken));
+        }


I'd originally had it return here, so it doesn't try to parseSimpleUnaryExpression. But I couldn't figure out how to make an empty identifier that obeyed node ordering rules. See failures from up till fd5adf7.

createMissingNode would probably work.

Edit: also, the stuff after a tag is JSX content anyway, right? What's the likelihood that it'll be text that the TS parser actually expects?

Hard to say. I would personally assume rather low given that most JSX element children are often one of:

More JSX

Plain text: will often be parsed mostly as identifiers and/or odd punctuation characters

{} blocks containing a function call or some other odd code - which is sometimes valid non-JSX

sandersn

There are parts I like and parts I dislike in this change. The change in parsePrimaryExpression is pretty invasive whereas the parseTypeAssertion is simple. How much of the benefit comes from the parseTypeAssertion change?

I also don't like the fact that old errors are still around, and that the new error spans the entire tag. I guess it doesn't really matter what the span is, since the important thing is for the "why not JSX?" message to be early in the list of error messages.

I almost wish we could add this message to an existing error that appears with JSX, only in cases where we can be sure that the cause really is a tag. Would that be a better approach? I'm not sure any of the existing errors have enough context to decide that they were really, truly caused by a JSX tag.

src/compiler/diagnosticMessages.json

sandersn · 2023-11-16T21:29:52Z

tests/baselines/reference/nonTsxExtensionOpeningClosingTagMultiline.errors.txt

+!!! error TS2304: Cannot find name 'yippee'.
+    </span>;
+    ~~~~~~~
+!!! error TS1465: JSX tags are not permitted in `.ts` files. Did you mean to change the file extension to `.tsx`?


is the span of this error the entire expression <span> ... </span> ?

It's the entire expression, yes.

sandersn · 2023-11-16T21:55:57Z

src/compiler/parser.ts

@@ -5698,7 +5699,7 @@ namespace Parser {
     *      1) UnaryExpression[?Yield]
     *      2) UpdateExpression[?Yield] ** ExponentiationExpression[?Yield]
     */
-    function parseUnaryExpressionOrHigher(): UnaryExpression | BinaryExpression {
+    function parseUnaryExpressionOrHigher(previousNode?: Node): UnaryExpression | BinaryExpression {


how much would you have to change to make this and the other new parameters non-optional? Even if it's not permanent, in can be a good exercise to see if other places need to change.

It wouldn't be a 100% straightforward "replace the ? with | undefined then add /*previousNode*/ undefined to function calls" because of a nextTokenAnd(parseLeftHandSideExpressionOrHigher). But it's 13 one-line changes otherwise: JoshuaKGoldberg@2f6b35e

Thanks. The main thing this points out to me is that there is only one narrow code path that preserves the previous node, and a bunch of others that could, but don't. I guess nothing else needs this information, but it still seems wasteful to wire it up for one usage.

sandersn · 2023-11-16T21:59:58Z

src/compiler/parser.ts

+        if (token() === SyntaxKind.SlashToken && scanner.scan() === SyntaxKind.GreaterThanToken) {
+            nextToken();
+            parseErrorAt(posLessThan - 1, getNodePos(), Diagnostics.JSX_tags_are_not_permitted_in_ts_files_Did_you_mean_to_change_the_file_extension_to_tsx, tokenToString(SyntaxKind.GreaterThanToken));
+        }


createMissingNode would probably work.

Edit: also, the stuff after a tag is JSX content anyway, right? What's the likelihood that it'll be text that the TS parser actually expects?

jakebailey · 2023-11-20T21:04:44Z

I'm not totally sure how I feel about threading the previous node down into the parser like this, honestly. I'm not sure of a better way, though, besides passing down a boolean that's true if it's a type assertion or something.

JoshuaKGoldberg · 2023-11-20T22:13:46Z

src/compiler/parser.ts

@@ -5558,9 +5559,9 @@ namespace Parser {
        );
    }

-    function parseBinaryExpressionOrHigher(precedence: OperatorPrecedence): Expression {
+    function parseBinaryExpressionOrHigher(precedence: OperatorPrecedence, previousNode?: Node): Expression {


From #56101 (comment):

I'm not totally sure how I feel about threading the previous node down into the parser like this, honestly. I'm not sure of a better way, though, besides passing down a boolean that's true if it's a type assertion or something.

I'll just go ahead and say this: I hate it. Adding in the first concept of a "previous" node to a parser for a specialized error message makes me unhappy.

An alternative could be to keep a variable alongside var currentToken: SyntaxKind & co. I dislike that ... more, honestly. But it's an option. 🤷

JoshuaKGoldberg · 2023-11-21T00:07:16Z

src/compiler/parser.ts

@@ -6601,6 +6611,25 @@ namespace Parser {
            case SyntaxKind.NewKeyword:
                return parseNewExpressionOrNewDotTarget();
            case SyntaxKind.SlashToken:
+                // JSX inside a .ts (not .tsx) file gets a specialized error...


From #56101 (review):

The change in parsePrimaryExpression is pretty invasive whereas the parseTypeAssertion is simple. How much of the benefit comes from the parseTypeAssertion change?

Hard to say what balance comes from parsing self-closing tags (parseTypeAssertion) vs. tags with children (parsePrimaryExpression). My intuition is that tags with children (the one we find more invasive) is probably much more common - especially in complex pages where the error messages without this change are convoluted. 😕

JoshuaKGoldberg · 2023-11-21T00:08:29Z

src/compiler/parser.ts

@@ -6601,6 +6611,25 @@ namespace Parser {
            case SyntaxKind.NewKeyword:
                return parseNewExpressionOrNewDotTarget();
            case SyntaxKind.SlashToken:
+                // JSX inside a .ts (not .tsx) file gets a specialized error...


From #56101 (review):

I almost wish we could add this message to an existing error that appears with JSX, only in cases where we can be sure that the cause really is a tag. Would that be a better approach? I'm not sure any of the existing errors have enough context to decide that they were really, truly caused by a JSX tag.

I looked into that at first but couldn't find any good path forward 😞. JSX syntax issues tend to cause multiple seemingly unrelated parsing errors. If you have a good path I would be very happy to go down it.

There should be a TypeAssertion somewhere earlier in the parser tree -- but we don't know something has gone wrong until much later, when we see an unexpected / that doesn't end up parsing as a regex.

What if you are extra suspicious of TypeAssertionExpressions? Maybe after parsePrimaryExpression you can do a speculative check for illegal stuff. That should at least let you detect small things like <x>y</x>, and probably also illegal things like <x>y z</x>. In theory that would then let you do some error recovery to skip past the closing tag, but that sounds quite difficult.

Edit: performance of parseTypeAssertion should be getting less important since I hope people are using it less over time.

JoshuaKGoldberg · 2023-11-21T00:17:11Z

src/compiler/parser.ts

+        if (token() === SyntaxKind.SlashToken && scanner.scan() === SyntaxKind.GreaterThanToken) {
+            nextToken();
+            parseErrorAt(posLessThan - 1, getNodePos(), Diagnostics.JSX_tags_are_not_permitted_in_ts_files_Did_you_mean_for_the_file_extension_to_be_tsx, tokenToString(SyntaxKind.GreaterThanToken));
+            return finishNode(factory.createTypeAssertion(type, createMissingNode(SyntaxKind.Identifier, /*reportAtCurrentPosition*/ false)), pos);


From #56101 (review):

I also don't like the fact that old errors are still around, and that the new error spans the entire tag. I guess it doesn't really matter what the span is, since the important thing is for the "why not JSX?" message to be early in the list of error messages.

Agreed! Even after adding createMissingNode (thanks for that!) there is still typically a Cannot find name '{0}'. reported on the type assertion. I tried replacing the factory.createTypeAssertion with a missingNode but that produced Error: Debug Failure. False expression: Invalid token in the tests.

If there's any way to tell TypeScript to not produce diagnostics for an entire area of code... that would be wonderful.

sandersn · 2023-11-29T14:14:54Z

src/compiler/parser.ts

+            parseErrorAt(posLessThan - 1, getNodePos(), Diagnostics.JSX_tags_are_not_permitted_in_ts_files_Did_you_mean_for_the_file_extension_to_be_tsx, tokenToString(SyntaxKind.GreaterThanToken));
+            return finishNode(factory.createTypeAssertion(type, createMissingNode(SyntaxKind.Identifier, /*reportAtCurrentPosition*/ false)), pos);
+        }
+
        parseExpected(SyntaxKind.GreaterThanToken);
        const expression = parseSimpleUnaryExpression();


Right after this, you'd want to check if (token() === SyntaxKind.Identifier) and tryParse some function that combines parseJSXChildren and parseJSXClosingElement. If that succeeds, you'll need to rebuild the previously parsed nodes above into an opening element (somehow??), issue your error, and return a JSX element. In theory the function you tryParse could be extracted from the first if body of parseJsxElementOrSelfClosingElementOrFragment, although it may do too much error recovery to be usable in a possibly-erroneous position.

Aside: Edge's AI-suggestion features are getting more aggressive but boy howdy was it confused by that paragraph.

Anyway, if (token() === SyntaxKind.LessThanToken) here, you'd instead tryParse(parseJSXClosingElement), which covers the <x>y</x> case.

I'm not yet sure how to reparse <x>y as JSX opening/child token. Maybe there's somewhere else in the parser that does that, or maybe you can create some error fields for TypeAssertion and just cram the body/close token there. Like I said, TypeAssertions are rarely used at this point, so making its structure weird or inefficient is not a huge problem.

JoshuaKGoldberg · 2024-03-26T17:30:51Z

Sorry to not have a good update here - this is a tricky issue and I don't have the bandwidth to focus on it now. 😞

If someone else could push this forward, that'd be great.

Thanks for the review!

Specialized parsing error for apparent JSX tag in .ts file

f1e5934

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Oct 13, 2023

Who added a formatter?!

80f126c

JoshuaKGoldberg added 11 commits October 17, 2023 20:50

Merge branch 'main' into better-jsx-parsing-errors-in-ts

f4166ea

Added handling for valid regular expression literals

973aac0

Extract into a tryReScan... scanner function

d4cd583

npx dprint fmt

8f284cc

Merge branch 'main' into better-jsx-parsing-errors-in-ts

914bdac

Remove unused baselines

00760ec

Hmm, tests seem to be passing locally...

7bd508f

Merge branch 'main' into better-jsx-parsing-errors-in-ts

24b4f45

Extracted into stateless helper

fd5adf7

fix: don't bypass parseTypeAssertion parsing

0f879a0

Who put that formatter here

0429983

JoshuaKGoldberg commented Nov 15, 2023

View reviewed changes

JoshuaKGoldberg marked this pull request as ready for review November 15, 2023 21:56

sandersn requested changes Nov 17, 2023

View reviewed changes

sandersn self-assigned this Nov 17, 2023

sandersn requested a review from jakebailey November 17, 2023 14:59

JoshuaKGoldberg commented Nov 20, 2023

View reviewed changes

JoshuaKGoldberg added 3 commits November 21, 2023 00:23

Improve error message

d31637a

Ah, the baselines too

5407472

Aha, createMissingNode. Thanks Nathan

0c9ce3b

JoshuaKGoldberg commented Nov 21, 2023

View reviewed changes

JoshuaKGoldberg requested a review from sandersn November 21, 2023 00:21

sandersn reviewed Nov 29, 2023

View reviewed changes

JoshuaKGoldberg closed this Mar 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Specialized parsing error for apparent JSX tag in .ts file #56101

Specialized parsing error for apparent JSX tag in .ts file #56101

JoshuaKGoldberg commented Oct 13, 2023

RyanCavanaugh commented Oct 13, 2023

JoshuaKGoldberg Nov 15, 2023

sandersn Nov 16, 2023

JoshuaKGoldberg Nov 21, 2023

sandersn left a comment

sandersn Nov 16, 2023

JoshuaKGoldberg Nov 21, 2023

sandersn Nov 16, 2023

JoshuaKGoldberg Nov 20, 2023 •

edited

Loading

sandersn Nov 28, 2023

sandersn Nov 16, 2023

jakebailey commented Nov 20, 2023

JoshuaKGoldberg Nov 20, 2023 •

edited

Loading

JoshuaKGoldberg Nov 21, 2023

JoshuaKGoldberg Nov 21, 2023

sandersn Nov 28, 2023 •

edited

Loading

JoshuaKGoldberg Nov 21, 2023 •

edited

Loading

sandersn Nov 29, 2023

JoshuaKGoldberg commented Mar 26, 2024 •

edited

Loading

Specialized parsing error for apparent JSX tag in .ts file #56101

Specialized parsing error for apparent JSX tag in .ts file #56101

Conversation

JoshuaKGoldberg commented Oct 13, 2023

RyanCavanaugh commented Oct 13, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey commented Nov 20, 2023

JoshuaKGoldberg Nov 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

JoshuaKGoldberg Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Mar 26, 2024 • edited Loading

JoshuaKGoldberg Nov 20, 2023 •

edited

Loading

JoshuaKGoldberg Nov 20, 2023 •

edited

Loading

sandersn Nov 28, 2023 •

edited

Loading

JoshuaKGoldberg Nov 21, 2023 •

edited

Loading

JoshuaKGoldberg commented Mar 26, 2024 •

edited

Loading