From f65c1af649ed7bf263a5e6a48c60f1eaff438a0e Mon Sep 17 00:00:00 2001 From: zdh <935021934@qq.com> Date: Wed, 12 Oct 2022 23:22:14 +0800 Subject: [PATCH 1/8] fix issue (#670) --- src/res_core.ml | 17 +++++++++-------- .../errors/other/expected/spread.res.txt | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/res_core.ml b/src/res_core.ml index 49d075c5..90150b46 100644 --- a/src/res_core.ml +++ b/src/res_core.ml @@ -3716,6 +3716,13 @@ and parseSpreadExprRegion p = | _ -> None and parseListExpr ~startPos p = + 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 listExprs = parseCommaDelimitedReversedList p ~grammar:Grammar.ListExpr ~closing:Rbrace ~f:parseSpreadExprRegion @@ -3724,16 +3731,10 @@ and parseListExpr ~startPos p = let loc = mkLoc startPos p.prevEndPos in match listExprs with | (true, expr) :: exprs -> - let exprs = exprs |> List.map snd |> List.rev in + 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 + let exprs = check_all_non_spread_exp exprs in makeListExpression loc exprs None diff --git a/tests/parsing/errors/other/expected/spread.res.txt b/tests/parsing/errors/other/expected/spread.res.txt index cfddb204..a5bc3e88 100644 --- a/tests/parsing/errors/other/expected/spread.res.txt +++ b/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. `[...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 From 5d9da596a511c2e1af2a0af2d5956ff941510d5d Mon Sep 17 00:00:00 2001 From: butter Date: Wed, 12 Oct 2022 23:58:07 +0800 Subject: [PATCH 2/8] formatting issue --- src/res_core.ml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/res_core.ml b/src/res_core.ml index 90150b46..371261dd 100644 --- a/src/res_core.ml +++ b/src/res_core.ml @@ -3722,7 +3722,8 @@ and parseListExpr ~startPos p = if spread then Parser.err p (Diagnostics.message ErrorMessages.listExprSpread); expr) - |> List.rev in + |> List.rev + in let listExprs = parseCommaDelimitedReversedList p ~grammar:Grammar.ListExpr ~closing:Rbrace ~f:parseSpreadExprRegion @@ -3734,8 +3735,7 @@ and parseListExpr ~startPos p = let exprs = check_all_non_spread_exp exprs in makeListExpression loc exprs (Some expr) | exprs -> - let exprs = check_all_non_spread_exp exprs - in + let exprs = check_all_non_spread_exp exprs in makeListExpression loc exprs None (* Overparse ... and give a nice error message *) From c1088fecc1014e77311fe8745324dcc6c03b0104 Mon Sep 17 00:00:00 2001 From: butter Date: Thu, 13 Oct 2022 14:19:56 +0800 Subject: [PATCH 3/8] rename to improve readability --- src/res_core.ml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/res_core.ml b/src/res_core.ml index 371261dd..c29ad591 100644 --- a/src/res_core.ml +++ b/src/res_core.ml @@ -3724,13 +3724,13 @@ and parseListExpr ~startPos p = expr) |> List.rev in - let listExprs = + 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 + match listExprsRev with | (true, expr) :: exprs -> let exprs = check_all_non_spread_exp exprs in makeListExpression loc exprs (Some expr) From 1a7c3f25b12d0cf5808b57f713583426509c4999 Mon Sep 17 00:00:00 2001 From: butter Date: Thu, 13 Oct 2022 14:24:53 +0800 Subject: [PATCH 4/8] comment on the boolean value indicates spread expr --- src/res_core.ml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/res_core.ml b/src/res_core.ml index c29ad591..9e7d0d8b 100644 --- a/src/res_core.ml +++ b/src/res_core.ml @@ -3731,7 +3731,8 @@ and parseListExpr ~startPos p = Parser.expect Rbrace p; let loc = mkLoc startPos p.prevEndPos in match listExprsRev with - | (true, expr) :: exprs -> + | (true, (* spread expression *) + expr) :: exprs -> let exprs = check_all_non_spread_exp exprs in makeListExpression loc exprs (Some expr) | exprs -> From a903246cb6f2693c751c5800d0d94aeb18dbae4d Mon Sep 17 00:00:00 2001 From: butter Date: Thu, 13 Oct 2022 14:31:47 +0800 Subject: [PATCH 5/8] formatting issue --- src/res_core.ml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/res_core.ml b/src/res_core.ml index 9e7d0d8b..cb248561 100644 --- a/src/res_core.ml +++ b/src/res_core.ml @@ -3732,7 +3732,7 @@ and parseListExpr ~startPos p = let loc = mkLoc startPos p.prevEndPos in match listExprsRev with | (true, (* spread expression *) - expr) :: exprs -> + expr) :: exprs -> let exprs = check_all_non_spread_exp exprs in makeListExpression loc exprs (Some expr) | exprs -> From d1c004cceecc1c0ea587a89f0142f960aab6fd01 Mon Sep 17 00:00:00 2001 From: butter Date: Thu, 13 Oct 2022 14:42:22 +0800 Subject: [PATCH 6/8] fix syntax error in error message --- src/res_core.ml | 6 +++--- tests/parsing/errors/other/expected/spread.res.txt | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/res_core.ml b/src/res_core.ml index cb248561..db11f141 100644 --- a/src/res_core.ml +++ b/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 list{s}; 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. `{...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`." diff --git a/tests/parsing/errors/other/expected/spread.res.txt b/tests/parsing/errors/other/expected/spread.res.txt index a5bc3e88..4c154155 100644 --- a/tests/parsing/errors/other/expected/spread.res.txt +++ b/tests/parsing/errors/other/expected/spread.res.txt @@ -59,7 +59,7 @@ Solution: you need to pull out each field you want explicitly. 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. `[...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. +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, 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`. @@ -73,7 +73,7 @@ Solution: directly use `concat`. 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 list{s}; out of performance concern, our pattern matching currently guarantees to never create new intermediate data. Syntax error! From 3a40a82b9b191377676f585833be7cf604099a75 Mon Sep 17 00:00:00 2001 From: butter Date: Thu, 13 Oct 2022 14:50:03 +0800 Subject: [PATCH 7/8] update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f4856d2..4b3a48af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,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 the syntax error caused by non-last spread doesn't report when its last element is a spread https://github.com/rescript-lang/syntax/pull/673/ #### :eyeglasses: Spec Compliance From 3c642cceb430a578e3018ad0ad9a4db1fea4a9ac Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Thu, 13 Oct 2022 10:00:03 +0200 Subject: [PATCH 8/8] tweak --- CHANGELOG.md | 2 +- src/res_core.ml | 4 ++-- tests/parsing/errors/other/expected/spread.res.txt | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b3a48af..48f568da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,7 +34,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 the syntax error caused by non-last spread doesn't report when its last element is a spread https://github.com/rescript-lang/syntax/pull/673/ +- 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 diff --git a/src/res_core.ml b/src/res_core.ml index db11f141..094307ec 100644 --- a/src/res_core.ml +++ b/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 = @@ -85,7 +85,7 @@ module ErrorMessages = struct "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, \ + 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`." diff --git a/tests/parsing/errors/other/expected/spread.res.txt b/tests/parsing/errors/other/expected/spread.res.txt index 4c154155..b6b0b914 100644 --- a/tests/parsing/errors/other/expected/spread.res.txt +++ b/tests/parsing/errors/other/expected/spread.res.txt @@ -59,7 +59,7 @@ Solution: you need to pull out each field you want explicitly. 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. `{...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. +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`. @@ -73,7 +73,7 @@ Solution: directly use `concat`. 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!