[RFC] Property hooks, nee accessors

Hi Larry,

pon., 8 maj 2023 o 23:38 Larry Garfield larry@garfieldtech.com napisał(a):

Ilija Tovilo and I would like to offer another RFC for your
consideration. It's been a while in coming, and we've evolved the design
quite a bit just in the last week so if you saw an earlier draft of it in
the past few months, I would encourage you to read it over again to make
sure we're all on the same page. I'm actually pretty happy with where it
ended up, even if it's not the original design. This approach eliminates
several hard-to-implement edge cases while still providing a lot of
functionality in one package.

https://wiki.php.net/rfc/property-hooks

Using $this->propertyName directly is supported, but not recommended.
Why is that? IMHO in the scope of accessor this is much clearer to me than
using the $field.

The following example does make use of $field, however, and thus a
backing value will be created, and write operations will simply write to
the property as normal.
This looks like new magic to me, whether we allow setting and baking the
value explicitly or not. I consider this behavior confusing.

Overall I vote yes.
Cheers,
Michał Marcin Brzuchalski

Hi

Ilija Tovilo and I would like to offer another RFC for your consideration. It's been a while in coming, and we've evolved the design quite a bit just in the last week so if you saw an earlier draft of it in the past few months, I would encourage you to read it over again to make sure we're all on the same page. I'm actually pretty happy with where it ended up, even if it's not the original design. This approach eliminates several hard-to-implement edge cases while still providing a lot of functionality in one package.

https://wiki.php.net/rfc/property-hooks

I've read a previous draft and now did another pass through the updated
version:

(1) How does the set() hook interact with #[\SensitiveParameter]?
(2) How will a Stack Trace emitted from a hook look like? Please include
an example of the Exception::__toString() output or so.
(3) ReflectionProperty::getHook(): Will this throw if an invalid hook
name is given or will this return null?
(4) The "Unaffected PHP Functionality" section is empty. It should
either be filled in or removed.
(5) I strongly dislike the doubly abbreviated form of public string $fullName => $this->first . " " . $this->last;. Having just the extra
'>' in there to distinguish it from a regular property feels non-obvious.

Best regards
Tim Düsterhus

Ilija Tovilo and I would like to offer another RFC for your consideration.

It's been a while in coming, and we've evolved the design quite a bit just
in the last week so if you saw an earlier draft of it in the past few
months, I would encourage you to read it over again to make sure we're all
on the same page. I'm actually pretty happy with where it ended up, even
if it's not the original design. This approach eliminates several
hard-to-implement edge cases while still providing a lot of functionality
in one package.

https://wiki.php.net/rfc/property-hooks

Congrats for the RFC for property hooks, I like it all, including
abbreviated forms, $value as default name, etc.

Some notes:

