Regression: escaping of "method" breaks JS interop

[cknitt]

The just released BS 7.0.2-dev.1 with the latest refmt compiles

type request;

[@bs.obj] external request: (~method: string) => request = "";

Js.log2("request", request(~method="GET"));

to

console.log("request", {
      method_: "GET"
    });

I.e. it gives me an object with a field method_. In previous versions, I used to get an object with a field method (as expected).

[cknitt]

(See #2507 and #2513 for previous issues with escaping "method".)

[cknitt]

Also,

let f = x => x##method;

gives

function f(x) {
  return x.method_;
}

[cknitt]

Same for React components, as they are using bs.obj internally:

module SomeComponent = {
  [@react.component]
  let make = (~method) => React.string(method);
};

<SomeComponent method="GET" />;

gives

React.createElement(Teest$SomeComponent, {
      method_: "GET"
    });

[cknitt]

Maybe method should just be forbidden and not escaped? Then the user is forced to use

[@bs.obj] external request: (~_method: string) => request = "";

which works fine.

[anmonteiro]

I don't think this is a bug. We did switch the representation, but it's "correct" now. Is the BuckleScript contract that the generated JS names can be relied upon?

If we revert this change then there are bugs interacting between Reason and OCaml, and IMO those need to remain fixed.

[cknitt]

I don't know if there is any "official" BuckleScript contract for that, but as a user of BuckleScript defining an external method in Reason syntax

[@bs.obj] external request: (~a: string, ~b: string, ~c: string) => t = "";

I definitely expect the resulting JS object to have the fields a, b and c, not "randomly" a, b and c_.

If the compiler told me that I am not allowed to use c because it is a reserved word, that would be fine with me, because then I'd know that something is wrong and could explicitly escape it myself by using _c (which BuckleScript changes to c in the JS output).

[cknitt]

For example,

[@bs.obj] external request: (~type: string) => t = "";

gives me a compile error:

Error: type is a reserved keyword, it cannot be used as an identifier.
Try `type_` or `_type` instead

I then change it to

[@bs.obj] external request: (~_type: string) => t = "";

and

Js.log(request(~_type="GET"));

generates

console.log({
      type: "GET"
    });

Is it not possible to have the same behavior for method / other OCaml keywords?

[bobzhang]

I suggest we have two modes, regular mode and compat mode.
For regular mode, reason users are not supposed to know method is a keyword, there should be no name mangling involved, for compat mode(compatible with vanilla ocaml), this should be a parse error (still no name mangling), since user is expected to know method is an ocaml keyword.
compat mode is a subset of regular mode

[bobzhang]

We are blocked on this issue. The current situation is a bit worse, since it would compile and break user's code silently

[jordwalke]

@bobzhang Is it clear that the name mangling needs to happen even if someone is calling into ocaml code that was not authored in Reason? I believe the name mangling is the right thing to do - and the only reason why we might skip it is just to avoid breakages in existing Reason code. It's not the "right" solution to skip name mangling, but it's the one we might have to do in the mean time.
What if we had a transitionary period until the next release, where we inject compiler warnings when using names like ~method instead of ~method_, so that people have time to fix their labels before we change the compiler output.

[jordwalke]

It's kind of hard to maintain two "modes" of compatibility, but it might be the easiest path forward. The worst break here would be with BuckleScript users who use these with externs which wouldn't issue compiler errors. Users of non-bucklescript Reason tend to use these names in places that aren't used to generate untyped JS objects, so they would usually get an error if calling out to OCaml code and they would know to fix up their code. How should such a compatibility mode be toggled? In the reason parser library? So that bucklescript could set that option before parsing?
I would still want it to inject a warning and provide a way for them to prepare their code for the future - and it still doesn't address the case where a bucklescript user is calling out to ocaml code. What do you suggest we do about that case?

[bobzhang]

@jordwalke As I said, the current name mangling scheme is a bit leaky, reason users should not be expected to know that method is a reserved keyword in OCaml. It is hard to do it right in the source level, we hit several regressions in last couple of releases (below is a non complete list).
rescript-lang/rescript#4008
#2505
#2507
rescript-lang/rescript#3969
#2494

For the two modes, the strict mode is subset of regular mode, so the maintainability is okay(I expect the maintenance is less than keep name mangling)

How should such a compatibility mode be toggled? In the reason parser library?

In general, regular mode is by default, if the reason library is intended to be called by OCaml users in the source level, it should have a strict mode as a sanity check.

Note what I am proposing is not having two modes where one mode does name mangling while the other does not, that would be a nightmare to maintain compatibility.

Edit: another place where such name mangling is leaky:

bin$bsc -i -e 'let method = 3'
let method_: int;

[jordwalke]

Alright. I looked more into it and here are things to consider:
The thing that changed was indeed a fix to make function label escaping match the rest of the escaping that happens in Reason.

~method --> ~method_
~method_ --> ~method__
~method_ --> ~method___

There was however a lot of code before this fix that explicitly used ~method_ because a popular library bs-fetch had a core API with argument named ~method_.
So the fix would then parse peoples' existing Reason code (~method_) as ~method__ which would break usages of bs-fetch (in addition to the [@bs.obj] feature mentioned in this issue.

But we need proper escaping in general for compatibility with OCaml at least when using native.
What would happen if we just turned off all escaping - but just for BS? That could break a lot of people, but not with function named arguments. It would break people who have regular let bindings exported from their modules like let method = ... or let match =....

1. There exists some popular code that uses the token method_ here
   Many of those people would break with Reason master branch. But also some of those people would break if we just turned mangling off completely for Reason. (And some others might as well).

Any time we change name mangling (even to fix an issue) - some people will break, which is why it's important to get these fixes in sooner than later.

But there's one potential saving option. [@bs.obj] special cases underscores that prefix reserved OCaml tokens. For example it will turn [@bs.obj] (~_method...) into {method: ..}.

So my proposal to unblock the BS release is that we use a slightly different escaping convention for labeled names than we use everywhere else. Function label names would then use this convention:

~method --> ~_method_
~_method_ --> ~__method_
~__method_ --> ~___method

(putting the underscore at the beginning instead of the end).

For compatibility with the OCaml ecosystem, all that matters is that there is some complete escaping convention, it doesn't matter too much what that convention is.

But - like any change to name mangling, you could always break someone. Fortunately, in this case, it seems like labels of the form ~_method are much less common. And even those won't necessarily break with my proposed fix, as long as they are calling from Reason to Reason, and not using bs.obj. (Otherwise, they would break because their existing ~_method labels will get interpreted as ~__method and that could influence callers from OCaml into Reason or the shape of their JS objects.

[hhugo]

Isn't this incompatible with punning ?

[mlms13]

As an extra data point, this issue affects out-of-the-box Reason React as well. This previously compiled (e.g. with bsb 5.2.x):

<form method="POST" />

But it stopped compiling with bsb 7.0.x. You can get it to compile by changing method to method_, but it outputs method_ in the JS, which causes the component to not work correctly.

[anmonteiro]

here's an updated summary of this issue:

with rename_labels == false (current state):

let x = (~method) => method

^ this becomes let x ~method:method_ = method_ if you convert to OCaml, but in Melange it generates the correct key in @@mel.obj

with rename_labels == true, we get OCaml compatibility:

let x ~method_  = method_

but then Melange keys get a _ appended to the end, e.g. @@mel.obj with ~method gets a method_ key (as originally reported in the issue)

so:

a) keeping it is sorta bad for OCaml compatibility, but
b) changing it breaks Melange code in the wild in a very bad way (codegen, in a very hidden way)

I guess some hope for this is OCaml 5.2, where you can write \#method for reserved keywords, and I guess in that case we wouldn't even need to rename anything at all when translating Reason code to OCaml, e.g. the same code becomes:

let x ~\#method:method_ = method_

which is maybe ~fine but then we'd need everyone using reason to be on OCaml 5.2+, which may take 10 years