Fix nested variant type spreads #7080
Conversation
There was a problem hiding this comment.
it makes sense that giving dotdotdot a new meaning in the syntax can lead to some corner cases for its existing use, but so far I find the trade-off acceptable, if we bump into more issues, we could try to refine the behavior of dotdotdot for lists.
Could you rebase the PR?
jscomp/syntax/src/res_grammar.ml
Outdated
| @@ -171,7 +171,8 @@ let is_structure_item_start = function | |||
| let is_pattern_start = function | |||
| | Token.Int _ | Float _ | String _ | Codepoint _ | Backtick | True | False | |||
| | Minus | Plus | Lparen | Lbracket | Lbrace | List | Dict | Underscore | |||
| | Lident _ | Uident _ | Hash | Exception | Percent | Module | At -> | |||
| | DotDotDot | Lident _ | Uident _ | Hash | Exception | Percent | Module | At | |||
There was a problem hiding this comment.
yeah same fix as for dicts! It smelled the same ^^
| I'm not sure what to parse here when looking at ",". | ||
| Did you forget a `}` here? |
There was a problem hiding this comment.
to be honest, the former error message was not great, the newer one is not better, but I'm not even sure what should be the error message here.
zth
force-pushed
the
fix-nested-variant-type-srpeads
branch
from
e98952d to
8a4bf5b
Compare
|
@tsnobip I did not add |
@zth Did you try adding it to the atomic pattern start ? Did it change the tests for lists too? Intuitively I'd say it's not atomic because it needs at least some argument, but to be honest I'm not sure what it really means. True and False definitely look atomic to me for example. Some cleanup would be nice here. Maybe Maxim knows better about the real semantics and origin of atomic pattern starts? I couldn't find his github handle. |
This fixes the issues outlined here: #6721 (review)
Caveat: It does seem to change the parsing recovery behavior in lists: https://github.com/rescript-lang/rescript-compiler/compare/fix-nested-variant-type-srpeads?expand=1#diff-f1835faf75e20ea1a0421cb16ab6be3e9556c0d4651f05789be6b26f1903f998L27-R38
Not sure what to think about that.
cc @tsnobip @glennsl