[RFC] [Discussion] Clone with

Alexandru Pătrănescu

3 years ago

On Mon, Apr 17, 2023, 07:32 Máté Kocsis <kocsismate90@gmail.com> wrote:

> finally I managed to create a working implementation for this feature which
> would make it possible to properly modify readonly properties
> while simplifying how we write "wither" methods:
> https://wiki.php.net/rfc/clone_with

Hey Máté,

How about just allowing a block of code after the clone statement that
would execute it in the same context as the clone context, allowing to
modify once readonly variables.
Allows better flexibility compared with clone with syntax or clone metho:

public function withStatus($code, $reasonPhrase = ''): Response
{
return clone $this {
$this->statusCode = $code;

$this->reasonPhrase = $reasonPhrase;

};

Regards,
Alex

Larry Garfield

3 years ago

On Mon, Apr 17, 2023, at 7:43 AM, Alexandru Pătrănescu wrote:

> On Mon, Apr 17, 2023, 07:32 Máté Kocsis <kocsismate90@gmail.com> wrote:
>
>> finally I managed to create a working implementation for this feature which
>> would make it possible to properly modify readonly properties
>> while simplifying how we write "wither" methods:
>> https://wiki.php.net/rfc/clone_with

A solid RFC, thanks!

I agree with the discussion of Nicolas's alternative, which has its merits but also some possible limitations.  My only major concern myself is the potential confusion between when to use : and when to use =>.  Using the => version only for dynamic keys feels very clunky.  In fact, I think the RFC is inconsistent on this front:

$object = clone $object with {"foo" => 1}; // the property name is a literal
$object = clone $object with {strtolower("FO") . "o" => 1}; // the property name is an expression
$object = clone $object with {PROPERTY_NAME => 1}; // the property name is a named constant

I would expect the first one to be a colon, not fat-arrow.  Is that a typo?

Is there a technical reason why they can't just all use a colon?  That would extend more nicely to supporting dynamic keys in the future for named arguments.

> Hey Máté,
>
> How about just allowing a block of code after the clone statement that
> would execute it in the same context as the clone context, allowing to
> modify once readonly variables.
> Allows better flexibility compared with clone with syntax or clone metho:
>
>     public function withStatus($code, $reasonPhrase = ''): Response
>     {
>         return clone $this {
>             $this->statusCode = $code;
>
>            $this->reasonPhrase = $reasonPhrase;
>
> };

This is an interesting alternative.  I don't think $this is that confusing, actually.  Inside __clone(), $this always refers to the new object.  This proposal would essentially be providing an additional __clone block that runs in the same context, viz, $this always refers to the new object.  I'm not sure if you need the original object separately at all, in fact; if you want to look up a value from the original object... you already have it on the new object in the same location, because it's a clone.  So:

public function withStatus($code, $reasonPhrase = ''): Response
{
return clone $this {
$this->statusCode = $code;
$this->reasonPhrase = "Old: $this->reasonPhrase, New: $reasonPhrase";
}
};

$this->reasonPhrase in the string refers to the cloned object, where that value is by definition identical to the original object.  It's evaluated, the string is computed, and then the clone's reasonPhrase property is updated.

The catch here is 2 fold:

1. This would essentially imply auto-capture.  While I am quite OK with that, a fair number of people are not.  But adding a use() block to that syntax just makes it very clunky for the obvious common case; which is basically the above, so it just forces you to repeat each variable name a third time.  I would probably vote against something that forced more redundant use() statements.

2. The scope permissioning becomes weirder.  The current RFC syntax is very predictable: properties get assigned from the context at which the clone statement lives, and if there are private/protected restrictions those are self-evident.  With the more closure-like approach, it "feels" like you're providing an anon function that will get bound to the new object and then executed... which would then always run with private scope and be able to modify values you probably didn't want to be publicly modifiable.  While we could potentially implement it such that the scope rules are still enforced, that may not be as obvious why you can/can't mess with a particular variable.

I'm open to this alternative though if Mate is; it has potential, but we'd have to sort out the above.

--Larry Garfield

Tim Düsterhus

3 years ago

Hi

On 4/18/23 17:45, Larry Garfield wrote:

> I agree with the discussion of Nicolas's alternative, which has its merits but also some possible limitations.  My only major concern myself is the potential confusion between when to use : and when to use =>.  Using the => version only for dynamic keys feels very clunky.  In fact, I think the RFC is inconsistent on this front:
>
> $object = clone $object with {"foo" => 1}; // the property name is a literal
> $object = clone $object with {strtolower("FO") . "o" => 1}; // the property name is an expression
> $object = clone $object with {PROPERTY_NAME => 1}; // the property name is a named constant
>
> I would expect the first one to be a colon, not fat-arrow.  Is that a typo?

No, that is correct. Colon takes a bare identifier, fat-arrow takes an
expression (that just happens to be a constant expression in that case).

> Is there a technical reason why they can't just all use a colon?  That would extend more nicely to supporting dynamic keys in the future for named arguments.

I'd rather see only the fat-arrow being allowed. Unless I'm missing
something, braces with colon is not used anywhere else, whereas braces +
'=>' is known from match() and '=>' more generally is already used with
array literals [1]. Having two similar syntaxes for the same thing is
not great when both are commonly needed is not great. They need to be
documented and learned by developers.

Best regards
Tim Düsterhus

[1] In fact if the right hand side of with may be an expression that
evaluates to an array, folks wouldn't need to learn new syntax at all:

$newProperties = [ "foo" => "bar" ];
clone $object with $newProperties;

and

clone $object with [ "foo" => "bar" ];

Bob Magic

3 years ago

> [1] In fact if the right hand side of with may be an expression that
> evaluates to an array, folks wouldn't need to learn new syntax at all:
>
>     $newProperties = [ "foo" => "bar" ];
>     clone $object with $newProperties;
>
> and
>
>     clone $object with [ "foo" => "bar" ];

in my opinion this is sick, as in awesome. the {} version makes no sense to
me unless the language gains $var = {} ; to skip $var = (object)[];

Andreas Hennings

3 years ago

On Tue, 18 Apr 2023 at 22:01, Bob Magic <bob@majdak.net> wrote:

>
> > [1] In fact if the right hand side of with may be an expression that
> > evaluates to an array, folks wouldn't need to learn new syntax at all:
> >
> >     $newProperties = [ "foo" => "bar" ];
> >     clone $object with $newProperties;
> >
> > and
> >
> >     clone $object with [ "foo" => "bar" ];
>
> in my opinion this is sick, as in awesome. the {} version makes no sense to
> me unless the language gains $var = {} ; to skip $var = (object)[];

I think the {...} is meant to be simply an "argument list", not a value.
But yes having an array here would add some flexibility. It would also
give us argument unpacking for free.

I wonder what this means for performance.
Will the expression always be evaluated as an array first, and then
applied, or can php do a "shortcut" where it internally treats it as
an argument list, even though the syntax implies array?
The different will be small, but it could become relevant if called
many times over.

-- Andreas

Tim Düsterhus

3 years ago

Hi

On 4/19/23 17:26, Andreas Hennings wrote:

> I wonder what this means for performance.
> Will the expression always be evaluated as an array first, and then
> applied, or can php do a "shortcut" where it internally treats it as
> an argument list, even though the syntax implies array?
> The different will be small, but it could become relevant if called
> many times over.

I've said it before, but I'm going to say it again: Performance should
be low in the list of things that decide how a feature will look.
Opcache and the optimizer can always be improved in the future, syntax
cannot.

Of course if a feature is unacceptably slow, it is a non-starter. But in
this case I expect it to just be "there is optimization potential", as
creating an array with a handful of elements shouldn't be unacceptably
slow and also 'clone with' is likely going to make up only a very small
part of a request's entire processing.

Best regards
Tim Düsterhus

Rowan Collins

3 years ago

On 18 April 2023 17:48:00 BST, "Tim Düsterhus" <tim@bastelstu.be> wrote:

>I'd rather see only the fat-arrow being allowed. Unless I'm missing something, braces with colon is not used anywhere else, whereas braces + '=>' is known from match() and '=>' more generally is already used with array literals [1]. Having two similar syntaxes for the same thing is not great when both are commonly needed is not great. They need to be documented and learned by developers.

I think it makes sense to have an unquoted form here, because the common case is that they are names which analysers can match statically to particular properties, not strings which will be analysed at runtime. There are plenty of places in the language where dynamic names are allowed, but we don't just use strings for the static case:

${'foo'} = 'bar'( constant('BAZ') )->{'quux'}();

More specifically, the "name: $value" syntax matches named parameters, and while you can use an array for that (via ... unpacking), we don't force users to do so.

In fact, the Future Scope of that RFC considered the opposite: using the colon syntax in arrays, along with a shorthand for pulling the key name from the variable name: https://wiki.php.net/rfc/named_params#shorthand_syntax_for_matching_parameter_and_variable_name

>  If I wanted to put these ideas into a general framework, I think one way to go about this would be as follows:
>
> - Consider identifier: $expr as a shorthand for "identifier" => $expr.
> - Consider :$variable as a shorthand for variable: $variable and thus "variable" => $variable.

That would give us:

$point = ['x' => $x, 'y' => $y, 'z' => $z];
$point = [x: $x, y: $y, z: $z];
$point = [:$x, :$y, :$z];

$point = new Point(...['x' => $x, 'y' => $y, 'z' => $z]);
$point = new Point(x: $x, y: $y, z: $z);
$point = new Point(:$x, :$y, :$z);

$point = clone $point with {'x' => $x, 'y' => $y, 'z' => $z};
$point = clone $point with {x: $x, y: $y, z: $z};
$point = clone $point with {:$x, :$y, :$z};

Rather than making everything use an array or array-like syntax, I would probably go the other way and scrap the special syntax for dynamic names, making the whole thing look like a function call, with support for array unpacking:

$point = clone $point with (x: $x, y: $y, z: $z);
$point = clone $point with (...['x' => $x, 'y' => $y, 'z' => $z]);

Regards,

--
Rowan Tommins
[IMSoP]

Robert Landers

3 years ago

On Thu, Apr 20, 2023 at 9:01 AM Rowan Tommins <rowan.collins@gmail.com> wrote:

>
> On 18 April 2023 17:48:00 BST, "Tim Düsterhus" <tim@bastelstu.be> wrote:
> >I'd rather see only the fat-arrow being allowed. Unless I'm missing something, braces with colon is not used anywhere else, whereas braces + '=>' is known from match() and '=>' more generally is already used with array literals [1]. Having two similar syntaxes for the same thing is not great when both are commonly needed is not great. They need to be documented and learned by developers.
>
>
> I think it makes sense to have an unquoted form here, because the common case is that they are names which analysers can match statically to particular properties, not strings which will be analysed at runtime. There are plenty of places in the language where dynamic names are allowed, but we don't just use strings for the static case:
>
> ${'foo'} = 'bar'( constant('BAZ') )->{'quux'}();
>
>
> More specifically, the "name: $value" syntax matches named parameters, and while you can use an array for that (via ... unpacking), we don't force users to do so.
>
> In fact, the Future Scope of that RFC considered the opposite: using the colon syntax in arrays, along with a shorthand for pulling the key name from the variable name: https://wiki.php.net/rfc/named_params#shorthand_syntax_for_matching_parameter_and_variable_name
>
> >  If I wanted to put these ideas into a general framework, I think one way to go about this would be as follows:
> >
> > - Consider identifier: $expr as a shorthand for "identifier" => $expr.
>  > - Consider :$variable as a shorthand for variable: $variable and thus "variable" => $variable.
>
>
> That would give us:
>
> $point = ['x' => $x, 'y' => $y, 'z' => $z];
> $point = [x: $x, y: $y, z: $z];
> $point = [:$x, :$y, :$z];
>
> $point = new Point(...['x' => $x, 'y' => $y, 'z' => $z]);
> $point = new Point(x: $x, y: $y, z: $z);
> $point = new Point(:$x, :$y, :$z);
>
> $point = clone $point with {'x' => $x, 'y' => $y, 'z' => $z};
> $point = clone $point with {x: $x, y: $y, z: $z};
> $point = clone $point with {:$x, :$y, :$z};
>
>
>
> Rather than making everything use an array or array-like syntax, I would probably go the other way and scrap the special syntax for dynamic names, making the whole thing look like a function call, with support for array unpacking:
>
> $point = clone $point with (x: $x, y: $y, z: $z);
> $point = clone $point with (...['x' => $x, 'y' => $y, 'z' => $z]);
>
>
>
> Regards,
>
> --
> Rowan Tommins
> [IMSoP]
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: https://www.php.net/unsub.php
>

I think using arrays here makes a lot of sense.

For example, this example:

public function withStatus($code, $reasonPhrase = ''): Response
{
return clone $this {
$this->statusCode = $code;
$this->reasonPhrase = "Old: $this->reasonPhrase, New: $reasonPhrase";
}
};

could be rewritten with

public function withStatus($statusCode, $reasonPhrase = ''): Response {
// perform validation here
$reasonPhrase = "Old: $this->reasonPhrase, New: $reasonPhrase";
return clone $this with compact('statusCode', 'reasonPhrase');
}

I, personally, would find this much more ergonomic than writing out
blocks of code and having a totally different syntax.

Rowan Collins

3 years ago

On 20/04/2023 08:11, Robert Landers wrote:

> public function withStatus($code, $reasonPhrase = ''): Response
> {
>      return clone $this {
>          $this->statusCode = $code;
>           $this->reasonPhrase = "Old: $this->reasonPhrase, New: $reasonPhrase";
>      }
> };

Note that this is not the current proposed syntax. Since the keys are
not dynamic, the current proposal is this:

public function withStatus($statusCode, $reasonPhrase = ''): Response {
// perform validation here
$reasonPhrase = "Old: $this->reasonPhrase, New: $reasonPhrase";
return clone $this with {statusCode: $statusCode, reasonPhrase: $reasonPhrase};
}

> public function withStatus($statusCode, $reasonPhrase = ''): Response {
>    // perform validation here
>    $reasonPhrase = "Old: $this->reasonPhrase, New: $reasonPhrase";
>    return clone $this with compact('statusCode', 'reasonPhrase');
> }

The compact() function always feels like a relic of the same era as
create_function() and call_user_func(), both of which now have dedicated
syntax.

That's what Nikita was talking about in the RFC section I quoted
earlier: that compact('foo', 'bar') could be replaced with a dedicated
syntax like [:$foo, :$bar]

So if we insisted on arrays, that would be:

public function withStatus($statusCode, $reasonPhrase = ''): Response {
// perform validation here
$reasonPhrase = "Old: $this->reasonPhrase, New: $reasonPhrase";
return clone $this with [:$statusCode, :$reasonPhrase];
}

But I still don't see why an array should be the default case here,
rather than using ... to unpack one if you really need to, like we do
with arguments.

public function withStatus($statusCode, $reasonPhrase = null): Response {
$newProps = [:$statusCode];
if ( $reasonPhrase !== null ) {
$newProps['reasonPhrase'] = "Old: $this->reasonPhrase, New: $reasonPhrase";
}
return clone $this with (...$newProps);
}


Regards,

--
Rowan Tommins
[IMSoP]

A.L.E.C

3 years ago

On 20.04.2023 09:00, Rowan Tommins wrote:

> Rather than making everything use an array or array-like syntax, I would probably go the other way and scrap the special syntax for dynamic names, making the whole thing look like a function call, with support for array unpacking:
>
> $point = clone $point with (x: $x, y: $y, z: $z);
> $point = clone $point with (...['x' => $x, 'y' => $y, 'z' => $z]);

or $point = clone($point, x: $x, y: $y, z: $z);

Also, I didn't see it mentioned, but maybe for future scope, these new
arguments should be passed to __clone().

--
Aleksander Machniak
Kolab Groupware Developer        [https://kolab.org]
Roundcube Webmail Developer  [https://roundcube.net]
----------------------------------------------------
PGP: 19359DC1 # Blog: https://kolabian.wordpress.com

Larry Garfield

3 years ago

On Thu, Apr 20, 2023, at 7:22 AM, Aleksander Machniak wrote:

> On 20.04.2023 09:00, Rowan Tommins wrote:
>> Rather than making everything use an array or array-like syntax, I would probably go the other way and scrap the special syntax for dynamic names, making the whole thing look like a function call, with support for array unpacking:
>>
>> $point = clone $point with (x: $x, y: $y, z: $z);
>> $point = clone $point with (...['x' => $x, 'y' => $y, 'z' => $z]);
>
> or $point = clone($point, x: $x, y: $y, z: $z);
>
> Also, I didn't see it mentioned, but maybe for future scope, these new
> arguments should be passed to __clone().

They should not.  See https://peakd.com/hive-168588/@crell/object-properties-part-2-examples .  I went through and experimented with different syntaxes, and passing arguments to __clone() was by far the worst option in practice. :-)

--Larry Garfield

A.L.E.C

3 years ago

On 20.04.2023 16:15, Larry Garfield wrote:

>> or $point = clone($point, x: $x, y: $y, z: $z);
>>
>> Also, I didn't see it mentioned, but maybe for future scope, these new
>> arguments should be passed to __clone().
>
> They should not.  See https://peakd.com/hive-168588/@crell/object-properties-part-2-examples .  I went through and experimented with different syntaxes, and passing arguments to __clone() was by far the worst option in practice. :-)

Sure, I didn't propose it to be a solution for "clone with", but rather,
if we implement "clone with" (no matter in which way) this is "cloning
an object with extra properties", so therefore one would ask to have
access to these extra properties inside __clone(). I'm not sure how
useful that could be, but maybe worth mentioning in future scope or
somewhere in the RFC.

And I think this syntax should still be on the table, no need for a new
keyword.

$point = clone($point, x: $x, y: $y, z: $z);

--
Aleksander Machniak
Kolab Groupware Developer        [https://kolab.org]
Roundcube Webmail Developer  [https://roundcube.net]
----------------------------------------------------
PGP: 19359DC1 # Blog: https://kolabian.wordpress.com

Larry Garfield

3 years ago

On Thu, Apr 20, 2023, at 7:00 AM, Rowan Tommins wrote:

> Rather than making everything use an array or array-like syntax, I
> would probably go the other way and scrap the special syntax for
> dynamic names, making the whole thing look like a function call, with
> support for array unpacking:
>
> $point = clone $point with (x: $x, y: $y, z: $z);
> $point = clone $point with (...['x' => $x, 'y' => $y, 'z' => $z]);

I agree here, for all the reasons Rowan indicated.  We already have a perfectly good syntax and semantics for named arguments that supports splat.  Using that here would handle all the use cases we care about, including dynamic names, without any additional syntax.

Let's not shoe-horn arrays in here where they're not needed.  Making arrays themselves fancier/more compact with an alternate syntax is worth discussing, but that should be a separate RFC.

--Larry Garfield

Tim Düsterhus

3 years ago

Hi

On 4/20/23 16:17, Larry Garfield wrote:

> On Thu, Apr 20, 2023, at 7:00 AM, Rowan Tommins wrote:
>
>> Rather than making everything use an array or array-like syntax, I
>> would probably go the other way and scrap the special syntax for
>> dynamic names, making the whole thing look like a function call, with
>> support for array unpacking:
>>
>> $point = clone $point with (x: $x, y: $y, z: $z);
>> $point = clone $point with (...['x' => $x, 'y' => $y, 'z' => $z]);
>
> I agree here, for all the reasons Rowan indicated.  We already have a perfectly good syntax and semantics for named arguments that supports splat.  Using that here would handle all the use cases we care about, including dynamic names, without any additional syntax.
>

FWIW I'm not too attached to my array proposal [1]. I would also be fine
with Rowan's proposal of making the "with()" syntactically identical to
a function call, if that's more agreeable.

But please no entirely new syntax with braces as it currently is shown
in the examples in the RFC.

Best regards
Tim Düsterhus

[1] Though I still consider arrays to be more "natural" than named
arguments.

Michał Brzuchalski

3 years ago

Hi Tim,

czw., 20 kwi 2023 o 16:39 Tim Düsterhus <tim@bastelstu.be> napisał(a):

> ...
> But please no entirely new syntax with braces as it currently is shown
> in the examples in the RFC.
>

Then we should vote for syntax. Personally, I prefer braces here
because it doesn't look like a regular function call allowing easily to
distinguish
between two different things.

Cheers,
Michał Marcin Brzuchalski

Máté Kocsis

3 years ago

Hey Everyone,

Thank you for the lot of feedback! Sorry, I'm going to have to answer in a
single email otherwise I would have to send too many emails.

Alexandru wrote:

How about just allowing a block of code after the clone statement that

> would execute it in the same context as the clone context, allowing to
> modify once readonly variables.
> Allows better flexibility compared with clone with syntax or clone method:

This is an interesting alternative, but as Tim pointed out in a later
reply, the block would run in the private scope, which is contrary to my
intentions (and what I consider
the best solution).

Michał wrote:

I am curious if possible to implement the feature without using `with`

> keyword
> it visually could look pretty close to something like an object
> initializer in the future:
> return clone $this {c: 1};
> return new Bar {c: 1};

That's exactly what I tried first, but unfortunately, this syntax led to
parser ambiguities, so at last I had to settle on using "with".

Rowan wrote:

1) You mention in the Alternatives sometimes needing access to the

> original instance; it would be good to have an example of how this looks
> with the clone-with syntax.

Makes sense, so I've just come up with an example where a linked list of
objects are created. Let me know if you have a better example :)

2) How does this interact with an __clone() method? I'm guessing the

> __clone() would be called first, and then the with-clause applied?

Yeah, thanks for pointing this out! I agree that the clarification is very
much needed. BTW the evaluation order
is exactly how you wrote. This is now added to the RFC.

Tim wrote:

> In which order will __clone(), foo(), bar(), quux(), baz() be evaluated
> and what will happen if one of them throws an Exception? Will
> destructors of the cloned object run? When?

In fact, after the initial ZEND_CLONE opcode (which possibly invokes
__clone()), a separate opcode is generated for each
assignment (the newly added ZEND_CLONE_INIT_PROP). This means that foo(),
bar(), quux(), and baz() will be evaluated
in this very order. If any of them throws an exception, evaluation stops
and the assignments are not rolled back.

Regarding the destructors: yes, the destructor of the cloned object runs
immediately. In order to make sure, I've just added a test case:
https://github.com/php/php-src/pull/9497/commits/4d184f960ac1b5590d87739ee3278c13fac157de
I hope that this result is what
you expect.

Michal wrote:

> Just noticed the "Property name expressions" and am wondering if it could
> be a separate feature
> allowing for passing named arguments to functions/constructors in the same
> fashion?

As far as I can see, Nikita didn't propose the expression1() =>
expression2() syntax in the named params RFC due to
the same ambiguity I mentioned in the RFC (identifier vs. global constant).
But I don't think this is set in stone, however,
I do think that some optimizations would have to be disabled when param
names weren't evaluatable in
compile-time (https://github.com/php/php-src/pull/10831).

Andreas wrote:

What about argument unpacking?

> I don't know if we can combine this with ":" syntax or only with "=>".

For now, argument unpacking (property unpacking?) is not possible. But it
is definitely something that could be added in the future.

Tim wrote:

I'd rather see only the fat-arrow being allowed. Unless I'm missing

> something, braces with colon is not used anywhere else, whereas braces +
> '=>' is known from match() and '=>' more generally is already used with
> array literals [1]. Having two similar syntaxes for the same thing is
> not great when both are commonly needed is not great. They need to be
> documented and learned by developers.

I can only repeat what Rowan answered, since I agree with it completely:

I think it makes sense to have an unquoted form here, because the common

> case is that they are names which analysers can match statically to
> particular properties, not strings which will be analysed at runtime. There
> are plenty of places in the language where dynamic names are allowed, but
> we don't just use strings for the static case

However, I'm not completely sold on making "clone with" look like a
function call (return clone $this with (a: 1);), but
at least I like it more than using an array-like style (return clone $this
with [a: 1];). My intention with
using curly brackets (return clone $this with {a: 1};) is to highlight the
fact that it is a map
of key-value pairs, similarly how the JSON standard does so. Not to mention
that "clone with" serves a very
similar purpose to object initializers, and the different languages I know
to have this feature use
a similar syntax (Java: http://wiki.c2.com/?DoubleBraceInitialization, C#:
https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/object-and-collection-initializers#object-initializers
).

Regards,
Máté

Larry Garfield

3 years ago

On Sun, Apr 23, 2023, at 6:01 PM, Máté Kocsis wrote:

> What about argument unpacking?
>> I don't know if we can combine this with ":" syntax or only with "=>".
>
>
> For now, argument unpacking (property unpacking?) is not possible. But it
> is definitely something that could be added in the future.

Not currently written but could be, or there's some reason why that is particularly hard?

> Tim wrote:
>
> I'd rather see only the fat-arrow being allowed. Unless I'm missing
>> something, braces with colon is not used anywhere else, whereas braces +
>> '=>' is known from match() and '=>' more generally is already used with
>> array literals [1]. Having two similar syntaxes for the same thing is
>> not great when both are commonly needed is not great. They need to be
>> documented and learned by developers.
>
>
> I can only repeat what Rowan answered, since I agree with it completely:
>
> I think it makes sense to have an unquoted form here, because the common
>> case is that they are names which analysers can match statically to
>> particular properties, not strings which will be analysed at runtime. There
>> are plenty of places in the language where dynamic names are allowed, but
>> we don't just use strings for the static case
>
>
> However, I'm not completely sold on making "clone with" look like a
> function call (return clone $this with (a: 1);), but
> at least I like it more than using an array-like style (return clone
> $this
> with [a: 1];). My intention with
> using curly brackets (return clone $this with {a: 1};) is to highlight
> the
> fact that it is a map
> of key-value pairs, similarly how the JSON standard does so. Not to
> mention
> that "clone with" serves a very
> similar purpose to object initializers, and the different languages I
> know
> to have this feature use
> a similar syntax (Java: http://wiki.c2.com/?DoubleBraceInitialization,
> C#:
> https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/object-and-collection-initializers#object-initializers
> ).
>
> Regards,
> Máté

True, but PHP 8.0 sort of added something close to object initializers with the combination of constructor promotion and named arguments.  It's not exactly the same thing, but it's the effective equivalent in PHP today.  So if you want to model on object initializers, named args would be the closest PHP equivalent.

--Larry Garfield

Tim Düsterhus

3 years ago

Hi

On 4/24/23 01:01, Máté Kocsis wrote:

> 2) How does this interact with an __clone() method? I'm guessing the
>> __clone() would be called first, and then the with-clause applied?
>
>
> Yeah, thanks for pointing this out! I agree that the clarification is very
> much needed. BTW the evaluation order
> is exactly how you wrote. This is now added to the RFC.
>
> Tim wrote:
>
>> In which order will __clone(), foo(), bar(), quux(), baz() be evaluated
>> and what will happen if one of them throws an Exception? Will
>> destructors of the cloned object run? When?
>
>
> In fact, after the initial ZEND_CLONE opcode (which possibly invokes
> __clone()), a separate opcode is generated for each
> assignment (the newly added ZEND_CLONE_INIT_PROP). This means that foo(),
> bar(), quux(), and baz() will be evaluated
> in this very order. If any of them throws an exception, evaluation stops
> and the assignments are not rolled back.
>
> Regarding the destructors: yes, the destructor of the cloned object runs
> immediately. In order to make sure, I've just added a test case:
> https://github.com/php/php-src/pull/9497/commits/4d184f960ac1b5590d87739ee3278c13fac157de
> I hope that this result is what
> you expect.

I'm combining your two replies, as they are related: The behavior is
what I expected. It would however be useful if you also added an
explicit example with side effects to "Interaction with the __clone
magic method" section in the RFC and the tests (just 'echo __FUNCTION__'
or so) to make it explicit. I believe the attached example should do the
trick. If I understand correctly the output should be:

__clone
a
b
c
d
unhandled exception
__destruct
__destruct

> I'd rather see only the fat-arrow being allowed. Unless I'm missing
>> something, braces with colon is not used anywhere else, whereas braces +
>> '=>' is known from match() and '=>' more generally is already used with
>> array literals [1]. Having two similar syntaxes for the same thing is
>> not great when both are commonly needed is not great. They need to be
>> documented and learned by developers.
>
>
> I can only repeat what Rowan answered, since I agree with it completely:
>
> I think it makes sense to have an unquoted form here, because the common
>> case is that they are names which analysers can match statically to
>> particular properties, not strings which will be analysed at runtime. There
>> are plenty of places in the language where dynamic names are allowed, but
>> we don't just use strings for the static case

A static analyzer should be able to understand a string literal.

>
> However, I'm not completely sold on making "clone with" look like a
> function call (return clone $this with (a: 1);), but
> at least I like it more than using an array-like style (return clone $this
> with [a: 1];). My intention with
> using curly brackets (return clone $this with {a: 1};) is to highlight the
> fact that it is a map
> of key-value pairs, similarly how the JSON standard does so. Not to mention

To a PHP programmer, a map of arbitrary key-value pairs is an array.
That's what is familiar to every PHP programmer and thus an array
literal should feel reasonably natural and obvious with regard to
semantics (that's why it was my suggestion).

The named parameters syntax is fairly new, but at least there is
precedent and thus developers are able to transfer their existing knowledge.

Both would be able to completely replace the Foo::withProperties()
example by means of existing syntax (either bare array or named
parameter destructuring) and without the overhead of repeated cloning.

The brace + colon syntax is a completely new invention and less flexible
as shown by the need to use a different separator character to
differentiate between bare names and global constants.

> that "clone with" serves a very
> similar purpose to object initializers, and the different languages I know
> to have this feature use
> a similar syntax (Java: http://wiki.c2.com/?DoubleBraceInitialization, C#:
> https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/object-and-collection-initializers#object-initializers
> ).

Different languages, different syntax choices and different existing
syntax. I don't do C#, but I'm sure that the C# syntax totally makes
sense in the context of C#. I do not believe it makes sense in PHP.

Best regards
Tim Düsterhus

Michał Brzuchalski

3 years ago

Hi Máté,

pon., 17 kwi 2023 o 08:32 Máté Kocsis <kocsismate90@gmail.com> napisał(a):

> Hi Everyone,
>
> Quite some time after mentioning the "clone with" construct the first time
> (at the end of the
> https://wiki.php.net/rfc/write_once_properties#run-time_behaviour
> section),
> finally I managed to create a working implementation for this feature which
> would make it possible to properly modify readonly properties
> while simplifying how we write "wither" methods:
> https://wiki.php.net/rfc/clone_with

Thanks for your efforts and for bringing that up.
I am curious if possible to implement the feature without using `with`
keyword
it visually could look pretty close to something like an object initializer
in the future:

return clone $this {c: 1};
return new Bar {c: 1};

Cheers,
Michał Marcin Brzuchalski

Zoltán Fekete

3 years ago

Hey,

> public function withStatus($code, $reasonPhrase = ''): Response
> {
>     return clone $this {
>         $this->statusCode = $code;
>
>         $this->reasonPhrase = $reasonPhrase;
>     }
> };

How to refer to any of the properties of the current instance? Let's say
this:

```
class Foo {
protected int $code;
protected string $message;
public function withStatus($code, $message): Foo {
return clone $this {
$this->code = $code; // so far so good
// Which $this->message is what?
$this->message = "cloned: (" . $this->message . ")" . $message;
}
}
}
```

> Thanks for your efforts and for bringing that up.
> I am curious if possible to implement the feature without using `with`
> keyword
> it visually could look pretty close to something like an object

initializer

> in the future:
>
> return clone $this {c: 1};
> return new Bar {c: 1};

Similarity is not necessarily always good.

Regards,
Zoltán Fekete

Alexandru Pătrănescu

3 years ago

Hi Zoltán,
On Mon, Apr 17, 2023 at 11:13 PM Zoltán Fekete <fekzol.13ker@gmail.com>
wrote:

> Hey,

> > public function withStatus($code, $reasonPhrase = ''): Response
> > {
> >     return clone $this {
> >         $this->statusCode = $code;
> >         $this->reasonPhrase = $reasonPhrase;
> >     };
> > }
>
> How to refer to any of the properties of the current instance? Let's say
> this:
>
> ```
> class Foo {
>     protected int $code;
>     protected string $message;
>     public function withStatus($code, $message): Foo {
>         return clone $this {
>             $this->code = $code; // so far so good
>             // Which $this->message is what?
>             $this->message = "cloned: (" . $this->message . ")" . $message;
>         }
>     }
> }
> ```

Yes, it's true that it looks weird that $this is present in two places with
two different meanings, but that's just because we are cloning $this. If we
were cloning $object, it would be clearer.
$this inside the braces is referring to the new object after it was cloned,
after __clone() was executed, if defined.
The old instance data is already copied over, no real need to reference
that.

Maybe we can think about it a bit more and improve the idea. I'm
thinking we can have a closure/callable or an instance method reference
instead of a block of code.
Also, that might be preferable, as there is a natural way to either use
closure with "use" or pass parameters.
I don't like too much how $code and $message are referenced inside the
block of code... maybe it's not an issue.
Also, I'm not really sure how easy it would be to model this behavior, to
have a block of code where $this would reference something else than
outside of it.

Regards,
Alex

Rowan Collins

3 years ago

On 17/04/2023 07:32, Máté Kocsis wrote:

> https://wiki.php.net/rfc/clone_with

I look the look of this; very concise.

A couple of clarifications that might be useful to add to the RFC:

1) You mention in the Alternatives sometimes needing access to the
original instance; it would be good to have an example of how this looks
with the clone-with syntax.

2) How does this interact with an __clone() method? I'm guessing the
__clone() would be called first, and then the with-clause applied?


Regards,

--
Rowan Tommins
[IMSoP]

Tim Düsterhus

3 years ago

Hi

On 4/18/23 10:10, Rowan Tommins wrote:

> 2) How does this interact with an __clone() method? I'm guessing the
> __clone() would be called first, and then the with-clause applied?
>

