Spread anywhere feature #692

butterunderflow · 2022-10-20T09:00:46Z

Feature request from #670.
The new feature will allow spread anywhere in a list creation expression.
For example, list creation like

let _ = list{1, 2, 3, ...x, 4, 5, ...y, 6}

will no longer trigger an error anymore, but parsed to

let _ = Belt.List.concatMany [|(1 :: 2 :: 3 :: x);(4 :: 5 :: y);[6]|]

Can anyone help review this feature PR? Thanks ^v^

cristianoc · 2022-10-20T09:45:14Z

Would you add printing tests?

butterunderflow · 2022-10-20T10:29:40Z

What is printing tests stand for? Like printing some parse results related to this feature?

cristianoc · 2022-10-20T10:44:13Z

Add a file in tests/print or extend an existing one where list examples are.

It will tell you how things print, and whether printing is idempotent, so you don't get different results if you format twice.

butterunderflow · 2022-10-20T10:56:16Z

Thanks, I think I understand. I'll give it a try.

cristianoc · 2022-10-20T12:17:04Z

See how the syntax of normal lists is preserved by printing.
A similar mechanism is needed for this extension to lists.

namenu · 2022-10-20T16:29:44Z

tiny concern, but would it be better not allowing this expression at all ?

lists are not okay when manipulation is occurred at non-head position.

Once spreadings are used inadvertently, it would cause performance problem.

By now, list spreading guarantees constant time

butterunderflow · 2022-10-20T20:01:23Z

IMHO, it's still a useful feature. For some reason.
First, IMHO again, list concatenation is a little bit verbose at present. We have to write some prefixes of Module.

Belt.List.concatMany([list{1, 2, ...x}, list{3, ...x}, list{4, ...x}, list{5}])
// or 
Belt.List.concat(list{1, 2, ...x}, list{3, ...x})
// or 
List.concat(list{list{1, 2, ...x}, list{3, ...x}})

