Recover from broken JSX prop #6660
Conversation
| | Equal -> ( | ||
| Parser.next state; | ||
| match state.Parser.token with | ||
| (* arrived at k1={ *) | ||
| | Lbrace -> ( | ||
| Parser.next state; | ||
| match state.Parser.token with | ||
| | Rbrace -> false | ||
| | _ -> | ||
| goToClosing Rbrace state; | ||
| isPossibleAfterRbrace state.Parser.token) | ||
| (* arrived at k1=v1 *) | ||
| | token when isPossibleAfterEqual token -> ( | ||
| Parser.next state; | ||
| match state.Parser.token with | ||
| (* arrived at k1=v1 =v2 *) | ||
| | Equal -> false | ||
| | _ -> true) | ||
| (* arrived at k1=x *) | ||
| | _ -> false) | ||
| | _ -> false) |
There was a problem hiding this comment.
When looking ahead, there are two possibilities when we encounter a prop,
- prop where the value is wrapped with
{}likeprop={...}.- any expression can live inside this
{}
- any expression can live inside this
- prop where the value is specified without
{}likeprop=true.- this can be number literal, string literal, list, array, module, etc
There was a problem hiding this comment.
I think for the first case, we could just check for { (Lbrace) but for completeness’s sake, it's probably a good idea to be this explicit.
There was a problem hiding this comment.
To keep things simple, I would do nothing in the { case, and avoid long look aheads searching for }.
Then keep the changes and additional code to a minimum.
There was a problem hiding this comment.
In fact I can't think of a case other than x = y = that one needs to handle.
The situation is one starts typing x = in a jsx context and expects completion.
This currently does not happen if it's before something of the form y = and I don't know if there are other cases to consider in this scenario.
There was a problem hiding this comment.
One more case that is not covered is prop punning. If we have just y instead of y=e and the user types x=, then we get x=y with no parse error at all. It's also totally not obvious that the intention is to autocomplete.
In other words, this approach does not work at all with prop pinning.
@zth perhaps one can try with a different approach directly in the vscode extension: in case of x=y if the cursor is after = and y is a valid prop name for the component, then infer the intent to autocomplete x=. This would cover punned as well as non punned cases. Thoughts?
|
@zth marked as draft for now as I'm not 100% sure if this is the best way to check for well formed props (I mean it works but it could be better). |
shulhi
force-pushed
the
fix/recover-broken-jsx-prop
branch
from
dcad755 to
7d7cf16
Compare
| if optional then Asttypes.Optional name else Asttypes.Labelled name | ||
| in | ||
| Some (label, attrExpr)) | ||
| else |
There was a problem hiding this comment.
This branch should probably raise an error. Otherwise a hole is added and the parser can continue as if the parsing was successful.
| | Equal -> ( | ||
| Parser.next state; | ||
| match state.Parser.token with | ||
| (* arrived at k1={ *) | ||
| | Lbrace -> ( | ||
| Parser.next state; | ||
| match state.Parser.token with | ||
| | Rbrace -> false | ||
| | _ -> | ||
| goToClosing Rbrace state; | ||
| isPossibleAfterRbrace state.Parser.token) | ||
| (* arrived at k1=v1 *) | ||
| | token when isPossibleAfterEqual token -> ( | ||
| Parser.next state; | ||
| match state.Parser.token with | ||
| (* arrived at k1=v1 =v2 *) | ||
| | Equal -> false | ||
| | _ -> true) | ||
| (* arrived at k1=x *) | ||
| | _ -> false) | ||
| | _ -> false) |
There was a problem hiding this comment.
In fact I can't think of a case other than x = y = that one needs to handle.
The situation is one starts typing x = in a jsx context and expects completion.
This currently does not happen if it's before something of the form y = and I don't know if there are other cases to consider in this scenario.
| | Equal -> ( | ||
| Parser.next state; | ||
| match state.Parser.token with | ||
| (* arrived at k1={ *) | ||
| | Lbrace -> ( | ||
| Parser.next state; | ||
| match state.Parser.token with | ||
| | Rbrace -> false | ||
| | _ -> | ||
| goToClosing Rbrace state; | ||
| isPossibleAfterRbrace state.Parser.token) | ||
| (* arrived at k1=v1 *) | ||
| | token when isPossibleAfterEqual token -> ( | ||
| Parser.next state; | ||
| match state.Parser.token with | ||
| (* arrived at k1=v1 =v2 *) | ||
| | Equal -> false | ||
| | _ -> true) | ||
| (* arrived at k1=x *) | ||
| | _ -> false) | ||
| | _ -> false) |
There was a problem hiding this comment.
One more case that is not covered is prop punning. If we have just y instead of y=e and the user types x=, then we get x=y with no parse error at all. It's also totally not obvious that the intention is to autocomplete.
In other words, this approach does not work at all with prop pinning.
@zth perhaps one can try with a different approach directly in the vscode extension: in case of x=y if the cursor is after = and y is a valid prop name for the component, then infer the intent to autocomplete x=. This would cover punned as well as non punned cases. Thoughts?
|
@shulhi, @cristianoc made a good point in the review, that it's probably not possible to make this work with a lookahead because of punning. I went back to the drawing board, and I think I have an acceptable trade off solution that just affects the editor tooling, so we don't have to change the parser at all: rescript-lang/rescript-vscode#935 This was some great work though and much appreciated! But if we can get away with not changing the parser, then that's of course better. Sorry for leading you down that rabbit hole. |
I thought about punning when implementing the look ahead and decided that it was probably okay to accept the case where
No worries, there are still plenty of issues to work on 😄 I'll close the PR in favor of rescript-lang/rescript-vscode#935 |
Fix for #6559