More generally the order of operations with regard to possible side
effects and/or exceptions would be interesting.

clone $something with {
foo() => bar(),
quux() => baz(),
};

In which order will __clone(), foo(), bar(), quux(), baz() be evaluated
and what will happen if one of them throws an Exception? Will
destructors of the cloned object run? When?

Best regards
Tim Düsterhus

Michał Brzuchalski

3 years ago

wt., 18 kwi 2023 o 10:20 Tim Düsterhus <tim@bastelstu.be> napisał(a):

> Hi
>
> On 4/18/23 10:10, Rowan Tommins wrote:
> > 2) How does this interact with an __clone() method? I'm guessing the
> > __clone() would be called first, and then the with-clause applied?
> >
>
> More generally the order of operations with regard to possible side
> effects and/or exceptions would be interesting.
>
>     clone $something with {
>       foo() => bar(),
>       quux() => baz(),
>     };
>

Just noticed the "Property name expressions" and am wondering if it could
be a separate feature
allowing for passing named arguments to functions/constructors in the same
fashion?

$something = new Something(
foo() => bar(),
quux() => baz(),
);

Cheers,
Michał Marcin Brzuchalski

Andreas Hennings

3 years ago

Hello Máté, internals,
I have been waiting for this to happen :)
Some feedback

> However, in some cases it would be useful to reference property names as expressions, e.g. when one needs to use “clone with” in a foreach loop where the index is the property name and the loop variable is the value to be assigned. This is also possible using a slightly more verbose syntax:

