Add flag rename_labels to toggle renaming of labels in function defin… #2530
Conversation
…ition/application and records.
So they are mangled or not? I wish we have some consistency (instead of make it configurable) |
| @@ -10,22 +10,22 @@ module V = { | |||
|
|
|||
| /* Record fields */ | |||
| module R = { | |||
| type r = {mutable method: int}; | |||
| type r = {mutable method_: int}; | |||
There was a problem hiding this comment.
Asked offline. I didn't think that you wanted to revert the record field mangling. Was that intentional?
There was a problem hiding this comment.
With records being used in place of objects, a difference in mangling would come up pretty soon.
To be on the safe side, I've kept refmt from bs 6.2.1 as a reference.
There are still a couple of difference to take into account, if one wants to have something that behaves like a revert. E.g. mangling of the name in type declarations.
There was a problem hiding this comment.
On the native side we pretty much want all the mangling we can get for greater compatibility. On the bs side, you can disable whatever you like under a flag. I just wasn't sure if you wanted something that behaved like a perfect revert. If you're fine with it, go ahead and merge.
| @@ -24,6 +24,9 @@ open Ast_mapper | |||
| open Parsetree | |||
| open Longident | |||
|
|
|||
| (* Rename labels in function definition/application and records *) | |||
| let rename_labels = ref false | |||
There was a problem hiding this comment.
We're going to flip this to true for non-bs usage pretty soon. Is that going to be easy to coordinate with BS in the next next update? Would it be easier to just flip this to true, and let BS flip it to false from the outside? Don't need to block on that decision, but just curious if there's a default that can be set here that makes it easier later.
There was a problem hiding this comment.
Sure flipping on the bs side can be done. One other question is what to do longer term with tests, if bs and native behave differently.
There was a problem hiding this comment.
This is gonna be my last comment in this discussion, since it seems like my concerns aren't really getting heard: I strongly disagree that BuckleScript and Reason should behave differently in the long term. This is introducing a dangerous precedent for deviating even more from OCaml.
I guess I'm OK with this PR to go in right now to fix the immediate issue of the breaking change we accidentally introduced. But I'm very strongly in favor or moving the ecosystem towards a world where we do output an OCaml compatible AST.
There was a problem hiding this comment.
The longer term situation needs to be looked at carefully, and will take more time.
One of the proposals is to have a compatibility mode, where you don't get to use keywords from the other language at all. In that proposal, bs and native would behave identically.
One evolution of the compatibility mode is to also ensure that platform-specific libraries/features are not used. So Js.* can't be used in code shared between Js and native, and you get an error/warning rather than having to think about it yourself. This is kind of the dual approach to trying to unify libraries: let libraries specialize optimally in their domain when it's important, but document the differences.
There was a problem hiding this comment.
Let's merge this PR just to unbreak people, but we can continue the discussion on this thread about what to do next.
…ition/application and records.
The flag
rename_labelscan be used to turn on/off renaming of labels for function definition/application (i.e. the labels of named arguments) and record creation/access (i.e. the labels of records).Issues:
Relevant diffs: