rescript-compiler icon indicating copy to clipboard operation
rescript-compiler copied to clipboard

Remove lazy keyword

Open namenu opened this issue 10 months ago β€’ 6 comments

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)

namenu avatar Aug 13 '23 15:08 namenu

@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.

cknitt avatar Aug 18 '23 10:08 cknitt

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

cristianoc avatar Aug 18 '23 10:08 cristianoc

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

cknitt avatar Feb 09 '24 17:02 cknitt

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

namenu avatar Feb 19 '24 01:02 namenu

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

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

cknitt avatar Feb 19 '24 07:02 cknitt

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

cknitt avatar Feb 19 '24 16:02 cknitt

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

Done!

namenu avatar Feb 20 '24 05:02 namenu

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 πŸ‘

zth avatar Feb 20 '24 08:02 zth