What about argument unpacking?
I don't know if we can combine this with ":" syntax or only with "=>".

clone $object with {
...$arr,
...['a' => 'A', $b => 'B'],
...(function () {yield 'x' => 'X';})(),
c: 'C',  // or
d => 'D',  // No mixing of ':' and '=>', we have to choose one.
}

If we want to go crazy in the future:
(This would require another language feature of inline code blocks
with return value or behaving as generator)

clone $object with {
...{
yield 'a' => 'A';
yield 'b' => 'B';
},
...{
return ['c' => 'C'],
}
}

-- Andreas

On Mon, 17 Apr 2023 at 08:32, Máté Kocsis <kocsismate90@gmail.com> wrote:

Andreas Hennings

3 years ago

> The biggest advantage of Nicolas' proposal over “clone with” is that it could be made part of the interface contract whether a method modifies the object state. So the PSR-7 ResponseInterface could look like the following:
> [..]
> public clone function withStatus($code, $reasonPhrase = '');

If this is the main argument, I think I would prefer a more general
"readonly" modifier on methods.

public readonly function withStatus($code, $reasonPhrase = ''): static;

> One cannot control whether $this should really be cloned: e.g. if a property should only be modified based on certain conditions (e.g. validation), the object would potentially be cloned in vain, resulting in a performance loss.