Second, I think if we support @ operator (like OCaml), the verbose issues will be moderated. But for the existence of list open token list{, looks still not concise enough. Like we have to tell the reader repeatedly "it's a list".

// pretend we have operator @ 
list{1, 2, ...x} @ list{3, ...x} @ list{4, ...x} @ list{5}
list{1,2, ...x} @ list{3, ...x}

If we let spread show anywhere, we can just write:

list{1, 2, ...x, 3, ...x, 4, ...x, 5}
list{1, 2, ...x, 3, ...x}

I think that's more concise, right?

And, as far as I know, literally, the word "spread" itself is not intuitively associated with constant time operation, unlike "prepend".

Finally, the performance issue you mentioned does exist while this feature is being misused. But this feature will not downside any performance of existing code, and I think (I'm not sure) concatenate seems can be optimized in some way. So I think it's still a good deal. I'll always wait for more suggestions.

IwanKaramazow · 2022-10-21T06:19:49Z

Before adding this to the language, we should take a closer look at the design of this feature.
What is the reach of this problem? There are 3 vectors:

volume of GH issues, forum posts, twitter etc…
how blocking is this problem if the developer encounters it
how many of the total ReScript devs does it affect in their day to day work?

When making design decisions regarding new syntax in the language, it’s also important to weigh the costs, not all of which are performance, against the potential benefits. The following things can happen:

The language gets larger.
The language gets slower.
The documentation becomes more extensive.
We're spending time developing new syntax, rather than refining other parts of the language.
We run the risk of introducing changes that don't play well with existing syntax. (Why don't we allow this for array then?)

I feel like this more design work needed before going into implementation mode.

bobzhang · 2022-10-21T11:29:35Z

Let me explain a bit why this feature it is useful and why you should not worry about performance.

This feature is very helpful for expressing lots of algorithms, for example

Add(e1,e2) => list {... compile(e1), ... compile(e2), Add }

It is much more readable than the alternatives, and as efficient as you could, in theory the compiler can do
a better job than you since it kows the compile is a temporay variable and can do some smart mutations.

So the argumets are:

You should avoid list if you care about performance
If you want to concate two lists, the alternative is not better at performance, you have to express this pattern. Simply disabling this syntax does not mean you can avoid the problem, it does not give a solution. JavaScript is also more consistent here

Before we add this feature, this used to be a bug, the syntax was accepted but not reported error, it is worse so
that we are not enriching the syntax: we are fixing existing issues.

With regard to developer resources, this is the feature I would like to add on day one when making BuckleScript,
finally, I have some dedicated resources to make it possible, it also makes the sample code for PL course in ReScript much more readable, instructions concatenation are everywhere.

Hope my explanantions make the motivation clear.

Edit: note this implementation should be improved, @butterunderflow no regressions should happen, x::y should be interpreted as it is, only some invalid patterns are now accepted -- it was silently ignored, so this PR should be a strict better improvement

bobzhang · 2022-10-21T11:38:00Z

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

@@ -114,7 +100,7 @@ let arr = [|x;y|]
 let [|arr;_|] = [|1;2;3|]
 let record = { x with y }
 let { x; y } = myRecord
-let myList = x :: y
+let myList = Belt.List.concatMany [|x;y|]


this case should be specialized?

this case too { ... x}

bobzhang · 2022-10-21T11:39:11Z

tests/printer/expr/expected/list.res.txt

@@ -2,6 +2,7 @@ let x = list{}
 let x = list{1}
 let x = list{1, 2}
 let x = list{1, 2, 3}
+let x = Belt.List.concatMany([list{1, 2, ...x}, list{3, ...x}])



could this be improved? print it as list{ 1, 2, ...x , 3 , ...x}

It could be, but just by changing the printer can cause some problems. For example, list{. . x, . .y} and Belt.List.concatMany([x, y]) will be printed as the same thing. I am still trying to solve this problem.

can you add an attribute to Belt.List.concatMany to avoid accidental simplifications?

This is what is done for string templates, I believe, to differentiate from normal string concatenation.

Check what isTemplateLiteral does.

Thank you for all your suggestions, I'll learn more about that.

butterunderflow · 2022-10-21T16:46:46Z

Made some changes to ensure that the printer output and source are the same. I would have to ask you to review it again, thanks!

IwanKaramazow · 2022-10-21T18:20:44Z

Thanks for the context @bobzhang 👍

bobzhang · 2022-10-24T06:40:28Z

src/res_core.ml

+(* | (true (\* spread expression *\), expr, _) :: exprs ->
+ *   let exprs = check_all_non_spread_exp exprs in
+ *   makeListExpression loc exprs (Some expr)
+ * | exprs ->


instead of comment unused code, can you just remove it

bobzhang · 2022-10-24T06:41:23Z

src/res_parsetree_viewer.ml

+            (Longident.Ldot (Longident.Lident "Belt", "List"), "concatMany");
+      }
+    when hasSpreadAttr expr.pexp_attributes ->
+    true


simplified: -> hasSpreadAttr exp.pexp_attributes

bobzhang · 2022-10-24T06:43:24Z

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

+  6 │ 
+  7 │ let list{...x, ...y} = myList
+  8 │ 
+  9 │ 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 lists; out of performance concern, our pattern matching currently guarantees to never create new intermediate data.


we may need update the docs about pattern in later commits, it is not supported since it is not deterministic

bobzhang · 2022-10-24T06:47:15Z

tests/printer/expr/expected/list.res.txt

@@ -2,6 +2,8 @@ let x = list{}
 let x = list{1}
 let x = list{1, 2}
 let x = list{1, 2, 3}
+let x = list{1, 2, ...x, 3, ...x}
+let x = Belt.List.concatMany([list{1, 2, ...x}, [list{3, ...x}]])

 let x = list{


can you add some more test cases, some edge cases like:

let x = list{ ...xs} let x = list{1, ...xs} let x = list{xs, ... ys} let x = list{ ...xs, ... ys}

bobzhang · 2022-10-24T06:48:22Z

@butterunderflow looks good to me once the comments are addressed.
Note Belt.List.concatMany is an implementation detail, in theory, we could swap more efficient ones in the future

butterunderflow · 2022-10-24T09:17:01Z

Done. Please check again, thanks!

bobzhang · 2022-10-25T01:29:17Z

@butterunderflow Thanks, cannot wait for to use it in the course!

cristianoc · 2022-10-25T06:25:29Z

@butterunderflow would you add a line to CHANGELOG.md

butterunderflow · 2022-10-25T07:28:58Z

Done. Please check #698

butterunderflow added 4 commits October 20, 2022 15:03

let spread at anywhere when creating a list

0447df1

concat accept an array; tests update

3f77897

intead by fold_left to suppress reanalyze termination check

769d719

remove unused error message

b34ec30

butterunderflow marked this pull request as ready for review October 20, 2022 09:12

printing test for spread anywhere feature

3459747

butterunderflow force-pushed the spread-anywhere-feature branch from c7969a8 to 3459747 Compare October 20, 2022 19:50

bobzhang reviewed Oct 21, 2022

View reviewed changes

butterunderflow added 7 commits October 21, 2022 23:53

add attribute for Belt.List.concatMany

cefe805

tweak

4733641

printer tests update

72409f1

tests

362fca5

format

f14b41e

add attribute to Belt.List.concatMany to improve printer

9a81c95

remove non-error test-cases from error

00226bf

pull out long branch to enhance readability

ecc4d40

bobzhang reviewed Oct 24, 2022

View reviewed changes

butterunderflow added 4 commits October 24, 2022 16:44

remove useless comments

001501f

simplify pattern match guard

0e005c0

more tests about new feature

61ac172

one more edge test

e71a61a

fix typo in comment

839d3e7

bobzhang merged commit 402b9c8 into rescript-lang:master Oct 25, 2022

cristianoc mentioned this pull request Oct 30, 2022

Create v10.1 docs rescript-lang/rescript-lang.org#568

Closed

57 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spread anywhere feature #692

Spread anywhere feature #692

butterunderflow commented Oct 20, 2022

cristianoc commented Oct 20, 2022

butterunderflow commented Oct 20, 2022

cristianoc commented Oct 20, 2022

butterunderflow commented Oct 20, 2022

cristianoc commented Oct 20, 2022

namenu commented Oct 20, 2022

butterunderflow commented Oct 20, 2022

IwanKaramazow commented Oct 21, 2022 •

edited

Loading

bobzhang commented Oct 21, 2022 •

edited

Loading

bobzhang Oct 21, 2022

bobzhang Oct 21, 2022

butterunderflow Oct 21, 2022

bobzhang Oct 21, 2022

cristianoc Oct 21, 2022

cristianoc Oct 21, 2022

butterunderflow Oct 21, 2022

butterunderflow commented Oct 21, 2022

IwanKaramazow commented Oct 21, 2022

bobzhang Oct 24, 2022

bobzhang Oct 24, 2022

bobzhang Oct 24, 2022

bobzhang Oct 24, 2022

bobzhang commented Oct 24, 2022

butterunderflow commented Oct 24, 2022

bobzhang commented Oct 25, 2022

cristianoc commented Oct 25, 2022

butterunderflow commented Oct 25, 2022

Spread anywhere feature #692

Spread anywhere feature #692

Conversation

butterunderflow commented Oct 20, 2022

cristianoc commented Oct 20, 2022

butterunderflow commented Oct 20, 2022

cristianoc commented Oct 20, 2022

butterunderflow commented Oct 20, 2022

cristianoc commented Oct 20, 2022

namenu commented Oct 20, 2022

butterunderflow commented Oct 20, 2022

IwanKaramazow commented Oct 21, 2022 • edited Loading

bobzhang commented Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

butterunderflow commented Oct 21, 2022

IwanKaramazow commented Oct 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobzhang commented Oct 24, 2022

butterunderflow commented Oct 24, 2022

bobzhang commented Oct 25, 2022

cristianoc commented Oct 25, 2022

butterunderflow commented Oct 25, 2022

IwanKaramazow commented Oct 21, 2022 •

edited

Loading

bobzhang commented Oct 21, 2022 •

edited

Loading