use arrow syntax for anonymous functions

[cometkim]

No description provided.

[cristianoc]

What's the motivation for this? Shorter code?

[cometkim]

Yes. It improves the readability of the output, especially where pipe syntax is used.

There is no difference in the runtime performance between anonymous functions that already allow implicit captures.

However, it might be a breaking change because it implicitly changes the behavior of this, arguments, and the function name of the inner closure. Idiomatic ReScript code should not rely on these.

[cristianoc]

@cometkim would you get feedback in discord?

[zth]

I like this! Just a few thoughts.

Yes. It improves the readability of the output, especially where pipe syntax is used.

Could you add one or two examples of this to the description?

However, it might be a breaking change because it implicitly changes the behavior of this, arguments, and the function name of the inner closure. Idiomatic ReScript code should not rely on these.

While not idiomatic, this type of thing is sometimes necessary/useful. Would it be possible to somehow opt into a regular function like we're emitting now when necessary?

[cometkim]

Would it be possible to somehow opt into a regular function like we're emitting now when necessary?

If is_method=true, it will still emit a regular function and users can use this and arguments. We can explicitly use E.method_ where it is semantically acceptable.

[cometkim]

Could you add one or two examples of this to the description?

There is no big difference. It's just like:

It is also possible to abbreviate blocks with only have single return.

[cknitt]

I like this, too! 😄

In the case of a single argument, could/should we emit x => ... instead of (x) => ...?

[cometkim]

In the case of a single argument, could/should we emit x => ... instead of (x) => ...?

If we want. I already implemented it in my local branch

[cometkim]

@DZakh Can you confirm that this doesn't harm your library perf?

[DZakh]

@DZakh Can you confirm that this doesn't harm your library perf?

I rely on this in a few places, if it's possible to opt-in to the function there, then I can try.

[DZakh]

Also, this change should positively impact the bundlesize of the projects. When I did benchmarks for rescript-schema, I noticed, that esbuild doesn't optimise function to arrow functions, which is noticeable in terms of size.

[cknitt]

While not idiomatic, this type of thing is sometimes necessary/useful. Would it be possible to somehow opt into a regular function like we're emitting now when necessary?

How would this opt-in look like syntactically? Something like

let x = [0, 1]->Array.map(@NoArrow x => x + 1)

?

Maybe this could be an interim solution until all libraries / non-idiomatic code relying on this has been migrated to support arrow function output?

[cometkim]

Yep, I thought the similar (mine was @method)

I also checked if it could be a named function, but it must be anonymous because that would break the behavior.

[DZakh]

Maybe something like this, but for regular functions as well? https://rescript-lang.org/syntax-lookup#this-decorator

[cometkim]

Maybe something like this, but for regular functions as well? https://rescript-lang.org/syntax-lookup#this-decorator

This PR wouldn't change regular function behavior.

I think we can support @this on anonymous functions. But I'm not sure we want to retain this pattern for the long term.

[DZakh]

Maybe something like this, but for regular functions as well? https://rescript-lang.org/syntax-lookup#this-decorator

This PR wouldn't change regular function behavior.

I think we can support @this on anonymous functions. But I'm not sure we want to retain this pattern for the long term.

Missed the fact that the change is only for anonymous functions. Then there's no problem from my side. I don't see a good usecase for this in anonymous functions in ReScript besides Js bindings. And they are already supported by @this

[DZakh]

Maybe something like this, but for regular functions as well? https://rescript-lang.org/syntax-lookup#this-decorator

This PR wouldn't change regular function behavior.
I think we can support @this on anonymous functions. But I'm not sure we want to retain this pattern for the long term.

Missed the fact that the change is only for anonymous functions. Then there's no problem from my side. I don't see a good usecase for this in anonymous functions in ReScript besides Js bindings. And they are already supported by @this

Just need to double check that there are tests for @this and they are working correctly after the change

[cometkim]

Just need to double check that there are tests for @this and they are working correctly after the change

@DZakh @zth Methods using @this binding shouldn't be affected.

Tests are exist here: https://github.com/cometkim/rescript-compiler/blob/emit-arrow/jscomp/test/test_bs_this.js

[cknitt]

In the case of a single argument, could/should we emit x => ... instead of (x) => ...?

If we want. I already implemented it in my local branch

I would want to 😄. What do you say @zth @cristianoc?

[zth]

@cknitt sure!

[cometkim]

@cknitt check it out!

[cometkim]

Fixed it, but it's a heuristic. It would be a better implementation if it could recognize IIFE pattern from the Lambda IR.

[cristianoc]

Just to double check: are there any concerns left about correctness, or have they all been answered?

[cometkim]

Just to double check: are there any concerns left about correctness, or have they all been answered?

If everyone has checked the output, I think it's OK. Concerned parts like IIFE and this binding, are all resolved.

The rest is just to make the output a little more "pretty", but it doesn't have to do with correctness.

E.g.

function findAllAddresses(businesses) {
  return Belt_List.toArray(Belt_List.flatten(Belt_List.fromArray(Belt_Array.map(businesses, business => Belt_List.concat(Belt_Option.mapWithDefault(business.address, /* [] */0, a => ({
    hd: a,
    tl: /* [] */0
  })), Belt_Option.mapWithDefault(business.owner, /* [] */0, p => Belt_Option.mapWithDefault(p.address, /* [] */0, a => ({
    hd: a,
    tl: /* [] */0
  }))))))));
}

function findAllAddresses(businesses) {
  return Belt_List.toArray(
    Belt_List.flatten(
      Belt_List.fromArray(
        Belt_Array.map(
          businesses,
          business => Belt_List.concat(
            Belt_Option.mapWithDefault(
              business.address,
              /* [] */0,
              a => ({
                hd: a,
                tl: /* [] */0
              })
            ),
            Belt_Option.mapWithDefault(
              business.owner,
              /* [] */0,
              p => Belt_Option.mapWithDefault(
                p.address,
                /* [] */0,
                a => ({
                  hd: a,
                  tl: /* [] */0
                })
              )
            )
          )
        )
      )
    )
  );
}

[DZakh]

About not wrapping args with () I personally things it's better with paranthes and this is also how Prettier formats the code.

[cknitt]

About not wrapping args with () I personally things it's better with paranthes and this is also how Prettier formats the code.

Yes, but

• it's not the way the ReScript printer does it
• for me it's extra noise
• the only advantage of (x) => ... I can see is that it is easier to manually add a second argument, but that should not be a concern for the compiler output.

[cknitt]

Did not notice any further issues in the compiler output.
Great work, thanks a lot! 👍

[DZakh]

it's not the way the ReScript printer does it

Since it's JavaScript, I think it should be formatted as idiomatic JavaScript. And even though it's an extra noise, the majority of Js devs already got used to it and find it more natural to see paranthes than not.

[cknitt]

Maybe I am biased because I always have

  arrowParens: "avoid",

in my prettier config.

[cknitt]

Anyway, how do we come to a decision regarding the arrow parens?
As preferences seem to differ. 🙂

[cometkim]

Maybe I am biased because I always have

  arrowParens: "avoid",

in my prettier config.

So is mine. Most of the projects I'm involved with use the style of avoiding it. It's more of a personal preference.

But I can agree with the always style. I don't think it's worth increasing the compiler complexity to avoid it.

[cometkim]

I'll leave it as is, unless there are other objections. The complexity I mentioned is trivial.

Changing x => x to (x) => x for all outputs is also noisy to me 😅

[cristianoc]

No special opinions as long as it does not break anything, and people are generally happy.