Exactly. I would say "conditionally clone", e.g. only clone if there
is a change, or if we don't already have a cached instance with this
value.

I think the "clone with" is much more flexible because we can call
this anywhere, not just in a dedicated method.

This said, I do agree with some of the benefits of the "Alternative" proposal.

-- Andreas


On Mon, 17 Apr 2023 at 08:32, Máté Kocsis <kocsismate90@gmail.com> wrote:

Nicolas Grekas

3 years ago

Hi Mate,

Quite some time after mentioning the "clone with" construct the first time

> (at the end of the
> https://wiki.php.net/rfc/write_once_properties#run-time_behaviour
> section),
> finally I managed to create a working implementation for this feature which
> would make it possible to properly modify readonly properties
> while simplifying how we write "wither" methods:
> https://wiki.php.net/rfc/clone_with
>

Thanks for working on this, we definitely need improvements on the topic.
Thanks also for mentioning my proposal and for the comparison analysis,
that's really helpful.

Quoting from the RFC:

> One cannot control whether $this should really be cloned: e.g. if a

property should only be modified based on certain conditions (e.g.
validation), the object would potentially be cloned in vain, resulting in a
performance loss.

Returning a clone or the same instance depending e.g. on some validation is
usually an abstraction leak. It's a leak because it allows knowing internal
implementation details by comparing the identity of the resulting objects.
Since the PHP community is leaning towards more strictness and better
abstractions, I think this point is actually a win for my proposal: it'd
naturally close the loophole in wither-based abstractions. See
https://3v4l.org/02IVc#v8.2.5 if what I mean is unclear.

