Clone with

[TimWolla]

No description provided.

[arnaud-lb]

This looks good to me in general!

Possibly, clone $obj as an opcode is faster than clone($obj) as a function call (no function resolution, no argument passing, no frame creation). A benchmark may be worth it. Maybe we should keep the opcode handler for the simple case of clone $obj for now, and optimize clone-with later?

If the function took a single argument named with, we could optimize clone($obj, $props) and clone($obj, with: $props) to a 2-ary frameless call. This would be nearly as fast as the opcode, and would require only minimal support from the compiler.

Another benefit of passing props to a dedicated parameter is that it allows us to add other parameters in the future. Also it feels nicer and more explicit to me:

clone($obj, with: [
   'foo' => $bar,
]);

Otherwise, it may be possible to compile clone($obj, foo: $bar, bar: $baz) to

T0 = CV($obj)
T1 = INIT_ARRAY CV($bar), string("foo")
T1 = ADD_ARRAY_ELEMENT CV($baz), string("bar")
CLONE T0, T1  

for simple arg lists with only named params.

Of course this can be done after the RFC is accepted, but it may be worth it to check that optimizations are possible before that, as sometimes the semantics preclude them.

[arnaud-lb]

Ouch :)

clone ($expr) vs clone($arg) ?

Ideally we should resolve the conflict to revert %expect to 0 again, as it will make future changes easier. I'm not sure how, however.

[TimWolla]

clone ($expr) vs clone($arg) ?

Yes.

Ideally we should resolve the conflict to revert %expect to 0 again, as it will make future changes easier. I'm not sure how, however.

The initial OPcode-based implementation had this already resolved: 4e5e58b. During the implementation of the function-based implementation I then used the %expect 1 hammer to not spend too much time with the parser in case it turns out it wouldn't work anyways.

I will definitely clean this up for the final implementation / official PR, should be reasonably straight-forward (just perhaps a little verbose).

[TimWolla]

Another benefit of passing props to a dedicated parameter is that it allows us to add other parameters in the future. Also it feels nicer and more explicit to me:

During the discussion of the initial clone … with … proposal from Máté folks were pretty unhappy with the array-based syntax (e.g. due to the need to quote the property names). That's why this one uses the named parameter-syntax instead.

Being able to add additional parameters in the future is a valid point, but I have a hard time of coming up with something other than properties to reassign that would be useful. At the point where you are interested in e.g. calling methods during the cloning process, you should like stop and reconsider what you are doing.

/cc @edorian

Maybe we should keep the opcode handler for the simple case of clone $obj for now, and optimize clone-with later?

Yes, having both the OPcode and the function is also something I was considering, but I wanted to get it functional first 😄

Otherwise, it may be possible to compile clone($obj, foo: $bar, bar: $baz) to

Yes. That's what I was doing in: 711020e and b527f3b, before I switched to the function-based implementation.

The OPcode-based implementation worked nicely for the simple cases (named parameters or array destructuring), but supporting arbitary iterables would require reimplementing quite a bit of logic that is already implemented for function calls.

If we keep both the OPcode and the function, we could possibly compile the named-parameter version to the OPcode and all the other cases to the function, though.

[arnaud-lb]

Maybe this has already been mentioned, but one issue with the signature function clone(object $object, mixed ...$updatedProperties): object {} is that it's not possible to update a property named object:

class C {
    public $object;
}
clone(new C(), object: 'foo');
// Uncaught Error: Named parameter $object overwrites previous argument

If we keep both the OPcode and the function, we could possibly compile the named-parameter version to the OPcode and all the other cases to the function, though.

I agree :) This is what I was thinking

[TimWolla]

Maybe this has already been mentioned, but one issue with the signature function clone(object $object, mixed ...$updatedProperties): object {} is that it's not possible to update a property named object:

Ah, good catch. I did not realize this yet. Thanks!

We'll make sure to list this in the RFC so that it can be properly discussed on Internals. Personally I would be fine with both using a “mangled” parameter name for the first parameter to make collisions less likely (and documenting that as a limitation) and also with making it a regular 2-parameter function that would also come with the implementation benefits that you mentioned.