Basic accessors implementation (WIP) #6873

nikic · 2021-04-16T16:13:17Z

No description provided.

Zend/tests/accessors/backing_prop.phpt

This doesn't properly handle auto-generated accessors yet.

This allows us to treat all accessors uniformly in most places. This would make it easier to support additional accessor kinds in the future.

This allows them to be used from reflection.

This can be supported, but would require additional implementation effort.

This is ugly, but necessary.

This matches the behavior we'd get with an implicit implementation, but also means that these would be less efficient :/

Apparently inlining zend_is_true() adversely affects isset() here.

This doesn't quite hold if we switch to a shadowed parent property. At the very least the error message may differ in the write case.

To be consistent with the treatment of abstract.

This was previously only checked for accessors.

TysonAndre · 2021-05-05T00:38:02Z

Zend/tests/accessors/final_private_prop.phpt

+<?php
+
+class Test {
+    final private $prop;


I'm still reading this rfc - final protected $prop { private get; private set; } is allowed and would work similarly to a private final property in terms of forbidding redefining it in subclasses? The only major difference would be serialize() output and ReflectionProperty behavior.

I guess that's consistent with private final methods recently being forbidden.

Yes. I was against that change at the time, but now we have to follow suit...

Zend/tests/accessors/interface_not_public.phpt

Zend/tests/accessors/interface_not_public_2.phpt

TysonAndre · 2021-05-05T01:24:42Z

Zend/zend_compile.c

+					"Accessor in interface cannot be explicitly abstract. "
+					"All interface members are implicitly abstract");
+			}
+			accessor->flags |= ZEND_ACC_ABSTRACT;


Would it also make sense to either (1) forbid the final qualifier on accessors in interfaces for now, or (2) to remove the abstract flag from accessors declared on properties of interfaces?

It'd be useful to have tests of the intended behavior.

E.g. right now, it seems impossible to override just the setter for an interface with a final getter

php > interface I { public $prop1 { final get; }} php > class A implements I { public $prop1; } php > class B implements I { public $prop1 { set($value) { echo "set $value\n"; } } } Fatal error: Class B contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (I::$prop1::get) in php shell code on line 1

Final + abstract is definitely supposed to be forbidden -- and interfaces are a form of abstract. But because the abstract is implicit, it wasn't enforced correctly... I've now added some additional checks and tests for this.

Zend/tests/accessors/final_prop_2.phpt

TysonAndre · 2021-05-05T01:51:11Z

Zend/tests/accessors/get_by_ref.phpt

+    public $_byVal = [];
+    public $byVal {
+        get { return $this->_byVal; }
+        set { $this->_byVal = $value; }


Would it make sense to reject $this and the names of superglobals if the implicit parameter name would be forbidden if used as an explicit parameter name (only for checking set {)

php > class B { public $_SERVER { set ($_SERVER) { var_dump($_SERVER); } }} Fatal error: Cannot re-assign auto-global variable _SERVER in php shell code on line 1 php > class A { public $_SERVER { set { var_dump($_SERVER); } }} php > $a = new A(); $a->_SERVER = 123; array(78) { // ...

The implicit parameter name is (currently) always $value, not the property name. As this goes through the normal compilation code for functions, it should also enforce all the usual rules for parameter names.

Zend/tests/accessors/unknown_accessor.phpt

TysonAndre · 2021-05-05T02:13:44Z

Zend/tests/accessors/unserialize.phpt

+$test2->prop3;
+$test2->prop3 = 42;
+
+// TODO: This should probably be forbidden.


I'd agree as a maintainer of igbinary -- for computed properties such as prop3 - __serialize and __unserialize could be added by applications in cases where data serialized for older versions of the classes was serialized with values for properties that later became magic accessors. (and saved to a cache/db to be read much later on)

The alternative of supporting it by calling set sounds much worse, since computed get wouldn't be serialized and php is moving towards forbidding calling functions in unserialize on objects that haven't been fully initialized yet.

You've already forbidden overriding plain properties with accessor properties, so it doesn't seem likely for applications to set something they couldn't read with the same definition of a class and ancestors.

You've already forbidden overriding plain properties with accessor properties, so it doesn't seem likely for applications to set something they couldn't read with the same definition of a class and ancestors.

You are allowed to override a property with implicit accessors with one with explicit accessors though, so there might be a problem there...

What I've implemented now is that this is not allowed for purely virtual properties, but is allowed if you had a parent with backing storage. In that case the child will also have backing storage, and I think that's reasonable in that the child could be making use of the parent (once there is some way to do that...)

The abstract + final combination is usually prohibited when the flags are parsed in the first place, but here the abstract is implicit, so we need to check it explicitly.

However, it is allowed if there is a parent for which the property was non-virtual.

bwoebi · 2021-05-06T10:35:01Z

Zend/zend_compile.c

+				zend_error_noreturn(E_COMPILE_ERROR, "Abstract accessor cannot have body");
+			}
+			if (accessor->flags & ZEND_ACC_PRIVATE) {
+				// TODO Trait exception?


I don't think a trait should be able to impose a private visibility. But a trait implementing class should be able to override any property visibility and decide to implement the abstract property itself with a private visibility.
I.e. the trait itself should not care about who can implement its property. It's none of his business and we should not make it seem like the trait had any say in that.

In short: if there is an abstract property, the class which implements the trait shall be able to a) define the property with any visibility or b) not implement it at all and the default visibility of the property is retained. In scenario b) private does not make any sense and hence should be forbidden.

