unhygienic expansion and unintended normalization in refmt

[sorawee]

Consider:

let f = (x, p) => p(x);
let g = (a, b) => a + b;

let ___x = 42;

Js.log(7->f(g(_, ___x))); /* output 49 */

Now, change ___x to __x:

let f = (x, p) => p(x);
let g = (a, b) => a + b;

let __x = 42;

Js.log(7->f(g(_, __x))); /* output 14 */

The reason is that 7->f(g(_, __x)) is rewritten to 7->f(__x => g(__x, __x)) in the compiler. This causes an unintended variable capture akin to what we can see from unhygienic macros.

A related bug is that refmt normalizes 7->f(g(_, foo)) to 7->f(__x => g(__x, foo)). I argue that it should not do this. Even if the compiler is now correct, that is, it generates a fresh identifier that avoids unintended variable capture, outputting the generated identifier to users doesn't seem to be a good normalization. Furthermore, it doesn't look good aesthetically (although this might be subjective).

[jaredly]

+1 to the g(_, foo) should be preserved by refmt instead of turning it into __x => g(__x, foo). It's a strong disincentive to use that otherwise very handy form.

[cristianoc]

@sorawee there are 2 things in play here:

1. Supporting the notation foo(_, e), which requires recognizing when the AST represents that form.
2. Converting to OCaml valid syntax, and ideally convert back to the same representation.

Currently there's a simplistic convention where a dedicated variable name __x is generated, which breaks when that same variable name is used in the source program.

One could use more elaborate mechanisms to preserve hygiene, such as making sure that the variable is fresh for the relevant scope, and use an annotation in the AST to mark that it represents _.

[alex35mil]

+1 to the g(_, foo) should be preserved by refmt instead of turning it into __x => g(__x, foo). It's a strong disincentive to use that otherwise very handy form.

I don't use it for exactly this reason. But I want to badly.