> Sometimes one also needs access to the original instance, but doing so

would only be possible via workarounds (e.g. by introducing $that or a
similar construct).

Here is how we would achieve this under my proposal (adapting from your
example):
class LinkedObject {
/*...*/

public function next(): static {
$clone = $this->prepareNext();
$clone->next = $this;

return $clone;
}

private clone function prepareNext(): static {
$this->number++;
unset($this->next);

return $this;
}
}

As you can see, this works by *not* using the clone keyword on the public
method. And this makes me realize that what you see as an advantage ("it
could be made part of the interface contract") might actually be a
drawback, by forcing a coupling between a contract and an implementation.
That's what you mean also with your last sentence on your analyses.

And that kills my proposal, RIP :)
But I have another proposal I'm sending separately.

Nicolas

Nicolas Grekas

3 years ago

Hi again,

Quite some time after mentioning the "clone with" construct the first time

> (at the end of the
> https://wiki.php.net/rfc/write_once_properties#run-time_behaviour
> section),
> finally I managed to create a working implementation for this feature which
> would make it possible to properly modify readonly properties
> while simplifying how we write "wither" methods:
> https://wiki.php.net/rfc/clone_with
>

As mentioned in another thread, I'd like to make an alternative proposal
for the syntax.
Alex talked about it already but I think it deserves more attention.

