Skip to content
This repository has been archived by the owner on Jun 15, 2023. It is now read-only.

Feature: relax the constraint of ... in list construction #670

Closed
bobzhang opened this issue Oct 11, 2022 · 5 comments
Closed

Feature: relax the constraint of ... in list construction #670

bobzhang opened this issue Oct 11, 2022 · 5 comments

Comments

@bobzhang
Copy link
Member

bobzhang commented Oct 11, 2022

Currently, ... is only allowed in the tail position, list {1,2,... tail},
For example:

let l = a => list{1,2,...a} // works
let l = a => list{1,2,...a, 3,  ...a} // fails

The current situation is a bit weird, it is not a syntax error, but a type error

This has type: int Somewhere wanted: list<int>

This also seems to be a bug:

let l = (a,b) => list{1,2, ...b , 3,...a}

The inferred type for b is int

@bobzhang bobzhang added the bug Something isn't working label Oct 11, 2022
butterunderflow pushed a commit to butterunderflow/syntax that referenced this issue Oct 12, 2022
@butterunderflow
Copy link
Contributor

This issue seems raised from here.

let exprs = exprs |> List.map snd |> List.rev in

The current behavior is: if the last list element expression is a spread, the parser will not check the rest element whether contains a spread expression. So the list expression

list{...a, ...b ,...a}

would be parsed to OCaml

a :: b :: a

which raises a type error in subsequent type checking.

But

list{...a, ...b, c}

would raise a syntax error, because the last is not a spread.

My commit seems fix this.

Would someone help review my code?

@cristianoc
Copy link
Contributor

Would you create a pull request?

cristianoc added a commit that referenced this issue Oct 13, 2022
* 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]>
@bobzhang
Copy link
Member Author

@butterunderflow
A proposal for fix:
when ... only appears in the tail position, nothing changes, so no regressions would happen.

when multiple ... appears

 list{ a0 , ... b0 , ai, ... bi, }

==> Belt.List.concatMany ([ list{a0, ...b0}, list{ai,... bi}]

This would make a working version first, we could improve the performance later

@bobzhang bobzhang removed the bug Something isn't working label Oct 13, 2022
@butterunderflow
Copy link
Contributor

As far as I understand, after this feature is added, there will be no syntax errors related to wrong spread location, since spread expression at anywhere could be compiled to list concatenation. Do I understand correctly?

@bobzhang
Copy link
Member Author

yes

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants