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

Disallow < after a JSX element #141

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nicolo-ribaudo
Copy link

@nicolo-ribaudo nicolo-ribaudo commented Feb 25, 2022

Fixes #120.

This was quite hard to specify:

  • I initially tried to add an early error to RelationalExpression saying something like "It is a SyntaxError if the left side of the expression recursively contains a JSXElementOrFragment as its right-most descendent.", but it would have required writing some custom sdo for every production that can derive a JSXElementOrFragment as a child of RelationalExpression.
  • The spec explicitly ignores lookaheads in static semantics, so I wanted to avoid doing it.

⚠️ Almost all the tools already implement this error, but it would be a breaking change for the Flow type checker.

Copy link
Contributor

@Huxpro Huxpro left a comment

Choose a reason for hiding this comment

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

I'm still waiting for Flow team to give a response but other than that (and a question), this LGTM.

<ins>JSXElement</ins>
<ins>JSXFragment</ins>
PrimaryExpression ::
<ins>JSXElementOrFragment `<`</ins>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this production to be able to define an early error on PrimaryExpression :: JSXElementOrFragment <?

Copy link
Author

Choose a reason for hiding this comment

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

We could do something like

PrimaryExpression :: JSXElementOrFragment

- It is a Syntax Error is this production is followed by the `<` token

but I used that new production to avoid introducing new language/checks that are not already used in the ecma262 spec

Copy link
Contributor

@Huxpro Huxpro Feb 28, 2022

Choose a reason for hiding this comment

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

I understand that without PrimaryExpression :: JSXElementOrFragment < we don't have a production to match with. But it's also not ideal to me to only add this production specifically for the early error 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Note that the ecma262 spec already does this in a few cases, for example to disallow a?.b`c`: https://tc39.es/ecma262/#sec-left-hand-side-expressions-static-semantics-early-errors

@Huxpro Huxpro added Proposal Living Proposals considerable for the living JSX Impl Reality Reality that the spec does not capture labels Feb 28, 2022
spec.emu Outdated Show resolved Hide resolved
Copy link
Contributor

@Huxpro Huxpro left a comment

Choose a reason for hiding this comment

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

I'm still on board with fixing #140 but I think we need to find a better mechanics.

Co-authored-by: Xuan Huang (黄玄) <[email protected]>
Copy link
Contributor

@Huxpro Huxpro left a comment

Choose a reason for hiding this comment

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

Came across and I think this LGTM.

It may be nicer if we could have a note like https://tc39.es/ecma262/#sec-left-hand-side-expressions-static-semantics-early-errors to say something similar:

This production (only) exists in order to prevent ...

As I'm no longer employed by Meta, we will need some help to actually merge this. cc @poteto if anyone on the team is interested in taking a look.

@Huxpro Huxpro removed their assignment Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Impl Reality Reality that the spec does not capture Proposal Living Proposals considerable for the living JSX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add early error to disallow invalid adjacent JSX elements
3 participants