Skip to content
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

Merged
merged 1 commit into from
Jul 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions UPGRADING
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ PHP 8.4 UPGRADE NOTES
RFC: https://wiki.php.net/rfc/new_without_parentheses
. Added the #[\Deprecated] attribute.
RFC: https://wiki.php.net/rfc/deprecated_attribute
. Implemented property hooks.
RFC: https://wiki.php.net/rfc/property-hooks

- Curl:
. curl_version() returns an additional feature_list value, which is an
Expand Down
3 changes: 3 additions & 0 deletions Zend/Optimizer/compact_literals.c
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,9 @@ void zend_optimizer_compact_literals(zend_op_array *op_array, zend_optimizer_ctx
LITERAL_INFO(opline->op2.constant, 2);
}
break;
case ZEND_INIT_PARENT_PROPERTY_HOOK_CALL:
LITERAL_INFO(opline->op1.constant, 1);
break;
case ZEND_CATCH:
LITERAL_INFO(opline->op1.constant, 2);
break;
Expand Down
7 changes: 6 additions & 1 deletion Zend/Optimizer/optimize_func_calls.c
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ static void zend_delete_call_instructions(zend_op_array *op_array, zend_op *opli
case ZEND_INIT_STATIC_METHOD_CALL:
case ZEND_INIT_METHOD_CALL:
case ZEND_INIT_FCALL:
case ZEND_INIT_PARENT_PROPERTY_HOOK_CALL:
if (call == 0) {
MAKE_NOP(opline);
return;
Expand Down Expand Up @@ -169,12 +170,15 @@ void zend_optimize_func_calls(zend_op_array *op_array, zend_optimizer_ctx *ctx)
case ZEND_INIT_METHOD_CALL:
case ZEND_INIT_FCALL:
case ZEND_NEW:
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. */
Comment on lines +173 to 175
Copy link
Member

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

Copy link
Member Author

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.

call_stack[call].func = zend_optimizer_get_called_func(
ctx->script, op_array, opline, &call_stack[call].is_prototype);
call_stack[call].try_inline =
!call_stack[call].is_prototype && opline->opcode != ZEND_NEW;
!call_stack[call].is_prototype
&& opline->opcode != ZEND_NEW
&& opline->opcode != ZEND_INIT_PARENT_PROPERTY_HOOK_CALL;
ZEND_FALLTHROUGH;
case ZEND_INIT_DYNAMIC_CALL:
case ZEND_INIT_USER_CALL:
Expand Down Expand Up @@ -212,6 +216,7 @@ void zend_optimize_func_calls(zend_op_array *op_array, zend_optimizer_ctx *ctx)
}
} else if (fcall->opcode == ZEND_INIT_STATIC_METHOD_CALL
|| fcall->opcode == ZEND_INIT_METHOD_CALL
|| fcall->opcode == ZEND_INIT_PARENT_PROPERTY_HOOK_CALL
|| fcall->opcode == ZEND_NEW) {
/* We don't have specialized opcodes for this, do nothing */
} else {
Expand Down
1 change: 1 addition & 0 deletions Zend/Optimizer/zend_call_graph.c
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ ZEND_API void zend_analyze_calls(zend_arena **arena, zend_script *script, uint32
case ZEND_INIT_FCALL:
case ZEND_INIT_METHOD_CALL:
case ZEND_INIT_STATIC_METHOD_CALL:
case ZEND_INIT_PARENT_PROPERTY_HOOK_CALL:
call_stack[call] = call_info;
func = zend_optimizer_get_called_func(
script, op_array, opline, &is_prototype);
Expand Down
35 changes: 35 additions & 0 deletions Zend/Optimizer/zend_optimizer.c
Original file line number Diff line number Diff line change
Expand Up @@ -974,6 +974,28 @@ zend_function *zend_optimizer_get_called_func(
}
}
break;
case ZEND_INIT_PARENT_PROPERTY_HOOK_CALL: {
zend_class_entry *scope = op_array->scope;
ZEND_ASSERT(scope != NULL);
if ((scope->ce_flags & ZEND_ACC_LINKED) && scope->parent) {
zend_class_entry *parent_scope = scope->parent;
zend_string *prop_name = Z_STR_P(CRT_CONSTANT(opline->op1));
zend_property_hook_kind hook_kind = opline->op2.num;
zend_property_info *prop_info = zend_get_property_info(parent_scope, prop_name, /* silent */ true);

if (prop_info
&& prop_info != ZEND_WRONG_PROPERTY_INFO
&& !(prop_info->flags & ZEND_ACC_PRIVATE)
&& prop_info->hooks) {
zend_function *fbc = prop_info->hooks[hook_kind];
if (fbc) {
*is_prototype = false;
return fbc;
}
}
}
break;
}
case ZEND_NEW:
{
zend_class_entry *ce = zend_optimizer_get_class_entry_from_op1(
Expand Down Expand Up @@ -1531,6 +1553,19 @@ void zend_foreach_op_array(zend_script *script, zend_op_array_func_t func, void
zend_foreach_op_array_helper(op_array, func, context);
}
} ZEND_HASH_FOREACH_END();

zend_property_info *property;
ZEND_HASH_MAP_FOREACH_PTR(&ce->properties_info, property) {
zend_function **hooks = property->hooks;
if (property->ce == ce && property->hooks) {
for (uint32_t i = 0; i < ZEND_PROPERTY_HOOK_COUNT; i++) {
zend_function *hook = hooks[i];
if (hook && hook->common.scope == ce) {
zend_foreach_op_array_helper(&hooks[i]->op_array, func, context);
}
}
}
} ZEND_HASH_FOREACH_END();
} ZEND_HASH_FOREACH_END();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@ const FOO_COMPILE_ERROR = "BAR"{0};
var_dump(FOO_COMPILE_ERROR);
?>
--EXPECTF--
Fatal error: Array and string offset access syntax with curly braces is no longer supported in %s on line 2
Parse error: syntax error, unexpected token "{", expecting "," or ";" in %s on line %d
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@ $foo = 'BAR';
var_dump($foo{0});
?>
--EXPECTF--
Fatal error: Array and string offset access syntax with curly braces is no longer supported in %s on line 3
Parse error: syntax error, unexpected token "{", expecting ")" in %s on line %d
2 changes: 1 addition & 1 deletion Zend/tests/alternative_offset_syntax_in_encaps_string.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ Alternative offset syntax should emit only E_COMPILE_ERROR in string interpolati
--FILE--
<?php "{$g{'h'}}"; ?>
--EXPECTF--
Fatal error: Array and string offset access syntax with curly braces is no longer supported in %s on line 1
Parse error: syntax error, unexpected token "{", expecting "->" or "?->" or "[" in %s on line %d
2 changes: 1 addition & 1 deletion Zend/tests/errmsg_037.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ class test {
echo "Done\n";
?>
--EXPECTF--
Fatal error: Cannot use the abstract modifier on a property in %s on line %d
Fatal error: Only hooked properties may be declared abstract in %s on line %d
13 changes: 0 additions & 13 deletions Zend/tests/errmsg_038.phpt

This file was deleted.

2 changes: 1 addition & 1 deletion Zend/tests/new_without_parentheses/unset_new.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,4 @@ unset(new ArrayObject());

?>
--EXPECTF--
Parse error: syntax error, unexpected token ")", expecting "->" or "?->" or "{" or "[" in %s on line %d
Parse error: syntax error, unexpected token ")", expecting "->" or "?->" or "[" in %s on line %d
22 changes: 22 additions & 0 deletions Zend/tests/property_hooks/abstract_hook.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
--TEST--
Abstract hooks compile successfully
--FILE--
<?php

abstract class A {
public abstract $prop {
get;
set {}
}
}

class B extends A {
public $prop {
get {}
}
}

?>
===DONE===
--EXPECT--
===DONE===
15 changes: 15 additions & 0 deletions Zend/tests/property_hooks/abstract_hook_in_non_abstract_class.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
--TEST--
Abstract hooks in non-abstract class gives an error
--FILE--
<?php

class Test {
public abstract $prop {
get;
set {}
}
}

?>
--EXPECTF--
Fatal error: Class Test contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Test::$prop::get) in %s on line %d
17 changes: 17 additions & 0 deletions Zend/tests/property_hooks/abstract_hook_not_implemented.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
--TEST--
Abstract hooks that are not implemented throw an error
--FILE--
<?php

abstract class A {
public abstract $prop {
get;
set {}
}
}

class B extends A {}

?>
--EXPECTF--
Fatal error: Class B contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (A::$prop::get) in %s on line %d
17 changes: 17 additions & 0 deletions Zend/tests/property_hooks/abstract_prop_hooks.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
--TEST--
Explicit hooked property satisfies abstract property
--FILE--
<?php

abstract class A {
abstract public $prop { get; set; }
}

class B extends A {
public $prop { get {} set {} }
}

?>
===DONE===
--EXPECT--
===DONE===
14 changes: 14 additions & 0 deletions Zend/tests/property_hooks/abstract_prop_not_implemented.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
--TEST--
Abstract property not implemented throws an error
--FILE--
<?php

class A {
abstract public $prop { get; set; }
}

class B extends A {}

?>
--EXPECTF--
Fatal error: Class A contains 2 abstract methods and must therefore be declared abstract or implement the remaining methods (A::$prop::get, A::$prop::set) in %s on line %d
17 changes: 17 additions & 0 deletions Zend/tests/property_hooks/abstract_prop_plain.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
--TEST--
Plain property satisfies abstract property
--FILE--
<?php

abstract class A {
abstract public $prop { get; set; }
}

class B extends A {
public $prop;
}

?>
===DONE===
--EXPECT--
===DONE===
12 changes: 12 additions & 0 deletions Zend/tests/property_hooks/abstract_prop_without_hooks.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
--TEST--
Abstract property without hook is illegal
--FILE--
<?php

class C {
abstract public $prop;
}

?>
--EXPECTF--
Fatal error: Only hooked properties may be declared abstract in %s on line %d
49 changes: 49 additions & 0 deletions Zend/tests/property_hooks/array_access.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
--TEST--
Array offset on ArrayAccess object in virtual property is allowed
--FILE--
<?php

class Collection implements ArrayAccess {
public function offsetExists(mixed $offset): bool {
echo __METHOD__ . "\n";
return true;
}

public function offsetGet(mixed $offset): mixed {
echo __METHOD__ . "\n";
return true;
}

public function offsetSet(mixed $offset, mixed $value): void {
echo __METHOD__ . "\n";
}

public function offsetUnset(mixed $offset): void {
echo __METHOD__ . "\n";
}
}

class C {
public function __construct(
public Collection $collection = new Collection(),
) {}
public $prop {
get => $this->collection;
}
}

$c = new C();
var_dump($c->prop['foo']);
var_dump($c->prop[] = 'foo');
var_dump(isset($c->prop['foo']));
unset($c->prop['foo']);

?>
--EXPECT--
Collection::offsetGet
bool(true)
Collection::offsetSet
string(3) "foo"
Collection::offsetExists
bool(true)
Collection::offsetUnset
45 changes: 45 additions & 0 deletions Zend/tests/property_hooks/ast_printing.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
--TEST--
Hook AST printing
--FILE--
<?php

try {
assert(false && new class {
public $prop1 { get; set; }
public $prop2 {
get {
return parent::$prop1::get();
}
final set {
echo 'Foo';
$this->prop1 = 42;
}
}
public $prop3 = 1 {
get => 42;
}
});
} catch (Error $e) {
echo $e->getMessage(), "\n";
}

?>
--EXPECT--
assert(false && new class {
public $prop1 {
get;
set;
}
public $prop2 {
get {
return parent::$prop1::get();
}
final set {
echo 'Foo';
$this->prop1 = 42;
}
}
public $prop3 = 1 {
get => 42;
}
})
Loading
Loading