Don't emit f(undefined) when passing unit ().

[cristianoc]

This changes code generation so it emits f() instead of f(undefined) when the function takes a single argument and its value is undefined.

Related companion PR: #6131

The current check distinguishes passing () vs passing e.g. undefined, and omits the argument only in the former case.

[cristianoc]

This comes from an issue reported by @sgrove, who can elaborate.

The RFC part is whether this change could cause other issues.

@cknitt @zth any thoughts about when this could be helpful, and un-helpful

It's possible to consider narrowing down the cases further.

[cknitt]

I am very much in favor of removing the superfluous undefineds for functions with a single unit argument. Less JS code created, and one less thing to explain to people looking at the JS output. Can't really think of a case where this might cause troubles.

Notice for simplicity there's currently no check in the implementation that the one argument is statically known to have type unit, it's only very often the case.

This means that (an example from the PR changes)

g.curr = Some(None)

used to get compiled to

g.curr = Caml_option.some(undefined);

but is now

g.curr = Caml_option.some();

This will still work, but somehow it goes against the meaning of the original ReScript code, as undefined stands for None here, not (). So I think it would be good to statically check that the argument type is unit. A similar situation would be when you pass a Belt.Map.empty to a unary function.

[sgoai]

The current behavior caused an issue with emscripten-produced ffi code that ends up looking something like:

function test (...args) {
   if (args.length === 0) {
       proceed()
   } else {
       setSomeOtherVariableTo(args[0])
       proceed()
   }   
}

It's not currently possible with Rescript ffi bindings to produce an invocation to test that hits the first path, and in my case caused some surprising errors. After tracking down the offending ffi code, I used the %raw escape hatch to work around it (great to have escape hatches!).

In general I agree with @cknitt that the presence of undefined for a zero-arity binding isn't very intuitive for most people reading the output JS, and it causes unexpected bugs at worst (who would think that test() and test(undefined) would trigger different paths?!).

[cristianoc]

I am very much in favor of removing the superfluous undefineds for functions with a single unit argument. Less JS code created, and one less thing to explain to people looking at the JS output. Can't really think of a case where this might cause troubles.

Notice for simplicity there's currently no check in the implementation that the one argument is statically known to have type unit, it's only very often the case.

This means that (an example from the PR changes)

g.curr = Some(None)

used to get compiled to

g.curr = Caml_option.some(undefined);

but is now

g.curr = Caml_option.some();

This will still work, but somehow it goes against the meaning of the original ReScript code, as undefined stands for None here, not (). So I think it would be good to statically check that the argument type is unit. A similar situation would be when you pass a Belt.Map.empty to a unary function.

Added check so that only passing () triggers this PR.

Still wondering if there are cases where this behaviour is undesirable, so we do know. Something where the number of arguments is counted explicitly on the callee site, and this behaviour is not the expected one. It's clear that any such case would be a corner case, but curious about what those corner cases are.

One could argue that right now there's no way to pass a unit argument to a function of arity 1. The question is whether there's ever such a need.

[zth]

If it distinguishes unit then I can't think of any potential issue with this, at least not OTOH. Will mull on it a bit more. I'd say the resulting code from this is much more what I'd expect the output to be (omitting undefined).