What about using a real closure to define the scope we need for cloning?
That closure would take the cloned instance as argument to allow
manipulating it at will.

Internally, the engine would "just" call that closure just after calling
__clone() if it's defined.

This could look like this:
$c = clone $a with $closure;

or maybe we could skip introducing a new keyword and go for something that
looks like a function call:
$c = clone($a, $closure);

I've adapted your examples from the RFC using the latter and here is how
they could look like:

The “wither” method copy-pasted from Diactoros:
class Response implements ResponseInterface {
public readonly int $statusCode;
public readonly string $reasonPhrase;
// ...
public function withStatus($code, $reasonPhrase = ''): Response
{
return clone($this, fn ($clone) => [
$clone->statusCode = $code,
$clone->reasonPhrase = $reasonPhrase,
]);
}
// ...
}

The property name expressions:
class Foo {
private $a;
private $b;
private $c;
/**
* @param array<string, mixed> $properties
*/
public function withProperties(array $properties) {
return clone($this, function ($clone) use ($properties) {
foreach ($properties as $name => $value) {
$clone->$name = $value;
}
});
}
}

Linking the cloned instance to the original one:
class LinkedObject {
public function __construct(
private readonly LinkedObject $next,
private readonly int $number
) {
$this->number = 1;
}
public function next(): LinkedObject {
return clone($this, fn ($clone) => [
$clone->number = $clone->number + 1,
$this->next = $clone,
]);
}
}

All these look pretty neat to me, and they come with no new syntax but a
simple call-like clone() statement.

Scope semantics remain the same as usual, so we already know how to
interpret that aspect.

Does that make sense to you?

Nicolas

Larry Garfield

3 years ago

On Tue, Apr 25, 2023, at 11:23 AM, Nicolas Grekas wrote:

> What about using a real closure to define the scope we need for cloning?
> That closure would take the cloned instance as argument to allow
> manipulating it at will.

I believe someone mentioned that one previously in the thread.  The problem is that the closure would run in the scope of the object, not the scope of the caller.  That means if called outside the object, it would allow modifying private or protected properties.  The itemized list of values (whether an array or named-args style) would allow the engine to enforce access restrictions, which is a desireable feature.

--Larry Garfield

Alexandru Pătrănescu

3 years ago

On Wed, Apr 26, 2023 at 3:25 AM Larry Garfield <larry@garfieldtech.com>
wrote:

> On Tue, Apr 25, 2023, at 11:23 AM, Nicolas Grekas wrote:
>
> > What about using a real closure to define the scope we need for cloning?
> > That closure would take the cloned instance as argument to allow
> > manipulating it at will.
>
> I believe someone mentioned that one previously in the thread.

Yes, Nicolas mentioned me.
I wanted to get back myself to discussing this topic more as well and find
a better solution but didn't get yet time for it.

>   The problem is that the closure would run in the scope of the object,
> not the scope of the caller.  That means if called outside the object, it
> would allow modifying private or protected properties.  The itemized list
> of values (whether an array or named-args style) would allow the engine to
> enforce access restrictions, which is a desireable feature.
>

As far as I can see, Nicolas was able to find a solution to this problem
and so far I like it:
The closure is running in the current scope where it is defined and not in
the scope of the cloned object. The cloned object is passed as the single
parameter to the closure.

The suggested clone signature for a class T would be:
- clone(T $object, callable(T $clone)): T; // calling clone as a function
- clone T $object with callable(T $clone): T; // calling clone as a
language construct
Alternatively, we can have also:
- clone T, callable(T $clone); // without "with" keyword
And improve it to allow even multiple closures to be executed:
- clone(T $object, ...callable(T $clone)): T;
- clone T $object, ...callable(T $clone): T;

IMHO, I think we should support both syntaxes.

The main reason I wanted to get back to this proposal is because > 90% of a
developer's job is to read code and the most trouble is following state
modifications of a variable.

When that variable to be investigated is a property, so far it was simple
to look for "->propertyName = <rhs expression>".
Editors would easily and clearly highlight all the write access statements
or lines (Alt+F7 in PhpStorm). I can't figure out what would be highlighted
in the non-closure syntax, especially the one with an array.

Thanks,
Alex

Nicolas Grekas

3 years ago

> > > What about using a real closure to define the scope we need for
> cloning?
> > > That closure would take the cloned instance as argument to allow
> > > manipulating it at will.
> >
> > I believe someone mentioned that one previously in the thread.
>
>
> Yes, Nicolas mentioned me.
> I wanted to get back myself to discussing this topic more as well and find
> a better solution but didn't get yet time for it.
>
>
> >   The problem is that the closure would run in the scope of the object,
> > not the scope of the caller.  That means if called outside the object, it
> > would allow modifying private or protected properties.  The itemized list
> > of values (whether an array or named-args style) would allow the engine
> to
> > enforce access restrictions, which is a desireable feature.
> >
>
> As far as I can see, Nicolas was able to find a solution to this problem
> and so far I like it:
> The closure is running in the current scope where it is defined and not in
> the scope of the cloned object. The cloned object is passed as the single
> parameter to the closure.
>

Absolutely. This would be a plain boring closure with all its current
visibility semantics.
Using a closure to run some code nested in a transaction is already quite a
common practice.
E.g.this is a common way to define the computation logic for a cache
storage:
$cache->get($cacheKey, $callback)

Or for a database transaction:
$db->transaction(function() { ... });

The cloning logic we want to run fits this style, so this is quite natural
once we realize that:
$clone = clone($this, $callback);

The only thing we would need to settle on is the interface of the $callback.

The suggested clone signature for a class T would be:

> - clone(T $object, callable(T $clone)): T; // calling clone as a function
> - clone T $object with callable(T $clone): T; // calling clone as a
> language construct

100% this. My preference goes for #1, to keep things as boring as possible.
Just to make it clear, I would document the closure with the void return
type:
clone(T $object, callable(T $clone)*:void*): T; // calling clone as a
function

> Alternatively, we can have also:
> - clone T, callable(T $clone); // without "with" keyword
>

This would be ambiguous, eg foo(clone T, callable(T $clone)) is that a
function call with two arguments, or?

> And improve it to allow even multiple closures to be executed:
> - clone(T $object, ...callable(T $clone)): T;
> - clone T $object, ...callable(T $clone): T;
>

I wouldn't allow this. Calling many closures is what the main closure can
do in its body, no need for more fancy things.

Nicolas

Máté Kocsis

3 years ago

Hi Everyone,

In the meanwhile, I changed my proposal to use [] instead of {} after the
"with" clause due to its better receptance.
Additionally, I removed support for the shorthand "property assignment"
syntax (clone $this with [property1: "foo"]) in
favor the more powerful one where the left-hand side supports expressions
(clone $this with ["property1" => "foo"])
so that we have more time to decide whether the former one is really needed.

After talking with Nicolas and Alexandru off-list (I accidentally didn't
click "Reply All" in my response), we identified
that their "clone callback" proposal could also be a future-scope of the
current RFC with the following syntax:

clone $this with function (object $clone): void {
$this->property1 = "foo";
}

In my opinion, this syntax is OK, albeit it's a bit long to write and a bit
difficult to decipher its meaning (maybe it's
just me). That's the main reason I prefer my originally proposed syntax.
Apart from this, I don't see a big issue with it,
neither conceptually, nor with the implementation.

Regards,
Máté

Michał Brzuchalski

3 years ago

Hi Máté,

pon., 29 maj 2023 o 11:18 Máté Kocsis <kocsismate90@gmail.com> napisał(a):

> Hi Everyone,
>
> In the meanwhile, I changed my proposal to use [] instead of {} after the
> "with" clause due to its better receptance.
> Additionally, I removed support for the shorthand "property assignment"
> syntax (clone $this with [property1: "foo"]) in
> favor the more powerful one where the left-hand side supports expressions
> (clone $this with ["property1" => "foo"])
> so that we have more time to decide whether the former one is really
> needed.
>