• does set($value) { mean set(mixed $value) { or set(TypeFromProp
  $value) { ? I understand it would be the latter but it might be nice to
  clarify (unless I missed it)
• function setFullName => ucfirst is not found in the "set" version
• could parent::$x::set($x) be replaced by parent::$x = $x ? Same for
  the get variant? Why not if not?
• readonly: could this be supported by materializing them, using the
  storage for memoization after first access? that'd be quite powerful
• I'm not sure we need ReflectionProperty::getHook('set') when we can
  do getHooks()['set'] ?? null
• What does ReflectionMethod::getName return on a hook? And getClosure?
• Should we add ReflectionProperty::IS_VIRTUAL ?
• I'm a bit surprised by the possibility of adding attributes to hooks.
  I'm not sure I see the use case and I think this might be confusing in
  addition to attributes on the property itself.
• For serialization: exclude them from var_export() and serialize(),
  which don't need virtual props to rebuild the state (you mean
  json_encode(), not JsonSerializable on that list), call get for
  var_dump/print_r when it's defined (but ignore exceptions). But (array) /
  get_mangled_object_vars() / get_object_vars() shouldn't call any "get".

I have a bigger concern: the take on references contradicts with the intro
about BC breaks: turning a materialized property into virtual one would
break BC as far as refs are concerned. One idea to fix that: add a ref
hook, that must return a reference. If not implemented => Error when trying
to get-by-ref. This could also help solve the $foo->array[] case (including
the asym-visiblity issue).

Nicolas

Ilija Tovilo and I would like to offer another RFC for your consideration. It's been a while in coming, and we've evolved the design quite a bit just in the last week so if you saw an earlier draft of it in the past few months, I would encourage you to read it over again to make sure we're all on the same page. I'm actually pretty happy with where it ended up, even if it's not the original design. This approach eliminates several hard-to-implement edge cases while still providing a lot of functionality in one package.

https://wiki.php.net/rfc/property-hooks

Thanks for all the work that has gone into this. It looks great. The
ability to add properties to interfaces is also really nice.

One thing that concerns me is the following: "the use of [] on any
property (with or without a key) will result in a runtime error being
thrown". While the intent of the RFC is to allow adding hooks without
causing BC breaks, this detail does introduce a BC break. And the break
may get introduced without realizing it.

I'm not sure which solution would be best here: Just accept the
limitations for arrays (same as for list properties in Python) or forbid
the use of hooks for array properties? I'm leaning towards accepting the
limitations, like Python does. Then it is up to the API designer to
decide if a property hook is the right tool for the job or not.

Regards,
Dik

On Mon, May 8, 2023 at 11:38 PM Larry Garfield larry@garfieldtech.com
wrote:

Ilija Tovilo and I would like to offer another RFC for your
consideration. It's been a while in coming, and we've evolved the design
quite a bit just in the last week so if you saw an earlier draft of it in
the past few months, I would encourage you to read it over again to make
sure we're all on the same page. I'm actually pretty happy with where it
ended up, even if it's not the original design. This approach eliminates
several hard-to-implement edge cases while still providing a lot of
functionality in one package.

https://wiki.php.net/rfc/property-hooks

Thank you! Looks interesting, I will need to think about things before more
qualitative feedback.

An error maybe, the "class C" example in "Detailed Proposal > set" uses a
"$this->_prop", but probably meant to use $this->_names, since private
array $_names; is declared in the example.

I think $field should be its own "chapter". Its a central part of the
proposal that should be clarified early and so that readers don't
accidentally skip it.

I am also confused why $field exists when $this->propertyName works and why
its not recommended to be used. Is $field a reference? or does the "compile
time macro" part mean its replaced at compile time? Ifso, this feels
different to anything else PHP, i am leaning towards $this->propertyName if
there are no other compelling reasons why $field should be used.

--
Larry Garfield
larry@garfieldtech.com

--

To unsubscribe, visit: https://www.php.net/unsub.php

Ilija Tovilo and I would like to offer another RFC for your consideration. It's
been a while in coming, and we've evolved the design quite a bit just in the last
week so if you saw an earlier draft of it in the past few months, I would
encourage you to read it over again to make sure we're all on the same page.
I'm actually pretty happy with where it ended up, even if it's not the original
design. This approach eliminates several hard-to-implement edge cases while
still providing a lot of functionality in one package.

https://wiki.php.net/rfc/property-hooks

+1 from me, I've used it in C# and it makes for clean code and less boilerplate.

-Jeff

Ilija Tovilo and I would like to offer another RFC for your
consideration. It's been a while in coming, and we've evolved the
design quite a bit just in the last week so if you saw an earlier draft
of it in the past few months, I would encourage you to read it over
again to make sure we're all on the same page. I'm actually pretty
happy with where it ended up, even if it's not the original design.
This approach eliminates several hard-to-implement edge cases while
still providing a lot of functionality in one package.

https://wiki.php.net/rfc/property-hooks

Thanks everyone for your feedback so far. I'm going to collapse the replies into one message to reduce noise.

(1) How does the set() hook interact with #[\SensitiveParameter]?

Internally, the hook is a method, so it should respect it the same way as any other method. Viz, you should be able to write:

public string $foo {
  set ($[\SensitiveParameter] string $value) {
    $field = strtolower($foo);
  }
}

I'll ask Ilija to add a test to confirm that.

(2) How will a Stack Trace emitted from a hook look like? Please include
an example of the Exception::__toString() output or so.

I'll ask Ilija to add a test to his branch and link it here.

(3) ReflectionProperty::getHook(): Will this throw if an invalid hook
name is given or will this return null?

Null if you ask for get/set and one doesn't exist. Error if you ask for something other than "get" or "set". We're still keeping it as a single method, though, to make supporting other hooks easier in the future if desired. I've updated the RFC to clarify that.

(4) The "Unaffected PHP Functionality" section is empty. It should
either be filled in or removed.

Removed. I don't quite understand the purpose of that section. :-)

• does set($value) { mean set(mixed $value) { or set(TypeFromProp
  $value) { ? I understand it would be the latter but it might be nice to
  clarify (unless I missed it)

Conceptually, I believe it means set(TypeFromProp $value). Since at present that cannot be enforced, though, they're basically the same thing. (If someone with more engine-fu than either of us wants to help enforce contravariant types in set, please speak up because we'd prefer to do so.)

• function setFullName => ucfirst is not found in the "set" version

Yes it is. Look at the end of the set line.

• could parent::$x::set($x) be replaced by parent::$x = $x ? Same for
  the get variant? Why not if not?

There's a subtle but important distinction here. parent::$x would suggest accessing the parent property, ie, its backing value, directly. What is actually intended here is accessing the parent hook, ie, its equivalent to parent::setFoo($foo). The property itself is already available as $field.

We're open to alternate syntaxes for accessing the parent accessor syntax if people have good (and easily implementable) suggestions.

• readonly: could this be supported by materializing them, using the
  storage for memoization after first access? that'd be quite powerful

That was one of my early ideas, as it then became a cheap form of cached lazy property. However, that then means there's no meaningful way to unset the cache. In the end we decided that there were too many nooks and crannies to readonly to try and make sense of. (The more I use it, the more I think readonly was a mistake and we should have gone straight to aviz.)

(As a side note, if we had aviz, then we could use the field itself as a cache by making writes private, like so:

public string $name => $field ??= $this->first . $this->last;

But right now that would also imply public-set, which we wouldn't want. Aviz would solve that issue entirely. Another reason aviz is an important and useful feature. :-) )

What a readonly flag on a set-only property means... I have no idea.

• I'm not sure we need ReflectionProperty::getHook('set') when we can
  do getHooks()['set'] ?? null

The double-methods were mainly for consistency with the rest of the Reflection API, which tends to have both getFoo(): object and getFoos(): array options. It's no more redundant than the rest of Reflection.

• What does ReflectionMethod::getName return on a hook? And getClosure?

Either "get" or some hashed string that includes "get". I'm not sure off hand. I'll have to check with Ilija.

• Should we add ReflectionProperty::IS_VIRTUAL ?

Funny story. I just asked Ilija this, and he said he already did add it for testing purpose but forgot to tell me. :-) RFC updated.

• I'm a bit surprised by the possibility of adding attributes to hooks.
  I'm not sure I see the use case and I think this might be confusing in
  addition to attributes on the property itself.

I think in practice it's actually more work not to, since hooks under the hood are just methods like anything else. And as Tim noted above, there are use cases like #[SensitiveParameter] for attributes even down here.

• For serialization: exclude them from var_export() and serialize(),
  which don't need virtual props to rebuild the state (you mean
  json_encode(), not JsonSerializable on that list), call get for
  var_dump/print_r when it's defined (but ignore exceptions). But (array) /
  get_mangled_object_vars() / get_object_vars() shouldn't call any "get".

I don't follow. json_encode() is on the list.

I have a bigger concern: the take on references contradicts with the intro
about BC breaks: turning a materialized property into virtual one would
break BC as far as refs are concerned. One idea to fix that: add a ref
hook, that must return a reference. If not implemented => Error when trying
to get-by-ref. This could also help solve the $foo->array[] case (including
the asym-visiblity issue).

That might be possible. The issue there is that the returned ref would still be bypassing the get/set hooks, so you're just adding a way to dance around the existing hook implementations, which is exactly what we're trying to avoid. Also, what would that hook even mean if used on a virtual property?

Moreover, in practice, getting a reference to a property is extremely rare, with the exception of array writes. I cannot recall the last time I even saw some code get a reference to a property. That's why we felt comfortable just leaving it at is. In an earlier version of the RFC we had a way for properties to just disable references without adding any hooks, but removed it on the grounds that it was not worth the effort.

I'm not sure which solution would be best here: Just accept the
limitations for arrays (same as for list properties in Python) or forbid
the use of hooks for array properties? I'm leaning towards accepting the
limitations, like Python does. Then it is up to the API designer to
decide if a property hook is the right tool for the job or not.

We considered disallowing it on array properties; the problem is, there's also mixed and iterable properties that would have the same challenge, but that couldn't be detected at compile time. That means the runtime check has to be there anyway, so also having a compile-time check for hooks on arrays doesn't really buy us anything but more engine code. So yes, Python-style "just deal with it sunglasses" is the most logical approach.

An error maybe, the "class C" example in "Detailed Proposal > set" uses a
"$this->_prop", but probably meant to use $this->_names, since private
array $_names; is declared in the example.

Yep, typo, thanks. Fixed now.

I think $field should be its own "chapter". Its a central part of the
proposal that should be clarified early and so that readers don't
accidentally skip it.

I am also confused why $field exists when $this->propertyName works and why
its not recommended to be used. Is $field a reference? or does the "compile
time macro" part mean its replaced at compile time? Ifso, this feels
different to anything else PHP, i am leaning towards $this->propertyName if
there are no other compelling reasons why $field should be used.

Regarding $field vs. $this->propName, there's a few reasons we went that route.

1. It's shorter and less typing for what will be a common pattern.
2. That makes it consistent between hook implementations. In working on examples, I found many cases where I was adding basically the same line to multiple hooks on the same class. Making the code easier to copy-paste seems like a win.
3. It also will be helpful if hook packages are added in the future, as they'll need a more "generic" way to access the backing property. (See Future Scope.) Eg, "$field = someLogic($value)" applied to a dozen different properties; it wouldn't work if it was "$this->specificProperty = someLogic($value)".
4. We're used to our eyes glossing over "$this->propName", as it's so common. Having a separate name to mentally scan for to determine if a property is virtual or not seems like it will be helpful in practice.
5. There's precedent for it: Kotlin has almost the same functionality as we describe here, and uses a field variable in the exact same way.

So it's mainly an ergonomics argument rather than a technical one. "Compile time macro" means it translates to the same AST as if you'd used $this->propName. There's precedent for that. Constructor Property Promotion works basically the same way.

--Larry Garfield

Ilija Tovilo and I would like to offer another RFC for your consideration. It's been a while in coming, and we've evolved the design quite a bit just in the last week so if you saw an earlier draft of it in the past few months, I would encourage you to read it over again to make sure we're all on the same page. I'm actually pretty happy with where it ended up, even if it's not the original design. This approach eliminates several hard-to-implement edge cases while still providing a lot of functionality in one package.

https://wiki.php.net/rfc/property-hooks

--
Larry Garfield
larry@garfieldtech.com

--

To unsubscribe, visit: https://www.php.net/unsub.php

Hi Larry.

public string $fullName => $this->first . " " . $this->last;

1. Suppose that I forgot to declare $this->first. Based on the
   "deprecate dynamic properties" proposal, will I get an error/warning?

2. The shorthand notations supported (the shortest one) creates
   impaired syntax, and not pretty to look at for constructor property
   promotion.

public function __construct(
  public string $prop1 => strtoupper($this->_prop1),
  public string $prop2 {set => $field = strtolower($value);},
){}

My suggestion is: use get/set keyword immediately after property
name/default value rather than "=>" or "{" without losing multiline
statements.

public function __construct(
  public string $prop1 get => strtoupper($this->_prop1),
  public string $prop2 set => $field = strtolower($value),
  public string $prop3 get {
    $temp = strtoupper($this->_prop1);
    return substr($temp, 0, 10) . '...';
  }
  public string $prop4 set {
    $temp = strtolower($value);
    $field = substr($temp, 0, 10);
  }
){}

This syntax is aligned with a single statement if/for/foreach.

Ilija Tovilo and I would like to offer another RFC for your consideration. It's been a while in coming, and we've evolved the design quite a bit just in the last week so if you saw an earlier draft of it in the past few months, I would encourage you to read it over again to make sure we're all on the same page. I'm actually pretty happy with where it ended up, even if it's not the original design. This approach eliminates several hard-to-implement edge cases while still providing a lot of functionality in one package.

https://wiki.php.net/rfc/property-hooks

--
Larry Garfield
larry@garfieldtech.com

--

To unsubscribe, visit: https://www.php.net/unsub.php

Hi Larry.

Suppose that I just want to cache once and freeze, and I don't want to
create additional hidden property. Please make a judgement on the
validity of this syntax:

public string $prop1 {
  get => $field ??= $this->heavyTask($this->propx, $prop->y);
  set => null;
}

Explanation:

• Since the get hook uses $field, the engine will create a
  backing store value with name $prop1, and will be filled at the
  first call only. The later call will use the already stored value.
• Since no new value is assigned to $field in the set
  hook, the value stored in $prop1 is still the same as before.
• Since any return statement will be discharged in set hook, the
  assignment chaining is safe to use and the value which is transferred
  is alway the rightmost value (not the value returned by the set
  hook).

$varx = $obj->prop1 = "hello";  // $varx value is "hello", and
$obj->prop1 value is unchanged

But, this statement below is NOT ALWAYS correct for "cache once and freeze":

public string $prop1 {
  set => $field ??= $this->heavyTask($this->propx, $prop->y);
}

Explanation:

• Since default get hook can be called before set hook,
  backing store value with name $prop1 will still be empty.
• Otherwise, I have to duplicate the body of the set hook into
  get hook to prevent an emptiness backing store.

Please give a comment.
Thanks.

Ilija Tovilo and I would like to offer another RFC for your consideration. It's been a while in coming, and we've evolved the design quite a bit just in the last week so if you saw an earlier draft of it in the past few months, I would encourage you to read it over again to make sure we're all on the same page. I'm actually pretty happy with where it ended up, even if it's not the original design. This approach eliminates several hard-to-implement edge cases while still providing a lot of functionality in one package.

https://wiki.php.net/rfc/property-hooks

--
Larry Garfield
larry@garfieldtech.com

Hi, Larry

interface IFace
{
  public string $readable { get; }
  public string $writeable { set; }
  public string $both { get; set; }
}

How important is { get; } statement (and friends) in the interface?

Since PHP does not enforce method definition existence in caller site,
this code is valid (at least at compile time):

interface IA {
  // ...
}

interface IB {
  public function doIt(IA $par1);
}

class CA1 implements IA {
  // makeIt is not part of IA
  public function makeIt() {
    echo " makeIt() is called";
  }
}

class CA2 implements IA {}

class CB implements IB {
  public function doit(IA $par1) {
    // makeIt is not enforced to be invalid
    $par1->makeit();
  }
}

$a1 = new CA1;
$a2 = new CA2;
$b = new CB;
$b->doIt($a1); // valid
$b->doIt($a2); // invalid at runtime

IMO, { get; } (and friends) only useful if there are:

• definition existence enforcement or implemented asymmetric visibility
• AND no default hook implementation provided.

I assume that { get; } do exist in the property accessors
proposal because there is no default hook implementation. Whether
there is already implemented property definition existence enforcement
or not, i don't know. If yes, then all things are in the right place.
If not, it feels like a half baked feature, just like the method was:
the error will pop up at runtime. The only positive value is { get; } can still be consumed by static analysis tools.

class CX {
  public function consumeIt(IFace $par) {
    // this statement should trigger error
    // produced by engine or SA tools
    $par->readable = "hello";
  }
}

Things will be different with property hooks. Without definition
existence enforcement nor implemented asymmetric visibility, static
analysis tools will give misconception about unlisted hooks if there
is error/warning message.

class CX {
  public function consumeIt(IFace $par) {
    // default hook is provided by engine
    // any message is not relevant for this valid statement
    $par->readable = "hello";
  }
}

If there is no forward compatibility consideration, can we simplify
interface definition to this one?

interface IFace
{
  public string $readable;
  public string $writeable;
  public string $both;
}

Thanks in advance.

Ilija Tovilo and I would like to offer another RFC for your consideration. It's been a while in coming, and we've evolved the design quite a bit just in the last week so if you saw an earlier draft of it in the past few months, I would encourage you to read it over again to make sure we're all on the same page. I'm actually pretty happy with where it ended up, even if it's not the original design. This approach eliminates several hard-to-implement edge cases while still providing a lot of functionality in one package.

https://wiki.php.net/rfc/property-hooks

--
Larry Garfield
larry@garfieldtech.com

Hi Larry

Can the engine detect these logical errors?

public string $fullName = "hendra gunawan" => $this->first . " " . $this->last;

// or with my proposal:
public string $fullName = "hendra gunawan" get => $this->first . " " .
$this->last;

// or even this one:
public string $fullName = "hendra gunawan" {
  get => $this->first . " " . $this->last;
  set => [$this->first, $this->last] = explode(" ", $value);

Explanation:
There is no automatic backing store value created to store the default value.

Terima Kasih.

Ilija Tovilo and I would like to offer another RFC for your
consideration. It's been a while in coming, and we've evolved the
design quite a bit just in the last week so if you saw an earlier draft
of it in the past few months, I would encourage you to read it over
again to make sure we're all on the same page. I'm actually pretty
happy with where it ended up, even if it's not the original design.
This approach eliminates several hard-to-implement edge cases while
still providing a lot of functionality in one package.

https://wiki.php.net/rfc/property-hooks

Hi folks. Based on feedback we've made a few smaller changes to the RFC.

Changelog:

• The sections describing isset/unset and magic methods have been rewritten to be clearer. Nothing changed in the actual behavior, it is now just explained better.

• Contravariance on the "set" type is now enforced. Ilija figured out how to make it work. :-) That means a non-contravariant type on the set hook will now throw an error, as expected.

• After extensive discussion with Nicolas Grekas, we've decided to allow references in a very narrow case. Specifically, on a virtual property (one that has no inherent backing store), you may implement a &get hook instead of get, and the value it returns will be returned by reference, and may be captured using $foo =& $bar->baz; We determined that it is a bit better for BC (in the rare cases that you actually want to get a reference to a property, you can, even if it requires a little more work), doesn't break anything else, and is consistent with the way &__get behaves. __get/__set are basically "anonymous virtual properties", so now they behave the same way. By the same token, setting by reference is still not allowed, which is also true for __set today.

As there was no other interest in it stated, we're going to hold off on an unset hook at this time. Given the earlier discussion I think there is a valid use case for it, so it would be a worthwhile follow up RFC in the future, but for the moment we want to keep it simple.

There also doesn't seem to be much interest in specifying which hook gets the double-shortened syntax. I can see the argument for it, but it would increase the typing in a common case for an unclear benefit, and only one person expressed any interest in it. So we're not going to go that route.

There's two items still pending.

1. Ilija is experimenting with the parent::$prop::get() syntax, to see if either the syntax or implementation can be simplified. There may or may not be a small change here as a result, TBD.

2. Ilija still has to verify that foreach() can work with virtual properties as the RFC currently describes. The implementation details are thornier than they seem, so that still needs some validation and testing.

Assuming both of those get sorted out soon, we will probably call a vote around the end of the month, give or take.

Cheers.

--Larry Garfield