Skip to content

Recover closing fragment with contents differently #47534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from

Conversation

jakebailey
Copy link
Member

Fixes #44154

I don't know if I like this. This effectively does what was noted in #44154 (comment), but does lead to some extra errors and a different JS output which feels undesirable.

@andrewbranch
Copy link
Member

I guess my ideal scenario would be

  • </Foo> parses as a JsxElement with a zero-length JsxOpeningElement and the error says you're missing a corresponding opening tag
  • <></Foo> parses as an unclosed JsxFragment with ^ inside it

It seems like that would pass the assertion in services and be a net improvement on parser errors reported today. But I don't know how difficult that is.

@jakebailey
Copy link
Member Author

It seems like at the moment, having a random </Foo> in your code doesn't actually parse anything JSX related at all (so the first bullet doesn't exist yet), which makes doing the above tricky because by the time we're here trying to parse the end of a fragment, the children for the fragment are already expected to be completed.

@jakebailey
Copy link
Member Author

Hmm, I have an idea here; I'll give it a try.

@jakebailey jakebailey marked this pull request as draft January 21, 2022 01:26
@jakebailey
Copy link
Member Author

@andrewbranch I made an attempt to implement the setup you suggested; it does seem to work, but it does seem to break auto-fragment closing in one case because now we think the fragment is closed (as opposed to element); the test will fail in the PR if you want to look.

The error's a placeholder for now.

//@Filename: file.tsx
//// let x = <></Fo/*$*/o>;

verify.quickInfoAt("$", "let Foo: any")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails right now as it says "any"; I think this test case was wrong originally given I didn't actually declare Foo.

@jakebailey
Copy link
Member Author

I'm sort of worried about this approach because it makes the parser start eating closing tags moreso than before, so if you stick a <> somewhere as you're trying to do something with it, you'll end up with a lot of errors below, at least that's my best guess. Will have to try.

Comment on lines -25 to -26
~~~~~~~~~~~~~~~~
!!! error TS2657: JSX expressions must have one parent element.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error seems to get dropped because we're eagerly eating up tags after <> (whereas this test assumed that it would stop earlier, and therefore wrote two tests on two different lines).

Comment on lines 5323 to 5325
if (tokenIsIdentifierOrKeyword(token())) {
parseErrorAtRange(parseJsxElementName(), Diagnostics.Expected_corresponding_closing_tag_for_JSX_fragment);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition is now impossible due to the change above.

@@ -50,7 +50,7 @@ verify.jsxClosingTag({
1: undefined,
2: undefined,
3: undefined,
4: { newText: "</>" },
4: undefined,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case looks like:

// @Filename: /4.tsx
////const x = <div>
////    <>/*4*/
////    </div>
////</>;

So, we now see the fragment close below and don't offer up a close. Maybe reasonable, maybe not.

@jakebailey
Copy link
Member Author

#52818 is comically better than this.

@jakebailey jakebailey closed this Feb 17, 2023
@jakebailey jakebailey deleted the fix-44154 branch March 18, 2023 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Services debug failure in presence of unclosed JSX fragment
3 participants