Improve parser recovery for unclosed/mismatched JSX elements

[sandersn]

1. Improve recovery for malformed elements: malformed elements not immediately followed by a < previously parsed [most of] the rest of the file as JsxText. That's because, nested inside another JSX tag, when attempting to parse a closing >, all the functions unconditionally advanced the token, whether or not the > was there. This parsed a following </ (for example) as jsx text, and then the rest of parsing went wrong.

parseExpected has a mode that doesn't advance the scanner, but it wasn't used consistently (or understandably) before.

2. Improve recovery for a correctly-formed, but unmatched, opening element that's nested inside a correctly matched open/close pair:

<div><span></div>

Previously this parsed as <div>(<span></div>), which meant that the first <div> was unclosed, which again parsed the rest of the file as jsx text, or at least until another </div> was found.

Now, in the code that finalises jsx elements, when there is a mismatched open/close (eg <span></div>, and the mismatched close text (</div>) matches the parent's opening text (<div>), the parser creates a new jsx element with the mismatched open (<span></span>) and re-attaches the mismatched close to parent open (<div>...</div>).

This may not be correct 100% of the time, but it's far better than the old nearest-attach behaviour.

3. Future work: the old way of issuing errors for mismatched open/closes was to parse the rest of the file as jsx text, then when the child was EndOfFileToken, issue a "no matching tag" error on the parent. I put an assert in its place and 6 tests failed. So there are 6 tests whose JSX parsing runs to the end of the file.

Edit: I investigated those 6 tests and didn't learn much because the unclosed tags really were at the end of the file. I need to learn more about what can appear in jsx text to see if there are other boundaries that could indicate a runaway unclosed tag.

Fixes #43496

[sandersn]

lol @ fourslash

[sandersn]

Fix (1) follows

[sandersn]

this is the bulk of fix (2), with a couple of ifs below to correct parsing in the error case.

[sandersn]

@weswigham I included you because you know a lot about JSX but if you don't feel like looking at a bunch of parsing, I think @elibarzilay and @andrewbranch have reviewed most of my recent parsing PRs.

[andrewbranch]

I understand fix (2) and it looks good. I don’t really understand fix (1) without debugging but it looks plausible and tested. How confident are you that this will not break valid syntax? Is our JSX test coverage pretty good, and do you feel good about merging this into 4.3 post-beta?

[RyanCavanaugh]

I share Andrew's concerns, conceptually. I wouldn't merge this too close to the RC but I think we have enough runway to go for it now.

[sandersn]

@andrewbranch I am very confident about fix (1); the manual advancement of the scanner before was just wrong, and in a confusing way.
Fix (2) has a lot of moving parts, so I'm less confident about it.

Some additional considerations:

• Our JSX test coverage has a similar character to our other tests, but more so: there's a fair amount of JSX but it doesn't cover cases you'd encounter in incomplete programs that well; most of the tests I wrote for this PR are not redundant with existing baselines.
• Both changes only apply in "non-expression context" (I don't know if there's another name for "in JSX text, nested inside JSX elements") so in theory they're less likely to cause problems outside.
• JSX parsing has been this bad for a long time, so fixing it is not urgent.

I would personally rather hold this till 4.4, I just forgot to add that section in the PR discussion. I would be OK shipping fix (1) separately in 4.3, but there's really not much urgency here.