Improve obj dup using spread syntax

[cometkim]

This removes Caml_obj.obj_dup runtime primitive. The spread syntax can be a perfect alternative to obj_dup, as it doesn't change the key order.

However, this may change FFI's behavior since it no longer handles Arrays separately. it's mostly Arrays extended with additional fields, so upcasting to Object makes sense IMO.

It's not an ideal implementation, but I'd like to merge first to finish #6984. It should be integrated into Caml_block, not Object.

Currently, the output looks like:

-let newrecord = Caml_obj.obj_dup(x);
+let newrecord = {...x};
newrecord.x0 = 1;
return newrecord;

But later:

return {
  ...x,
  x0: 1
};

[cristianoc]

The implementation looks great.
Not sure I understand the comments about FFI.
Changes in current tests seem harmless, but perhaps I'm not fully getting what the concern is.

[cometkim]

Not sure I understand the comments about FFI.
Changes in current tests seem harmless, but perhaps I'm not fully getting what the concern is.

When using %identity of array values, previously the result of obj_dup(array) keep Array class, but now { ...array } cast it to a plain object. The behavior on the ReScript side is fine, but on the JS side the result of Array.isArray will be changed.

[cristianoc]

Not sure I understand the comments about FFI.
Changes in current tests seem harmless, but perhaps I'm not fully getting what the concern is.

When using %identity of array values, previously the result of obj_dup(array) keep Array class, but now { ...array } cast it to a plain object. The behavior on the ReScript side is fine, but on the JS side the result of Array.isArray will be changed.

And was that a desired behaviour in FFI or a programming mistake of mixing arrays and objects, that now will have different semantics?

[cometkim]

Well, the existing obj_dup implementation was clearly handling it intentionally.

One case I found during experiments was Caml_int64.discard_sign,
https://github.com/rescript-lang/rescript-compiler/blob/1bc9e03/jscomp/runtime/caml_int64.res#L524
which has an internal representation of [hi, lo], but is treated as a record in ReScript code. Therefore, the int64 literal and the cloned value will have different prototypes.

In a real-world case, a pattern in JS like Object.assign([ ... ], { extra_fields }) (e.g. RegExpMatchArray), the existing obj_dup is problematic too because it discards extra_fields.

So yes, this PR will change the semantics. We have to choose whether to preserve the prototype of the array or preserve the data.

[cristianoc]

Well, the existing obj_dup implementation was clearly handling it intentionally.

One case I found during experiments was Caml_int64.discard_sign,

https://github.com/rescript-lang/rescript-compiler/blob/1bc9e03/jscomp/runtime/caml_int64.res#L524

which has an internal representation of [hi, lo], but is treated as a record in ReScript code. Therefore, the int64 literal and the cloned value will have different prototypes.

In a real-world case, a pattern in JS like Object.assign([ ... ], { extra_fields }) (e.g. RegExpMatchArray), the existing obj_dup is problematic too because it discards extra_fields.

So yes, this PR will change the semantics. We have to choose whether to preserve the prototype of the array or preserve the data.

Ok so it's a semantic change that removes a hack.
That sounds like a good semantic change.

[cometkim]

I'm merging this to continue work at #6984 , but I have to revisit this to remove newrecord bindings. I need to dive deeper into the compiler backend for that.

[cometkim]

@cknitt @cristianoc Should we note this as a breaking change?

[cknitt]

@cknitt @cristianoc Should we note this as a breaking change?

It is only breaking in the unlikely case that people misused this using %identity on arrays, right?
So from my point of view it is fine in the CHANGELOG as it is.

Thanks a lot for your work!

[cometkim]

It is only breaking in the unlikely case that people misused this using %identity on arrays, right?
So from my point of view it is fine in the CHANGELOG as it is.

Right. IMHO it's very rare that this change is a problem, in fact it often fixes (another) problem, like copying such RegExpMatchArray stuff.