-
Notifications
You must be signed in to change notification settings - Fork 7.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RFC] Property hooks #13455
[RFC] Property hooks #13455
Conversation
case ZEND_INIT_PARENT_PROPERTY_HOOK_CALL: | ||
/* The argument passing optimizations are valid for prototypes as well, | ||
* as inheritance cannot change between ref <-> non-ref arguments. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since hooks never receive by ref, we may be able to optimize ops to their non-ref variants like we do for known functions. E.g. for this code:
public mixed $x {
set => parent::$x::set($value['x']);
}
we generate this:
0000 CV0($value) = RECV 1
0001 INIT_PARENT_PROPERTY_HOOK_CALL 1 string("x") 1
0002 CHECK_FUNC_ARG 1
0003 V1 = FETCH_DIM_FUNC_ARG CV0($value) string("x")
0004 SEND_FUNC_ARG V1 1
0005 DO_FCALL
0006 RETURN null
and it should optimize to something like this once the optimizer knows that hooks do not receive by ref:
0000 CV0($value) = RECV 1
0001 INIT_PARENT_PROPERTY_HOOK_CALL 1 string("x") 1
0003 T1 = FETCH_DIM_R CV0($value) string("x")
0004 SEND_VAL T1 1
0005 DO_FCALL
0006 RETURN null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. Parent hooks are not very optimized at this moment. I think they will be rare in practice. I'll have a look nonetheless. We might be switching to a different syntax though (parent::$prop
and parent::$prop = $value
), if possible.
Potential scoping issue when hooks are defined in traits. Not sure if this is the expected behavior or not. |
@TRowbotham Thanks for letting me know. I added this to my todo list. |
Here is another one: https://3v4l.org/7Al8b/rfc#vrfc.property-hooks When hooks are defined in a class that implements the |
@TRowbotham Good catch! I'll have a look at that one as well. |
Ran into a behavior difference between (property hooks) https://3v4l.org/6W26c/rfc#vrfc.property-hooks |
@TRowbotham This is intended behavior. Inside functions called from a hook, An additional difference is that, for the I hope that clears it up. |
@iluuu1994 Thanks for the explanation. That makes it clear. |
Congratulations!!! RFC passed! PHP 8.4 will be AWESOME!!! |
@rodrigoslayertech The voting is open until 29 April. Let's not count our chickens before they hatch. ;-) |
Awsome, this sounds like the dynamic properties we have in laravel models, voted. |
Hi, While playing with the feature on 3v4l.org, I found the following bug. Although I’ve not tested the PR in its current state, I suspect that the bug might be still here, because I didn’t find test against it. Test case (https://3v4l.org/fuj6p/rfc#vrfcproperty-hooks): abstract class A {
abstract public $x { get; }
}
class C extends A {
private $_x;
public $x {
get => $this->_x;
}
}
// expected: bool(true); actual: bool(false)
var_dump((new ReflectionProperty(C::class, 'x'))->isVirtual());
// expected: Error: Property C::$x is read-only; actual: no error
$c = new C;
$c->x = 3; The test case works as expected when I replace the abstract class with an interface (https://3v4l.org/ed1TG/rfc#vrfc.property-hooks). (Background: I expect to use that pattern in order to approximate asymmetric visibility.) |
699ff29
to
01ca5f7
Compare
@claudepache The 3v4l build is indeed outdated. In any case, I'll make sure to have a look at your example. Thanks! |
01ca5f7
to
30696aa
Compare
While continuing to play with this, I came across a case where using the get hook as a generator results in a segfault. <?php
class A {
public Generator $prop {
get {
for ($i = 0; $i < 3; ++$i) {
yield $i;
}
}
}
}
$a = new A();
var_dump($a->prop->valid()); |
@TRowbotham Thanks! This wasn't really specified in the RFC. I fixed the issue, but also raised it to Larry to discuss over whether we even want to allow |
67f6e6d
to
77615a5
Compare
The implementation now solves all known issues, and CI (including nightly) passes. So I will be merging it in the coming days. |
} | ||
|
||
/* Get-only virtual property can never be written to. */ | ||
if ((prop->flags & ZEND_ACC_VIRTUAL) && !prop->hooks[ZEND_PROPERTY_HOOK_SET]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it the intention that ZEND_ACC_VIRTUAL implies the existence of hooks? If we allow to specify @virtual
in the stub file in a follow-up to avoid allocating backing storage, then this check can potentially dereference a NULL pointer because prop->hooks isn't necessarily filled in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know all the checks by heart but at least here, ZEND_ACC_VIRTUAL
assumes hooks are present. I presume your intention is to use @virtual
for existing virtual properties implemented using the existing object property handlers? Tweaking the logic where necessary to check for the existence of hooks would be ok with me, if these cannot immediately be migrated to hooks (in part because internal classes aren't supported yet). But I'd prefer doing that after this has merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume your intention is to use @virtual for existing virtual properties implemented using the existing object property handlers?
Indeed.
Tweaking the logic where necessary to check for the existence of hooks would be ok with me, if these cannot immediately be migrated to hooks (in part because internal classes aren't supported yet)
I don't know how internal hooks would look like so can't really comment on this (yet).
But I'd prefer doing that after this has merged.
Of course
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how internal hooks would look like so can't really comment on this (yet).
My intention is to allow marking the property with { get; set; }
(or possibly just annotations until we have PHP-Parser support), which will look for and link the corresponding function into zend_property_info.hooks
. So, hopefully it will not be too complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, those would be zend_function's right? That's a bit of a pitty because it would require completely reworking the virtual properties in DOM/XMLReader with the additional penalty of having to deal with a stackframe, unless they can be frameless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Good question. Avoiding the stack frame was not planned for now, but I'll see what's possible once I get to it.
e6fb2ba
to
a793809
Compare
RFC: https://wiki.php.net/rfc/property-hooks Co-authored-by: Nikita Popov <nikita.ppv@gmail.com> Closes phpGH-13455
a793809
to
f11dfc3
Compare
Co-authored-by: Nikita Popov <nikita.ppv@gmail.com> RFC: https://wiki.php.net/rfc/property-hooks Closes phpGH-13455
ee4d7e2
to
9f295d9
Compare
RFC: https://wiki.php.net/rfc/property-hooks Co-authored-by: Nikita Popov <nikita.ppv@gmail.com>
9f295d9
to
6059538
Compare
@iluuu1994 I know it's already merged, but I have question: wouldn't it be a good idea to introduce |
@Wirone Unfortunately, we are lacking the context in the lexer ourselves to do that. |
} | ||
} | ||
|
||
static bool zend_compile_parent_property_hook_call(znode *result, zend_ast *ast, uint32_t type) /* {{{ */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know if anyone cares, but this comment (/* {{{ */
) seems to be mismatched (no closing braces).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I fixed it.
https://wiki.php.net/rfc/property-hooks