fix misparsing in/after JSX (#6677) #6686
Conversation
tsnobip
force-pushed
the
fix_hyphens_in_jsx
branch
from
1b884e6 to
ec4c09f
Compare
| Scanner.popMode p.scanner Jsx; | ||
| Parser.expect GreaterThan p; |
There was a problem hiding this comment.
we expect the end of the JSX so we can pop JSX mode
| Scanner.popMode p.scanner Jsx; | ||
| Parser.expect GreaterThan p; |
| @@ -2687,7 +2689,6 @@ and parseJsxOpeningOrSelfClosingElement ~startPos p = | |||
| * jsx-children ::= primary-expr* * => 0 or more | |||
| *) | |||
| and parseJsx p = | |||
| Scanner.popMode p.scanner Jsx; | |||
There was a problem hiding this comment.
this was not needed
| @@ -2700,7 +2701,6 @@ and parseJsx p = | |||
| parseJsxFragment p | |||
| | _ -> parseJsxName p | |||
| in | |||
| Scanner.popMode p.scanner Jsx; | |||
| if p.token = LessThan then p.token <- Scanner.reconsiderLessThan p.scanner; | ||
| Parser.expect LessThanSlash p; | ||
| Parser.expect GreaterThan p; | ||
| Scanner.popMode p.scanner Jsx; | ||
| Parser.expect GreaterThan p; |
There was a problem hiding this comment.
we first check if the next token is not </ instead of just < to test the end of the fragment with a following > (forming a </>), if it's the case then we can pop JSX mode.
| | _ -> None | ||
|
|
||
| and parseJsxProps p = | ||
| parseRegion ~grammar:Grammar.JsxAttribute ~f:parseJsxProp p | ||
|
|
||
| and parseJsxChildren p = | ||
| Scanner.popMode p.scanner Jsx; |
There was a problem hiding this comment.
JSX children should not be parsed in JSX mode because they could be any kind of expression between curly braces.
| let children = List.rev (loop p []) in | ||
| (false, children) | ||
| in | ||
| Scanner.setJsxMode p.scanner; |
There was a problem hiding this comment.
once we're done parsing the children, we reset JSX mode because we have to parse the closing tag.
|
side-note: This is off-topic for this PR, but it seems like the current implementation of parsing with Jsx and Diamond as state values for |
I think the current implementation is far from being ideal. Basically, it was bearable before because the implementation was not really sensitive to the mode. The result was that the JSX mode was leaking most of the time but basically it didn't have any impact. Now that the JSX mode is more different, you have to be careful about when you set and pop it. I even tried modifying the parser in order to reflect the mode in the type of the parser, but it was not really possible because of the way the parser is designed (using mutation). The easiest solution would likely be as you suggested to just get rid of the mode in the parser and likely add a parameter to Parser.next or something like that. What do you think? |
Agreed. |
|
@tsnobip Thanks a lot for the fix! 👍 Regarding the refactoring of the mode handling, I think we should do that on the master branch. For the 11.0_release branch, let's just merge this fix. The tests are running without errors, and I also just tried compiling a large project with these changes, and everything worked fine. |
|
Great work @tsnobip! |
Fixes #6677.
The tracking of JSX mode should be more robust, we basically pop JSX mode where it should (inside children for example), and we pop it before checking for the end of JSX (because expect also parses the next token and we don't want it to be parsed in JSX mode).