Skip to content
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

Disable JSX recovery hack when in unary expression context #53666

Merged
merged 2 commits into from Apr 5, 2023

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Apr 5, 2023

Fixes #53114
Fixes #53637

I broke this in #52667 by adding a new call to the JSX element parser. That element parser has a unique recovery method which returns a BinaryExpression, but, this is incompatible with unary expressions, which cannot contain binary expressions.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Apr 5, 2023
@jakebailey jakebailey added this to the TypeScript 5.0.4 milestone Apr 5, 2023
@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Apr 5, 2023
Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

I guess the fix here is okay - it seems overly-targeted to a specific code path though. My feeling is the right thing to do is to only do the recovery step when everything prior to the < had no errors.

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

also using "crash" in the test name - I'm not huge on it

@jakebailey
Copy link
Member Author

jakebailey commented Apr 5, 2023

I guess the fix here is okay - it seems overly-targeted to a specific code path though. My feeling is the right thing to do is to only do the recovery step when everything prior to the < had no errors.

If that were the implementation, then I think we'd end up crashing on this code too: ~<></> <, or the code that the comment above this code mentions + unary, so ~<div></div><div></div>. So I feel like the fix in this PR is correct.

also using "crash" in the test name - I'm not huge on it

I feel like we've somewhat consistently done this:

$ ls tests/cases/compiler | grep -i crash | wc -l
63

If it were me I'd call this test case issue53637.ts!

@DanielRosenwasser
Copy link
Member

Duh - good point. Could you add that case though?

@jakebailey
Copy link
Member Author

Yes, will do.

@jakebailey jakebailey merged commit bebb6d0 into microsoft:main Apr 5, 2023
19 checks passed
@jakebailey jakebailey deleted the fix-53114-2 branch April 5, 2023 20:04
@jakebailey
Copy link
Member Author

@typescript-bot cherry-pick this to release-5.0

@typescript-bot
Copy link
Collaborator

typescript-bot commented Apr 5, 2023

Heya @jakebailey, I've started to run the task to cherry-pick this into release-5.0 on this PR at 74e40e4. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, I've opened #53676 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Apr 5, 2023
Component commits:
db8e22a Disable JSX recovery hack when in unary expression context

74e40e4 Add additional test case
DanielRosenwasser pushed a commit that referenced this pull request Apr 5, 2023
…e-5.0 (#53676)

Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
drivron pushed a commit to scenari/typescript that referenced this pull request Sep 14, 2023
…to release-5.0 (microsoft#53676)

Co-authored-by: Jake Bailey <5341706+jakebailey@users.noreply.github.com>
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
4 participants