Add spread element check when the last element is a spread. (#673)

* fix issue (#670)

* formatting issue

* rename to improve readability

* comment on the boolean value indicates spread expr

* formatting issue

* fix syntax error in error message

* update CHANGELOG.md

* tweak

Co-authored-by: zdh <[email protected]>
Co-authored-by: Cristiano Calcagno <[email protected]>

Author: 3 people

CHANGELOG.md

@@ -38,6 +38,7 @@
 - Fix printing of comments inside JSX tag https://github.com/rescript-lang/syntax/pull/664
 - Fix issue where formatter erases tail comments inside JSX tag https://github.com/rescript-lang/syntax/issues/663
 - Fix issue where the JSX prop has type annotation of the first class module https://github.com/rescript-lang/syntax/pull/666
+- Fix issue where a spread `...x` in non-last position would not be reported as syntax error https://github.com/rescript-lang/syntax/pull/673/
 
 #### :eyeglasses: Spec Compliance
 

src/res_core.ml

@@ -50,7 +50,7 @@ module ErrorMessages = struct
   let listPatternSpread =
     "List pattern matches only supports one `...` spread, at the end.\n\
      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 \
+     middle would create new lists; out of performance concern, our pattern \
      matching currently guarantees to never create new intermediate data."
 
   let recordPatternSpread =
@@ -84,8 +84,8 @@ module ErrorMessages = struct
   let listExprSpread =
     "Lists can only have one `...` spread, and at the end.\n\
      Explanation: lists are singly-linked list, where a node contains a value \
-     and points to the next node. `list[a, ...bc]` efficiently creates a new \
-     item and links `bc` as its next nodes. `[...bc, a]` would be expensive, \
+     and points to the next node. `list{a, ...bc}` efficiently creates a new \
+     item and links `bc` as its next nodes. `list{...bc, a}` would be expensive, \
      as it'd need to traverse `bc` and prepend each item to `a` one by one. We \
      therefore disallow such syntax sugar.\n\
      Solution: directly use `concat`."
@@ -3716,25 +3716,27 @@ and parseSpreadExprRegion p =
   | _ -> None
 
 and parseListExpr ~startPos p =
-  let listExprs =
+  let check_all_non_spread_exp exprs =
+    exprs
+    |> List.map (fun (spread, expr) ->
+           if spread then
+             Parser.err p (Diagnostics.message ErrorMessages.listExprSpread);
+           expr)
+    |> List.rev
+  in
+  let listExprsRev =
     parseCommaDelimitedReversedList p ~grammar:Grammar.ListExpr ~closing:Rbrace
       ~f:parseSpreadExprRegion
   in
   Parser.expect Rbrace p;
   let loc = mkLoc startPos p.prevEndPos in
-  match listExprs with
-  | (true, expr) :: exprs ->
-    let exprs = exprs |> List.map snd |> List.rev in
+  match listExprsRev with
+  | (true, (* spread expression *)
+           expr) :: exprs ->
+    let exprs = check_all_non_spread_exp exprs in
     makeListExpression loc exprs (Some expr)
   | exprs ->
-    let exprs =
-      exprs
-      |> List.map (fun (spread, expr) ->
-             if spread then
-               Parser.err p (Diagnostics.message ErrorMessages.listExprSpread);
-             expr)
-      |> List.rev
-    in
+    let exprs = check_all_non_spread_exp exprs in
     makeListExpression loc exprs None
 
 (* Overparse ... and give a nice error message *)

tests/parsing/errors/other/expected/spread.res.txt

@@ -49,6 +49,20 @@ Explanation: you can't collect a subset of a record's field into its own record,
 Solution: you need to pull out each field you want explicitly.
 
 
+  Syntax error!
+  tests/parsing/errors/other/spread.res:8:1-3
+
+   6 │ 
+   7 │ let myList = list{...x, ...y}
+   8 │ let list{...x, ...y} = myList
+   9 │ 
+  10 │ type t = {...a}
+
+  Lists can only have one `...` spread, and at the end.
+Explanation: lists are singly-linked list, where a node contains a value and points to the next node. `list{a, ...bc}` efficiently creates a new item and links `bc` as its next nodes. `list{...bc, a}` would be expensive, as it'd need to traverse `bc` and prepend each item to `a` one by one. We therefore disallow such syntax sugar.
+Solution: directly use `concat`.
+
+
   Syntax error!
   tests/parsing/errors/other/spread.res:8:13-22
 
@@ -59,7 +73,7 @@ Solution: you need to pull out each field you want explicitly.
   10 │ type t = {...a}
 
   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.
+Explanation: a list spread at the tail is efficient, but a spread in the middle would create new lists; out of performance concern, our pattern matching currently guarantees to never create new intermediate data.
 
 
   Syntax error!