So there would be no option to vote on shorthand properties, right?

Array syntax with all properties as strings in quotes probably means no
support from IDE.
I think this is a really bad move, I'd be against it.

Cheers,
Michał Macin Brzuchalski

Larry Garfield

3 years ago

On Mon, May 29, 2023, at 6:47 AM, Michał Marcin Brzuchalski wrote:

> Hi Máté,
>
> pon., 29 maj 2023 o 11:18 Máté Kocsis <kocsismate90@gmail.com> napisał(a):
>
>> Hi Everyone,
>>
>> In the meanwhile, I changed my proposal to use [] instead of {} after the
>> "with" clause due to its better receptance.
>> Additionally, I removed support for the shorthand "property assignment"
>> syntax (clone $this with [property1: "foo"]) in
>> favor the more powerful one where the left-hand side supports expressions
>> (clone $this with ["property1" => "foo"])
>> so that we have more time to decide whether the former one is really
>> needed.
>>
>
> So there would be no option to vote on shorthand properties, right?
>
> Array syntax with all properties as strings in quotes probably means no
> support from IDE.
> I think this is a really bad move, I'd be against it.

I concur, especially with a dynamic list pushed off to future scope.  As is, we're getting less IDE friendliness and a lot more ' characters in return for making the property name dynamic.  However, the *number* of property names is still fixed at code time.  That's an incomplete tradeoff.

Conversely, using the named arguments style syntax (what future scope calls a shorthand, which I don't think is accurate as it's nearly the same amount of typing) and allowing it to be "splatted", just like named arguments, gives us the best of all worlds.  The typical case is easy to read, easy to write, and IDE-refactor-friendly.  Allowing it to be splatted (just like named arguments) means that in the unusual case where you want the number or names of the properties to be dynamic (which is a valid use case, but a minority one), you can pre-build an array and splat it in place, exactly like you can named arguments.

Typical:

return clone $this with (
statusCode: $code,
);

Fancy:

$props[strtolower('statusCode')] = $code;
if ($reason) {
$props['reason'] = $reason;
}
return clone $this with ...$props;

That approach, taken now, gives us all the flexibility people have been asking for, reuses the existing named arguments syntax entirely, and is the most refactorable option.

Additionally, the current "property names expression" section shows a very odd example.  It's recloning the object N times, reinvoking the __clone method each time, and reassigning a single property.  If you're updating several properties at once, that is extremely inefficient.  Dynamic property names are not the solution there; allowing the list to be dynamic in the first place is, and that should not be punted to future scope.

I really want clone-with, but this seems like a big step backwards in terms of its capability.  It may be enough for me to vote no on it, but I'm not sure.

(Pushing clone-with-callable off to later is, I agree, the correct move.)

I am also confused by the choice to make __clone() run before with with properties.  I would have expected it to be the other way around.  Can you explain (in the RFC) why you did it that way?  That seems more limiting than the alternative, as you cannot forward-pass information to __clone() this way.

--Larry Garfield

Máté Kocsis

3 years ago

Hi Michał and Larry,

As Tim has already clarified, using literal strings in the left-hand side
of "clone with expressions" won't cause any issues
for IDEs and static analysers to identify the correct property. I got rid
of the shorthand syntax from the proposal because
it is not strictly required (since it has an equivalent alternative), and I
acknowledge the fact that there are certain people
who don't want to have two syntaxes to express the same thing. If it turns
out that we really want to have the short-hand
syntax as well, we can always add support for it later.

Conversely, using the named arguments style syntax (what future scope calls

> a shorthand, which I don't think is accurate as it's nearly the same amount
> of typing)

I'm just using the same wording as Nikita used in his named argument RFC.
Please take a look at the following section:
https://wiki.php.net/rfc/named_params#shorthand_syntax_for_matching_parameter_and_variable_name
Especially this sentence is important:

If I wanted to put these ideas into a general framework, I think one way to

> go about this would be as follows:
> - Consider identifier: $expr as a shorthand for "identifier" => $expr.

Even though the shorthand syntax is only 4 characters shorter than the
normal one (including the missing space before the colon),
for a typical property name which may be 4-8 characters long (I guess),
it's a significant reduction of typing.

That approach, taken now, gives us all the flexibility people have been

> asking for, reuses the existing named arguments syntax entirely, and is the
> most refactorable option.
>

Your suggestion is very interesting/promising... However, I'm a bit worried
that if I only supported the "identifier: expression" syntax for
"clone assignments", and I required people to pass an array (or splatted
array) instead of the "expression => expression" syntax,
they would start complaining why they cannot directly use expressions as
property names... That's why I think all the alternatives are useful
on their own, but only the "expressions => expression" syntax is
vital, since all the other use-cases can be written like this.

> Additionally, the current "property names expression" section shows a very
> odd example.  It's recloning the object N times, reinvoking the __clone
> method each time, and reassigning a single property.  If you're updating
> several properties at once, that is extremely inefficient.  Dynamic
> property names are not the solution there; allowing the list to be dynamic
> in the first place is, and that should not be punted to future scope.
>

I'm aware that it's currently inefficient, but it's the case only when you
want to update a dynamic list of properties. In my opinion, you update
a fixed set of properties most of the time
(e.g. Psr\Http\Message\ResponseInterface::withStatus()), and it's only a
rare case when you need
more dynamic behavior. That's why I believe it's not a huge problem to keep
cloning objects unnecessarily until we add support for the stuff
mentioned in the future scope section.

I am also confused by the choice to make __clone() run before with with

> properties.  I would have expected it to be the other way around.  Can you
> explain (in the RFC) why you did it that way?  That seems more limiting
> than the alternative, as you cannot forward-pass information to __clone()
> this way.
>

To be honest, the current behavior seemed like the natural choice for
me, and I didn't really consider to execute the __clone() method after the
clone assignments.
Do you have a use-case in mind when you need to forward-pass information to
__clone()?

Regards,
Máté

Larry Garfield

3 years ago

On Mon, May 29, 2023, at 11:22 PM, Máté Kocsis wrote:

> Hi Michał and Larry,
>
> As Tim has already clarified, using literal strings in the left-hand side
> of "clone with expressions" won't cause any issues
> for IDEs and static analysers to identify the correct property. I got rid
> of the shorthand syntax from the proposal because
> it is not strictly required (since it has an equivalent alternative), and I
> acknowledge the fact that there are certain people
> who don't want to have two syntaxes to express the same thing. If it turns
> out that we really want to have the short-hand
> syntax as well, we can always add support for it later.

My point is, rather, that if we want to avoid having multiple syntaxes (which is valid), the existing named-args syntax and behavior already more than adequately covers all bases.

> That approach, taken now, gives us all the flexibility people have been
>> asking for, reuses the existing named arguments syntax entirely, and is the
>> most refactorable option.
>>
>
> Your suggestion is very interesting/promising... However, I'm a bit worried
> that if I only supported the "identifier: expression" syntax for
> "clone assignments", and I required people to pass an array (or splatted
> array) instead of the "expression => expression" syntax,
> they would start complaining why they cannot directly use expressions as
> property names... That's why I think all the alternatives are useful
> on their own, but only the "expressions => expression" syntax is
> vital, since all the other use-cases can be written like this.

Has the "if you need a variable name/number, use an array" been a problem for named arguments?  I've not seen it been one.  Has anyone else?

You cannot use an expression as a function argument name today, and... no one seems to mind.  Using an array in that rare edge case is fine.

I think we agree that the most common case by far will be a fixed, small set of statically-named properties.  So we should optimize for that case, which is the named-args style.

>> Additionally, the current "property names expression" section shows a very
>> odd example.  It's recloning the object N times, reinvoking the __clone
>> method each time, and reassigning a single property.  If you're updating
>> several properties at once, that is extremely inefficient.  Dynamic
>> property names are not the solution there; allowing the list to be dynamic
>> in the first place is, and that should not be punted to future scope.
>>
>
> I'm aware that it's currently inefficient, but it's the case only when you
> want to update a dynamic list of properties. In my opinion, you update
> a fixed set of properties most of the time
> (e.g. Psr\Http\Message\ResponseInterface::withStatus()), and it's only a
> rare case when you need
> more dynamic behavior. That's why I believe it's not a huge problem to keep
> cloning objects unnecessarily until we add support for the stuff
> mentioned in the future scope section.

