-
Notifications
You must be signed in to change notification settings - Fork 428
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
Super spread errors part 2 #1973
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I've tweaked the messages! Could you rebase please? Sorry about that
src/reason-parser/reason_parser.mly
Outdated
@@ -1036,6 +1036,22 @@ let raise_record_trailing_semi_error loc = | |||
let msg = "Record entries are separated by comma; we've found a semicolon instead." in | |||
raise Reason_syntax_util.(Error(loc, (Syntax_error msg))) | |||
|
|||
let record_exp_spread_msg = | |||
"Records can only have one `...` spread, at the beginning" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget the period at the end!
Records can only have one `...` spread, at the beginning.
Explanation: since records have a known, fixed shape, a spread like `{a, ...b}` wouldn't make sense, as `b` would override every field of `a` anyway.
src/reason-parser/reason_parser.mly
Outdated
"Records can only have one `...` spread, at the beginning" | ||
|
||
let record_pat_spread_msg = | ||
"The `...` spread operator is not supported in record pattern matches" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Record's `...` spread is not supported in pattern matches.
Explanation: you can't collect a subset of a record's field into its own record, since a record needs an explicit declaration and that subset wouldn't have one.
Solution: you need to pull out each field you want explicitly.
src/reason-parser/reason_parser.mly
Outdated
lseparated_list(COMMA, opt_spread(expr_optional_constraint)) | ||
COMMA? | ||
BARRBRACKET | ||
{ let msg = "Arrays can not use the `...` spread. Arrays in Reason are fixed length and spreading them would allocate a new array." in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arrays can't use the `...` spread currently. Please use `concat` or other Array helpers.
src/reason-parser/reason_parser.mly
Outdated
(* If the `...` appears in any other location then the last, | ||
* show a friendly, clear message explaining the consequences. *) | ||
let msg = "Lists can only have one `...` spread, and at the end. | ||
let msg = "Lists can only have one `...` spread, and at the end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry could you change this one to remove and
? The comma is probably enough to disambiguate meaning here.
src/reason-parser/reason_parser.mly
Outdated
raise Reason_syntax_util.(Error(dotdotdotLoc, (Syntax_error msg))) | ||
| None -> e | ||
) es in | ||
Solution: directly use `concat`." in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also change this one too:
Solution: directly use `concat` or other List helpers.
(I wrote it that way for List and Array because concat
is shared by stdlib and Belt, but have different meanings. In reality you'd use List.append
or Belt.List.concat
, or List.concat
or Belt.List.concatMany
.)
src/reason-parser/reason_parser.mly
Outdated
@@ -3701,22 +3702,42 @@ mark_position_pat | |||
; | |||
|
|||
%inline pattern_comma_list: | |||
lseparated_nonempty_list(COMMA, pattern) { $1 }; | |||
lseparated_nonempty_list(COMMA, opt_spread(pattern)) | |||
{ let msg = "Pattern matching on arrays can not have a `...` spread. It would allocate a whole new array every time." in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array's `...` spread is not supported in pattern matches.
Explanation: such spread would create a subarray; out of performance concern, our pattern matching currently guarantees to never create new intermediate data.
Solution: if it's to validate the first few elements, use a `when` clause + Array size check + `get` checks on the current pattern. If it's to obtain a subarray, use `Array.sub` or `Belt.Array.slice`.
src/reason-parser/reason_parser.mly
Outdated
| Some dotdotdotLoc -> (ps, Some p) | ||
| None -> (hd::ps, None) | ||
in | ||
let msg = "Pattern matching on a list can only have one `...` spread, and at the end" in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List pattern matches only supports one `...` spread, at the end.
Explanation: a list spread at the tail is efficient, but a spread in the middle would create new list(s); out of performance concern, our pattern matching currently guarantees to never create new intermediate data.
-Arrays (expressions + patterns) -List (patterns) -Records (expressions + patterns)
Fixes #1952
-Arrays (expressions + patterns)
-List (expressions + patterns)
-Records (expressions + patterns)