Skip to content
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

Remove lazy keyword #6342

Merged
merged 4 commits into from
Feb 20, 2024
Merged

Remove lazy keyword #6342

merged 4 commits into from
Feb 20, 2024

Conversation

namenu
Copy link
Contributor

@namenu namenu commented Aug 13, 2023

Resolves #6241

This PR removes lazy keyword and modifies itself to use Lazy.from_fun and Lazy.from_val instead.

limitations

The semantics were not fully compatible.

1. let rec syntax

In let rec, lazy is a specially treated grammar. 1

The following syntax could not be converted to Lazy.from_fun.

let rec hamming = lazy Cons(nn1, merge(cmp, ham2, merge(cmp, ham3, ham5)))
and ham2 = lazy force(map(x2, hamming))
and ham3 = lazy force(map(x3, hamming))
and ham5 = lazy force(map(x5, hamming))

Compiler complains with this error:

Error: This kind of expression is not allowed as right-hand side of `let rec’

2. Pattern matching with short-circuit eval

Consider following example.

let foo = (x) => {
  switch (lazy Js.log(1), lazy Js.log(2), x) {
  | (lazy (), _, true) => 0
  | (_, lazy (), false) => 1
  }
}

Generated code prints 2 only when failed to match first case.

This imperative style isn't possible anymore. (and usually better)

Implementing Lazy module

The existing implementation was also using the lazy syntax directly.
This was simulated with deferred function calls. (for more details, see camlinternalLazy.res)

Footnotes

  1. https://v2.ocaml.org/releases/4.14/htmlman/letrecvalues.html

@cknitt
Copy link
Member

cknitt commented Aug 18, 2023

@namenu Thank you for your contribution! 👍 There is still an error when running the tests though - can you have a look at that?

Also note that there is more stuff that can be removed from the compiler later (handling of Pexp_lazy/Texp_lazy/Blk_lazy_general). I actually started doing this some time ago, but never finished it. 🙂 I think this can wait until v12 though as for now it is still possible to produce Pexp_lazy using .ml syntax.

@cristianoc Do we want to include this PR in v11? The advantage would be that we can then have React.lazy in rescript-react.

@cristianoc
Copy link
Collaborator

I would keep it for v12, and only consider non-breaking bug fixes for v11.

@cknitt cknitt added this to the v12 milestone Aug 18, 2023
@cknitt
Copy link
Member

cknitt commented Feb 9, 2024

@namenu Now that v11 is released and master is for v12 development - would you rebase this on the current master?

@namenu namenu marked this pull request as draft February 12, 2024 00:52
@namenu namenu marked this pull request as ready for review February 18, 2024 14:31
@namenu
Copy link
Contributor Author

namenu commented Feb 19, 2024

@cknitt PR is ready, but somehow CI is not running.

@cknitt
Copy link
Member

cknitt commented Feb 19, 2024

@namenu Thanks a lot! I had to approve CI for it to run.

It has some errors, could you have a look at those?

Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks again!

@cristianoc @zth Merge?

@cknitt
Copy link
Member

cknitt commented Feb 19, 2024

Ah, wait - @namenu could you update the changelog, too?

@namenu
Copy link
Contributor Author

namenu commented Feb 20, 2024

Ah, wait - @namenu could you update the changelog, too?

Done!

@zth
Copy link
Collaborator

zth commented Feb 20, 2024

Good to go from my end. @cknitt if you've looked through it and think it's fine then I'm fine with it for sure 👍

@cknitt cknitt merged commit 470d034 into rescript-lang:master Feb 20, 2024
14 checks passed
JonoPrest pushed a commit to JonoPrest/rescript-compiler that referenced this pull request Mar 21, 2024
* remove lazy keyword

* fix contributing doc stating old ocaml ver

* sync test output

* change CHANGELOG
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove lazy keyword
4 participants