Everything

php

[RFC] Static property asymmetric visibility

php.internals

Larry Garfield

1 year ago

Static property asymmetric visibility was left out of the original RFC, because it seemed like it would be hard and of little use.  Turns out, Ilija found a way to make it easy.  (Ilija is smart.)  So here's a small RFC to add aviz to static properties, because we can't think of a reason to NOT do so.

https://wiki.php.net/rfc/static-aviz

--
Larry Garfield
larry@garfieldtech.com

Tim Düsterhus

1 year ago

Hi

Am 2024-11-25 17:52, schrieb Larry Garfield:

> Static property asymmetric visibility was left out of the original RFC,
> because it seemed like it would be hard and of little use.  Turns out,
> Ilija found a way to make it easy.  (Ilija is smart.)  So here's a
> small RFC to add aviz to static properties, because we can't think of a
> reason to NOT do so.
>
> https://wiki.php.net/rfc/static-aviz

The RFC has the status “Draft” and does not appear to be linked in the
overview at https://wiki.php.net/rfc. Is it intended to be open for
discussion yet?

Regarding the RFC contents: I am not sure the behavior of `private(set)`
implying `final` is obvious for static properties. Consider this example
(https://3v4l.org/bvQjM):

<?php

class Foo {
public static $baz;
}

class Bar extends Foo {
public static $baz;
}

Foo::$baz = '2';
Bar::$baz = '1';
var_dump(Foo::$baz, Bar::$baz);

Non-static properties only have `$this->` for property access, but with
static properties you can distinguish between `self::` and `static::`,
which makes overriding a `private(set)` static property meaningful,
because they are actually independent properties.

Best regards
Tim Düsterhus

Larry Garfield

1 year ago

On Tue, Nov 26, 2024, at 3:57 AM, Tim Düsterhus wrote:

> Hi
>
> Am 2024-11-25 17:52, schrieb Larry Garfield:
>> Static property asymmetric visibility was left out of the original RFC,
>> because it seemed like it would be hard and of little use.  Turns out,
>> Ilija found a way to make it easy.  (Ilija is smart.)  So here's a
>> small RFC to add aviz to static properties, because we can't think of a
>> reason to NOT do so.
>>
>> https://wiki.php.net/rfc/static-aviz
>
> The RFC has the status “Draft” and does not appear to be linked in the
> overview at https://wiki.php.net/rfc. Is it intended to be open for
> discussion yet?

Paperwork oversight on my part.  Fixed now, thanks.

> Regarding the RFC contents: I am not sure the behavior of `private(set)`
> implying `final` is obvious for static properties. Consider this example
> (https://3v4l.org/bvQjM):
>
>      <?php
>
>      class Foo {
>          public static $baz;
>      }
>
>      class Bar extends Foo {
>          public static $baz;
>      }
>
>      Foo::$baz = '2';
>      Bar::$baz = '1';
>      var_dump(Foo::$baz, Bar::$baz);
>
> Non-static properties only have `$this->` for property access, but with
> static properties you can distinguish between `self::` and `static::`,
> which makes overriding a `private(set)` static property meaningful,
> because they are actually independent properties.
>
> Best regards
> Tim Düsterhus

Hm, interesting point.  I am not sure of the best way to model that.

Thinking aloud, my expectation would be that it behaves similarly to how final static methods would behave.  Which appears to be a syntax error:  https://3v4l.org/j8mp0#v8.4.1

So, shouldn't properties do the same?

--Larry Garfield

Jonathan Vollebregt

1 year ago

On 11/26/24 9:35 PM, Larry Garfield wrote:

> Thinking aloud, my expectation would be that it behaves similarly to how final static methods would behave.  Which appears to be a syntax error:  https://3v4l.org/j8mp0#v8.4.1
>
> So, shouldn't properties do the same?

Without final you can still override both private static properties and
private static methods: https://3v4l.org/MS73Y#v8.4.1

When explicitly declared final, static properties do result in a syntax
error: https://3v4l.org/fqI8v#v8.4.1

Re-reading the logic in the original aviz RFC makes me think implicit
final here is unnecessary. All static properties are "Shadowed" like
private properties (even when they're public) so there's no conflicting
behavior.

The two behaviors described as conflicting in the aviz RFC are decided
explicitly in the context of static properties, by the caller accessing
it with `self::` or `static::`. Not by a combination of the visibility
and child classes.

Consider this example:

```
class A {
private(set) static int $a = 1;

public static function test(int $val) {
static::$a = $val;
}
}

class B extends A {
private(set) static int $a = 2;
}

B::test(3)
```

Yes this would produce a fatal error, but doing this with just `private
static` does the same in current PHP: https://3v4l.org/Y6lZ7#v8.4.1

You might want to discuss banning use of `static::` on private statics,
but that's a big BC break.

Since static properties do still have to have equal or wider visibility
when extending I'd say using `static::$prop` on a property you know is
private is a known risk and remove the implicit final.

- Jonathan

Larry Garfield

1 year ago

On Tue, Nov 26, 2024, at 3:42 PM, Jonathan Vollebregt wrote:

> On 11/26/24 9:35 PM, Larry Garfield wrote:
>> Thinking aloud, my expectation would be that it behaves similarly to how final static methods would behave.  Which appears to be a syntax error:  https://3v4l.org/j8mp0#v8.4.1
>>
>> So, shouldn't properties do the same?
>
> Without final you can still override both private static properties and
> private static methods: https://3v4l.org/MS73Y#v8.4.1
>
> When explicitly declared final, static properties do result in a syntax
> error: https://3v4l.org/fqI8v#v8.4.1
>
> Re-reading the logic in the original aviz RFC makes me think implicit
> final here is unnecessary. All static properties are "Shadowed" like
> private properties (even when they're public) so there's no conflicting
> behavior.
>
> The two behaviors described as conflicting in the aviz RFC are decided
> explicitly in the context of static properties, by the caller accessing
> it with `self::` or `static::`. Not by a combination of the visibility
> and child classes.
>
> Consider this example:
>
> ```
> class A {
>      private(set) static int $a = 1;
>
>      public static function test(int $val) {
>          static::$a = $val;
>      }
> }
>
> class B extends A {
>      private(set) static int $a = 2;
> }
>
> B::test(3)
> ```
>
> Yes this would produce a fatal error, but doing this with just `private
> static` does the same in current PHP: https://3v4l.org/Y6lZ7#v8.4.1
>
> You might want to discuss banning use of `static::` on private statics,
> but that's a big BC break.
>
> Since static properties do still have to have equal or wider visibility
> when extending I'd say using `static::$prop` on a property you know is
> private is a known risk and remove the implicit final.
>
> - Jonathan

Hi folks.  Finally getting back to this thread.

Ilija and I have discussed it, and it seems to us that the consistency of implicit-final-on-private-set (on both object and static properties) is more valuable than maximal flexibility, especially as we're dealing with a rare use case to begin with.

Specifically, `static::$prop` is still expected to behave the same as the parent class's $prop.  Eg, its type can't change, its visibility cannot be narrowed, etc.  So a private(set) parent $prop would violate that expectation, the same way it would for a dynamic property.  Introducing another subtle difference between self:: and static:: doesn't seem like a good thing for DX.

Additionally, if we find in the future that it really would be better to not have the implicit final and allow people to "opt in" via self::vs static::, it's less of a BC break to remove the implicit final in the future than to try and introduce it.

For that reason, we are going to keep static properties consistent with object properties in this regard.

--Larry Garfield

Larry Garfield

1 year ago

On Mon, Nov 25, 2024, at 10:52 AM, Larry Garfield wrote:

> Static property asymmetric visibility was left out of the original RFC,
> because it seemed like it would be hard and of little use.  Turns out,
> Ilija found a way to make it easy.  (Ilija is smart.)  So here's a
> small RFC to add aviz to static properties, because we can't think of a
> reason to NOT do so.
>
> https://wiki.php.net/rfc/static-aviz

Heads up: Absent any other feedback, I'm going to call the vote on this RFC early next week.

--Larry Garfield

Niels Dossche

1 year ago

On 04/01/2025 00:14, Larry Garfield wrote:

> On Mon, Nov 25, 2024, at 10:52 AM, Larry Garfield wrote:
>> Static property asymmetric visibility was left out of the original RFC,
>> because it seemed like it would be hard and of little use.  Turns out,
>> Ilija found a way to make it easy.  (Ilija is smart.)  So here's a
>> small RFC to add aviz to static properties, because we can't think of a
>> reason to NOT do so.
>>
>> https://wiki.php.net/rfc/static-aviz
>
> Heads up: Absent any other feedback, I'm going to call the vote on this RFC early next week.
>
> --Larry Garfield

Hi

I'm not sure how I feel about this.
The current implementation actually uses a workaround because otherwise it interferes with cache slot merging.
This creates duplicated code in the VM handlers. Although the complication is small, it still is one. And the complication may be duplicated in the future in the JIT.
Workarounds/complications can be fine if having the feature is justifiable, but you said yourself that it seems of little use and the RFC text says it's just for completeness.
So honestly, I'm conflicted and leaning towards voting "no".

Kind regards
Niels

Ilija Tovilo

1 year ago

Hi Niels

On Tue, Jan 7, 2025 at 7:37 PM Niels Dossche <dossche.niels@gmail.com> wrote:

>
> On 04/01/2025 00:14, Larry Garfield wrote:
> >>
> >> https://wiki.php.net/rfc/static-aviz

> I'm not sure how I feel about this.
> The current implementation actually uses a workaround because otherwise it interferes with cache slot merging.
> This creates duplicated code in the VM handlers. Although the complication is small, it still is one. And the complication may be duplicated in the future in the JIT.
> Workarounds/complications can be fine if having the feature is justifiable, but you said yourself that it seems of little use and the RFC text says it's just for completeness.
> So honestly, I'm conflicted and leaning towards voting "no".

I wouldn't necessarily call this a workaround, but more of a missed
optimization (one branch for each static property write that is
unlikely to mispredict). If you wish, I can have a look at separating
cache slots. This may lead to a slowdown due to cache priming (as only
R/IS and RW/W/UNSET will be shared). To be honest, I doubt either of
those will lead to a measurable difference in real code. I can confirm
this by running some benchmarks. The benefit of separate cache slots
is that it can be entirely handled in
zend_fetch_static_property_address(), which would reduce VM/ future
JIT changes, although the separation of the cache slots themselves
might be more complex (given how it's currently implemented).

Ilija

Niels Dossche

1 year ago

On 07/01/2025 19:49, Ilija Tovilo wrote:

> I wouldn't necessarily call this a workaround, but more of a missed
> optimization (one branch for each static property write that is
> unlikely to mispredict). If you wish, I can have a look at separating
> cache slots. This may lead to a slowdown due to cache priming (as only
> R/IS and RW/W/UNSET will be shared). To be honest, I doubt either of
> those will lead to a measurable difference in real code. I can confirm
> this by running some benchmarks. The benefit of separate cache slots
> is that it can be entirely handled in
> zend_fetch_static_property_address(), which would reduce VM/ future
> JIT changes, although the separation of the cache slots themselves
> might be more complex (given how it's currently implemented).
>

I think splitting cache slots will indeed be worse than what it is now.
However, that argument in itself just doesn't really convince me honestly.
You are likely right about the branch prediction, but the CPU still has to do some extra work; and it's also just more things to think about as a maintainer.
I'll think about it more, maybe I'll just abstain from voting.

Kind regards
Niels

Larry Garfield

1 year ago

On Tue, Jan 7, 2025, at 1:21 PM, Niels Dossche wrote:

> On 07/01/2025 19:49, Ilija Tovilo wrote:
>> I wouldn't necessarily call this a workaround, but more of a missed
>> optimization (one branch for each static property write that is
>> unlikely to mispredict). If you wish, I can have a look at separating
>> cache slots. This may lead to a slowdown due to cache priming (as only
>> R/IS and RW/W/UNSET will be shared). To be honest, I doubt either of
>> those will lead to a measurable difference in real code. I can confirm
>> this by running some benchmarks. The benefit of separate cache slots
>> is that it can be entirely handled in
>> zend_fetch_static_property_address(), which would reduce VM/ future
>> JIT changes, although the separation of the cache slots themselves
>> might be more complex (given how it's currently implemented).
>>
>
> I think splitting cache slots will indeed be worse than what it is now.
> However, that argument in itself just doesn't really convince me
> honestly.
> You are likely right about the branch prediction, but the CPU still has
> to do some extra work; and it's also just more things to think about as
> a maintainer.
> I'll think about it more, maybe I'll just abstain from voting.
>
> Kind regards
> Niels

Late follow-up here.  Ilija finally managed to squeeze in time to benchmark this.  Synthetic benchmarks were inconclusive.  A practical benchmark using Symfony Demo showed the code from this patch as 0.03% faster than HEAD, which is within the margin of error of the benchmark, so it's basically a wash/no-impact.

With that settled, I will be opening the vote on this RFC tomorrow sometime, baring any last second questions or complications.

--Larry Garfield

Claude Pache

1 year ago

> Le 25 nov. 2024 à 17:52, Larry Garfield <larry@garfieldtech.com> a écrit :
>
> Static property asymmetric visibility was left out of the original RFC, because it seemed like it would be hard and of little use.  Turns out, Ilija found a way to make it easy.  (Ilija is smart.)  So here's a small RFC to add aviz to static properties, because we can't think of a reason to NOT do so.
>
> https://wiki.php.net/rfc/static-aviz

Hi,

About use case: Looking at my code, I’ve found places where I store data (e.g. global config values) in public static properties, but those data is supposed to be initialised and/or modified only inside the class.

In those cases, aviz would allow me to document and enforce what is currently a gentleman's agreement.

—Claude