Note that traits can already define private abstract methods, so I think allowing this would be in line with the current behavior. https://3v4l.org/FA0Ar#v8.1.5

bwoebi · 2021-05-06T11:24:23Z

Zend/zend_compile.h

@@ -381,6 +395,7 @@ typedef struct _zend_property_info {
 	HashTable *attributes;
 	zend_class_entry *ce;
 	zend_type type;
+	zend_function **accessors;


Is this some sort of memory optimization or why double indirect pointer instead of zend_function *accessors[ZEND_ACCESSOR_COUNT];?

I was thinking ahead to when we have more accessors, and didn't want to allocate storage for them unconditionally. A bit weird if only two are involved.

Maybe a nice option would be to use a flexible array member.

mvorisek · 2021-06-20T06:41:45Z

Zend/tests/accessors/default_on_virtual.phpt

+    public $prop = 0 {
+        get { return $this->_prop; }
+        set { $this->_prop = $value; }
+    }


Suggested change

public $prop = 0 {

get { return $this->_prop; }

set { $this->_prop = $value; }

}

public $prop {

get { return $this->_prop; }

set { $this->_prop = $value; }

} = 0;

is allowed by C#, why such syntax is not supported? Supporting default values would be very helpful.

skie · 2021-07-03T21:16:10Z

Does it have sense to consider not only {get; set;} pair but also {get; init;} for immutable structures like in it is done in C# 9.0? It is much more easy to add such feature on the initial phase of adding property accessors into php language.

NeilMasters · 2022-06-06T11:48:57Z

@nikic to say that a large part of the community desires this change is an understatement. Thank you for taking it on.

This PR seems to have stagnated a bit and although it is unlikely I can offer any tangible assistance if there is please do a shout out.

UweKeim · 2022-12-22T16:39:29Z

Having to switch back and forth between developing C# and PHP code every now and then, I would love to have properties in a way, C# does have.

Would also make it way more easier and straightforward to migrate C# code to PHP.

mx03 · 2022-12-22T17:47:54Z

Having to switch back and forth between developing C# and PHP code every now and then, I would love to have properties in a way, C# does have.

Would also make it way more easier and straightforward to migrate C# code to PHP.

rfcs working on it:

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

rodrigoslayertech · 2023-08-05T05:10:41Z

Awesome feature! Very excited to be approved.

3UR · 2023-08-21T06:52:09Z

when will this be a thing?

rodrigoslayertech · 2023-08-22T14:54:24Z

The RFC asymmetric-visibility can be incorporated into this RFC:

* The example below is not the best (I got it from the RFC).

class User implements Named
{
    private bool $isModified = false;
 
    public function __construct(private string $first, private string $last) {}
 
    public string $fullName {
        // Override the "read" action with arbitrary logic.
        public get => $this->first . " " . $this->last;
 
        // Override the "write" action with arbitrary logic.
        private set($value) => [$this->first, $this->last] = explode(' ', $value);
    }
}

Asymmetric visibility in hooks public get() ... private set($value)!
Once you set the visibility of the hooks, the visibility on the property will always be public.

AwokeKnowing · 2023-12-08T17:54:16Z

I really hope this gets accepted. My company surely would benefit, and i'm sure this would be one of the most exciting features ever. being able to seemlesly transition from simple constructor property promotion based objects to occasionally add some logic after the fact to some of them would be very in keeping with the php tradition of minimal unceremonious code most of the time.

6XGate · 2024-05-02T15:27:38Z

This is the one feature that I feel PHP really needs to finally become the end-all of languages for back-end web development, and maybe more.

rodrigoslayertech · 2024-05-02T16:03:58Z

This is the one feature that I feel PHP really needs to finally become the end-all of languages for back-end web development, and maybe more.

Already accepted with 'RFC Property hooks' based on this RFC.

bwoebi · 2024-05-02T16:14:25Z

I suppose we can close this PR as obsolete by now.

nikic added the RFC label Apr 16, 2021

nikic marked this pull request as draft April 16, 2021 16:13

mvorisek reviewed Apr 17, 2021

View reviewed changes

Zend/tests/accessors/backing_prop.phpt Outdated Show resolved Hide resolved

nikic force-pushed the accessors-2 branch 4 times, most recently from a72a9b0 to 9866518 Compare April 27, 2021 16:00

nikic force-pushed the accessors-2 branch from 9866518 to 1636e32 Compare April 30, 2021 09:16

nikic added 22 commits May 4, 2021 14:50

Basic accessors implementation

fd951b9

Support (generated) accessors in property promotion

6a7269b

Suppress compile warning

6f6f4a2

Add basic reflection support

bce7dda

This doesn't properly handle auto-generated accessors yet.

Represent accessors using an array

484dfa9

This allows us to treat all accessors uniformly in most places. This would make it easier to support additional accessor kinds in the future.

Generate proper implementations for auto accessors

672b615

This allows them to be used from reflection.

Don't use mangled propery name during compilation

a051fc1

Explicit forbid accessors on static props

19bb004

This can be supported, but would require additional implementation effort.

Improve opcache compatibility

aa1abc9

Fix JIT (by bailing out)

e3145b4

Fix clang build (va promotion UB)

11aafaa

Try to fix another maybe-uninitialized warning

d2c177d

Optimize reads from auto accessors

36dff63

Property handle by-ref distinction for auto accessors

ad8916d

This is ugly, but necessary.

Handle overloaded object semantics more precisely

4c46fb1

This matches the behavior we'd get with an implicit implementation, but also means that these would be less efficient :/

Optimize isset() with proper accessor

a94d2cb

Apparently inlining zend_is_true() adversely affects isset() here.

Optimize assignment to trivial accessor

f982549

Add dump test

afcd945

Fix trivial read/write cache

a467ae1

This doesn't quite hold if we switch to a shadowed parent property. At the very least the error message may differ in the write case.

Prohibit redundant final modifier

f518ce0

To be consistent with the treatment of abstract.

Prohibit final private property

f4f07fd

This was previously only checked for accessors.

Fix rebase fallout

a8b8d08

nikic added 9 commits May 4, 2021 14:50

Forbid isolated &get even if not implicit

373f453

Extend dump test

4514702

Allow final property with private accessor

976f2db

Expose isFinal() and isAbstract() in reflection

04dad4a

Use proper return types for newly added methods

1857bb9

Test one more private + final combination

5069793

Add ref source for by-ref auto-prop

2a2880b

Fix constant updating with virtual properties

00323ab

Fix unset of uninit flag if initialization fails

91ca074

nikic force-pushed the accessors-2 branch from 38906f9 to 91ca074 Compare May 4, 2021 12:58

TysonAndre reviewed May 5, 2021

View reviewed changes

nikic added 4 commits May 5, 2021 11:01

Forbid final in interface

eb70521

The abstract + final combination is usually prohibited when the flags are parsed in the first place, but here the abstract is implicit, so we need to check it explicitly.

Fix class name in final property error

e97adc6

List supported accessors in error message

a151ec5

Don't allow unserializing to virtual property

43d2b5c

However, it is allowed if there is a parent for which the property was non-virtual.

bwoebi reviewed May 6, 2021

View reviewed changes

mvorisek reviewed Jun 20, 2021

View reviewed changes

bwoebi closed this May 2, 2024

Basic accessors implementation (WIP) #6873

Basic accessors implementation (WIP) #6873

Uh oh!

Conversation

nikic commented Apr 16, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwoebi May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skie commented Jul 3, 2021

Uh oh!

NeilMasters commented Jun 6, 2022

Uh oh!

UweKeim commented Dec 22, 2022

Uh oh!

mx03 commented Dec 22, 2022

Uh oh!

rodrigoslayertech commented Aug 5, 2023

Uh oh!

3UR commented Aug 21, 2023

Uh oh!

rodrigoslayertech commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AwokeKnowing commented Dec 8, 2023

Uh oh!

6XGate commented May 2, 2024

Uh oh!

rodrigoslayertech commented May 2, 2024

Uh oh!

bwoebi commented May 2, 2024

Uh oh!

Uh oh!

bwoebi May 6, 2021 •

edited

Loading

rodrigoslayertech commented Aug 22, 2023 •

edited

Loading