Except that named-args style gives us the better alternative today, with known, pre-existing syntax and semantics.

> I am also confused by the choice to make __clone() run before with with
>> properties.  I would have expected it to be the other way around.  Can you
>> explain (in the RFC) why you did it that way?  That seems more limiting
>> than the alternative, as you cannot forward-pass information to __clone()
>> this way.
>>
>
> To be honest, the current behavior seemed like the natural choice for
> me, and I didn't really consider to execute the __clone() method after the
> clone assignments.
> Do you have a use-case in mind when you need to forward-pass information to
> __clone()?

Not a specific one off hand.  It's more a conceptual question.  `with` has more contextual awareness than __clone(), so it should have "first crack" at the operation, so that if necessary it can make changes that __clone() can then respond to.  The inverse doesn't make sense.

The only reason for `with` to come after would be to allow `with` to "override" or "undo" something that __clone() did.  Generally speaking, if you have to undo something you just did, you shouldn't have done it in the first place, so that's a less compelling combination.

This one isn't a deal breaker, but we should be sure to think it through as it's kinda hard to reverse later.

--Larry Garfield

Brandon Jackson

3 years ago

Hello everyone,

On Mon, May 29, 2023 at 6:48 AM Michał Marcin Brzuchalski
<michal.brzuchalski@gmail.com> wrote:

> So there would be no option to vote on shorthand properties, right?
>
> Array syntax with all properties as strings in quotes probably means no
> support from IDE.
> I think this is a really bad move, I'd be against it.

I agree. I think going to a single syntax was a good move, but using the literal
property name would have been better imo.

I really think the syntax would benefit from being more similar to
function calls,
with a syntax like:

clone $obj with (prop1: "value");

This brings the familiarity from named arguments, which in this case would
represent the object's properties.

Then for dynamic property name support, you can borrow array unpacking
into an argument list from function calls as well, allowing the dynamic
properties to be set like:

$myProp = 'prop1';
clone $obj with (...[$myProp => 'value']);

This also gives the ability to have create dynamic properties via anonymous
function as well, like so:

$myFunc = function ($original): array {
$props = [];
// add results
return $props;
};
clone $obj with (...$myFunc($obj));

You even have the ability to pass it whatever you want including the original
object to operate off of.

Regards,
Brandon Jackson

Tim Düsterhus

3 years ago

Hi

On 5/29/23 13:47, Michał Marcin Brzuchalski wrote:

> So there would be no option to vote on shorthand properties, right?
>
> Array syntax with all properties as strings in quotes probably means no
> support from IDE.
> I think this is a really bad move, I'd be against it.
>

I expect any IDE or SA tool worth its salt to be able to understand that
a string-literal is … a string-literal and treat it as constant. Whether
there are quotes around that literal or not should not make a meaningful
difference for automated processing.

I'm fine with the proposed syntax [1], but agree that "clone with
expressions" would be useful to have even in the initial version.

Best regards
Tim Düsterhus

[1] It's consistent with existing syntax and that is important.

A.L.E.C

3 years ago

On 29.05.2023 11:18, Máté Kocsis wrote:

> Hi Everyone,
>
> In the meanwhile, I changed my proposal to use [] instead of {} after the
> "with" clause due to its better receptance.

In "Property name expressions" section's first code example I think you
intended to do `clone $self` (not `clone $this`) inside the foreach() loop.

Also, I have a feeling that it would be better to implement `clone
$object with $properties` syntax first and if that is accepted think
about expressions support. It's not a strong feeling though.

--
Aleksander Machniak
Kolab Groupware Developer        [https://kolab.org]
Roundcube Webmail Developer  [https://roundcube.net]
----------------------------------------------------
PGP: 19359DC1 # Blog: https://kolabian.wordpress.com

A.L.E.C

3 years ago

On 29.05.2023 14:12, Aleksander Machniak wrote:

> In "Property name expressions" section's first code example I think you
> intended to do `clone $self` (not `clone $this`) inside the foreach() loop.
>
> Also, I have a feeling that it would be better to implement `clone
> $object with $properties` syntax first and if that is accepted think
> about expressions support. It's not a strong feeling though.

Actually, with some more thought, my opinion now is that introduction of
property name expressions is not justified. And will be a reason for me
to vote No.

I'm still not decided about which syntax I'd prefer, maybe you should do
a poll with these basic options. Sorry, if there was one and I missed it.

1) clone $object with $properties;

2) clone($object, $properties);

3) clone($object, prop1: $var1, prop2: $var2);

4) clone($object, function ($clone) use ($properties) {
foreach ($properties as $name => $value) {
$clone->{$name} = $value;
}
});

5) clone $obj with (prop1: $var1, prop2: $var2);


note: $properties is iterable
note: in 2-4) brackets are potentially optional.
note: imho, 5) is similar to 3), but worse.

--
Aleksander Machniak
Kolab Groupware Developer        [https://kolab.org]
Roundcube Webmail Developer  [https://roundcube.net]
----------------------------------------------------
PGP: 19359DC1 # Blog: https://kolabian.wordpress.com

Derick Rethans

3 years ago

On Mon, 17 Apr 2023, Máté Kocsis wrote:

> Hi Everyone,
>
> Quite some time after mentioning the "clone with" construct the first time
> (at the end of the
> https://wiki.php.net/rfc/write_once_properties#run-time_behaviour section),
> finally I managed to create a working implementation for this feature which
> would make it possible to properly modify readonly properties
> while simplifying how we write "wither" methods:
> https://wiki.php.net/rfc/clone_with

| So far, all “clone with” examples introduced in the current RFC used
| fixed property names which were referenced as identifiers followed by
| a colon (:), while the values to be assigned were expressions.
|
| …
|
|  both side of the assignment is an expression, separated by =>

I don't think there should be two syntaxes here. Either allow
expressions on the left side of the ':' or always use '=>' and require a string on
the LHS. The example already allows for using a string on the LHS in
{"foo" => 1}, so I think that the most useful choice. I don't think in
{PROPERTY_NAME => 1} , the LHS should be interpreted as the literal
value PROPERTY_NAME, but only the value of that constant.

cheers,
Derick

Unnamed Person

3 years ago

On Mon, Apr 17, 2023 at 12:32 AM Máté Kocsis <kocsismate90@gmail.com> wrote:

>
> Hi Everyone,
>
> Quite some time after mentioning the "clone with" construct the first time
> (at the end of the
> https://wiki.php.net/rfc/write_once_properties#run-time_behaviour section),
> finally I managed to create a working implementation for this feature which
> would make it possible to properly modify readonly properties
> while simplifying how we write "wither" methods:
> https://wiki.php.net/rfc/clone_with
>
> Regards,
> Máté Kocsis

I apologize if this has been discussed before, as I have fallen very
behind on internals discussions. I think it would be helpful to add an
example where the object being cloned accesses its properties.
Something like this:

$b = clone $a with ["count" => $a->count * 2];

If this is not valid, or if it has unexpected behavior or semantics,
that should also be included in the RFC.

Máté Kocsis

3 years ago

(Resending my earlier message to Levi:)

Hi Levi,

I apologize if this has been discussed before, as I have fallen very

> behind on internals discussions. I think it would be helpful to add an
> example where the object being cloned accesses its properties.
> Something like this:
>
>    $b = clone $a with ["count" => $a->count * 2];
>
> If this is not valid, or if it has unexpected behavior or semantics,
> that should also be included in the RFC.
>

It hasn't been discussed yet, but thankfully, there's nothing special about
it:
the LHS is evaluated first, then comes the RHS. Nevertheless, I've just
clarified this in the RFC,
and thanks for pointing this out!

Regards,
Máté