[RFC] Add `#[\DelayedTargetValidation]` attribute #18817

DanielEScherzer · 2025-06-09T20:25:30Z

https://wiki.php.net/rfc/delayedtargetvalidation_attribute

DanielEScherzer · 2025-08-01T15:44:53Z

CC @TimWolla @edorian for the NoDiscard-related changes, the "don't apply on property hooks" error was moved around so that it could be delayed

TimWolla · 2025-08-01T21:14:19Z

Currently on vacation, so can't review, but I trust that you are capable of correctly moving error messages 😄

TimWolla

Some first thoughts. I don't currently have the mental capacity to perform an in-depth review (diff is larger than anticipated) and didn't look at the entire PR.

Zend/zend_attributes.stub.php

Zend/zend_attributes.c

UPGRADING

iluuu1994

After sleeping on this, my preferred approach would be:

Stop using zend_error_noreturn() in validator in favor of zend_throw_error() for target validation, instruct from UPGRADING.INTERNALS to do the same for extensions.
Use EG(exception) from zend_compile_attributes() to understand whether the validator succeeded.
Without #[DelayedTargetValidation], promote to a fatal error with zend_exception_uncaught_error().
With #[DelayedTargetValidation], record whether the validator has succeeded and store it in zend_attribute. EG(exception) would have to be removed/freed.
When it failed, repeat the validation on ReflectionAttribute::newInstance(). Skipping for successful validators prevents writing to locked shm. Validators should be idempotent, i.e. we should be able to rely on the fact that calling it again will also throw.

The benefit is that we don't need to handle this for every validator, and we don't need the ZEND_ATTRIBUTE_NO_TARGET_VALIDATION/ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION flags. Then, this feature should work mostly automatically.

WDYT?

ext/reflection/php_reflection.c

iluuu1994 · 2025-08-13T08:05:53Z

Zend/zend_compile.c

@@ -8422,6 +8446,10 @@ static zend_op_array *zend_compile_func_decl_ex(

 	CG(active_op_array) = op_array;

+	zend_oparray_context_begin(&orig_oparray_context, op_array);
+	CG(context).active_property_info_name = property_info_name;
+	CG(context).active_property_hook_kind = hook_kind;


Same with this.

DanielEScherzer · 2025-08-13T14:30:16Z

Stop using zend_error_noreturn() in validator in favor of zend_throw_error() for target validation, instruct from UPGRADING.INTERNALS to do the same for extensions.

zend_throw_error() will just use zend_error_noreturn() if I'm reading correctly,

php-src/Zend/zend.c

Lines 1831 to 1836 in 222f751

    
           //TODO: we can't convert compile-time errors to exceptions yet??? 
        
           if (EG(current_execute_data) && !CG(in_compilation)) { 
        
           	zend_throw_exception(exception_ce, message, 0); 
        
           } else { 
        
           	zend_error_noreturn(E_ERROR, "%s", message); 
        
           }

when validators are run, CG(in_compilation) is true, see the validate_nodiscard() logic

Skipping for successful validators prevents writing to locked shm.

No validators should be writing anything at runtime, e.g. for allow dynamic properties the validator just ZEND_ASSERTs that it passed the first time

iluuu1994 · 2025-08-13T16:02:42Z

zend_throw_error() will just use zend_error_noreturn() if I'm reading correctly,

RIght, at least when EG(current_execute_data) is empty, which can occur for the main script. You would have to install a fake frame.

No validators should be writing anything at runtime, e.g. for allow dynamic properties the validator just ZEND_ASSERTs that it passed the first time

Yes, but every validator must abort to avoid this, which is a potential foot gun. What I'm suggesting is a way for #[DelayTargetValidation] to mostly just work.

DanielEScherzer · 2025-08-13T17:07:15Z

zend_throw_error() will just use zend_error_noreturn() if I'm reading correctly,

RIght, at least when EG(current_execute_data) is empty, which can occur for the main script. You would have to install a fake frame.

Even if it is not empty, the && means that noreturn would be used? Or am I missing something?

iluuu1994 · 2025-08-14T11:18:16Z

Yes, you can change this value as well. But there are alternatives. E.g. validator could return a zend_string containing the error message or null on success, which could be attached to zend_attribute and then re-thrown by reflection. Then you don't need to trigger validator twice at all.

DanielEScherzer · 2025-08-14T18:25:30Z

I switched it so that validators return error messages instead of emitting them, which allowed removing the ZEND_ATTRIBUTE_NO_TARGET_VALIDATION flag. For some reason I kept getting segfaults if I tried to add a field to the zend_attribute struct to hold the error message, even if I didn't actually touch the field, I'll try to debug

iluuu1994 · 2025-08-14T18:43:19Z

Valgrind or sanitizers might help. Adding -m to run-tests.php is a simple way to run Valgrind on some tests without having to recompile with sanitizers.

DanielEScherzer · 2025-08-15T18:34:56Z

Valgrind or sanitizers might help. Adding -m to run-tests.php is a simple way to run Valgrind on some tests without having to recompile with sanitizers.

Pretty sure the issue was that I was putting the new field after the zend_attribute_arg args[1]; and so breaking the struct hack

iluuu1994

Thanks for the changes thus far. It seems simpler to me, hopefully you agree.

ext/reflection/php_reflection.c

Zend/zend_compile.c

Zend/zend_attributes.c

TimWolla

C stuff LGTM now, didn't look into the (very verbose) tests.

ext/reflection/php_reflection.c

iluuu1994

Thanks for the changes!

Zend/zend_compile.c

ext/reflection/php_reflection.c

See php#19061

DanielEScherzer · 2025-08-17T21:38:19Z

Addressed the notes about comments, and rebased
I'll plan to merge this tomorrow, I want to take another look myself with fresh eyes
In the meantime I'll go write up a blog post explaining the feature that can be used to help with the process of updating php/doc-en later

TimWolla · 2025-08-20T08:27:28Z

ext/reflection/php_reflection.c

+		);
+		ZEND_ASSERT(delayed_target_validation != NULL);
+#endif
+		zend_throw_exception(zend_ce_error, ZSTR_VAL(attr->data->validation_error), 0);


Seeing this, I wonder if we should export zend_throw_exception_zstr.

Don't think that is necessary at the moment, this worked fine

github-actions bot added Category: Engine Extension: reflection ABI break labels Jun 9, 2025

DanielEScherzer added the RFC label Jun 20, 2025

DanielEScherzer force-pushed the delayed-target-validation branch from 037775b to 26ad0f2 Compare July 6, 2025 17:30

DanielEScherzer force-pushed the delayed-target-validation branch from 1978ce1 to 82f556a Compare August 1, 2025 14:38

DanielEScherzer changed the title ~~Add #[\DelayedTargetValidation] attribute~~ [RFC] Add #[\DelayedTargetValidation] attribute Aug 1, 2025

DanielEScherzer marked this pull request as ready for review August 1, 2025 15:43

DanielEScherzer requested a review from kocsismate as a code owner August 1, 2025 15:43

DanielEScherzer force-pushed the delayed-target-validation branch from 82f556a to abbef91 Compare August 11, 2025 16:39

DanielEScherzer requested a review from iluuu1994 August 11, 2025 16:40

DanielEScherzer force-pushed the delayed-target-validation branch from abbef91 to 6b11fd2 Compare August 12, 2025 13:35

DanielEScherzer requested a review from TimWolla August 12, 2025 13:36

TimWolla reviewed Aug 12, 2025

View reviewed changes

Zend/zend_attributes.stub.php Outdated Show resolved Hide resolved

Zend/zend_attributes.c Outdated Show resolved Hide resolved

UPGRADING Outdated Show resolved Hide resolved

iluuu1994 mentioned this pull request Aug 12, 2025

Use attribute validator for assigning flags #19465

Open

iluuu1994 reviewed Aug 13, 2025

View reviewed changes

github-actions bot added the Extension: zend_test label Aug 14, 2025

DanielEScherzer requested a review from dstogov as a code owner August 15, 2025 18:33

github-actions bot added the Extension: opcache label Aug 15, 2025

DanielEScherzer force-pushed the delayed-target-validation branch from 8959bec to 254926f Compare August 15, 2025 20:45

iluuu1994 reviewed Aug 16, 2025

View reviewed changes

ext/reflection/php_reflection.c Outdated Show resolved Hide resolved

ext/reflection/php_reflection.c Show resolved Hide resolved

Zend/zend_compile.c Show resolved Hide resolved

Zend/zend_attributes.c Outdated Show resolved Hide resolved

TimWolla reviewed Aug 17, 2025

View reviewed changes

ext/reflection/php_reflection.c Show resolved Hide resolved

iluuu1994 approved these changes Aug 17, 2025

View reviewed changes

Zend/zend_compile.c Outdated Show resolved Hide resolved

ext/reflection/php_reflection.c Outdated Show resolved Hide resolved

ext/reflection/php_reflection.c Show resolved Hide resolved

DanielEScherzer added 23 commits August 17, 2025 14:33

Add #[\DelayedTargetValidation] attribute

a687a7d

Avoid uninitialized memory

5bc6d09

Run validators for delayed validators

7093f3f

Fix for reflection

fa95746

Test application of all internal attributes, fix 2 bugs

04f280b

Missing comment

285af54

Tests for property hooks

e0e7b71

Delay validation of #[\NoDiscard] on property hooks

2935829

Update tests; tweak comments

4e867ee

#[\Attribute]

4bd054c

UPGRADING

175fed1

without

d983c47

Update for #[\Override] on properties

f31f883

See php#19061

Validators return error messages

fc0eb67

%s

af89164

Only run validators once, remember the error

5513a54

-on_property_hook

9044549

Cleanup and tweaks

317f2b8

Happy path at the bottom

877fcfe

Simplify reflection for non-debug builds

b2b24fd

zend_persist_attributes

72dc9df

Suggestions from TimWolla

74b10f9

comment cleanup

3dac774

DanielEScherzer force-pushed the delayed-target-validation branch from 9152360 to 3dac774 Compare August 17, 2025 21:37

DanielEScherzer merged commit 63acc4b into php:master Aug 20, 2025
9 checks passed

DanielEScherzer deleted the delayed-target-validation branch August 20, 2025 07:41

TimWolla reviewed Aug 20, 2025

View reviewed changes

[RFC] Add #[\DelayedTargetValidation] attribute #18817

[RFC] Add #[\DelayedTargetValidation] attribute #18817

Conversation

DanielEScherzer commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanielEScherzer commented Aug 1, 2025

Uh oh!

TimWolla commented Aug 1, 2025

Uh oh!

TimWolla left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

iluuu1994 left a comment • 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!

Uh oh!

iluuu1994 Aug 13, 2025

Choose a reason for hiding this comment

Uh oh!

DanielEScherzer commented Aug 13, 2025

Uh oh!

iluuu1994 commented Aug 13, 2025

Uh oh!

DanielEScherzer commented Aug 13, 2025

Uh oh!

iluuu1994 commented Aug 14, 2025

Uh oh!

DanielEScherzer commented Aug 14, 2025

Uh oh!

iluuu1994 commented Aug 14, 2025

Uh oh!

DanielEScherzer commented Aug 15, 2025

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TimWolla left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iluuu1994 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanielEScherzer commented Aug 17, 2025

Uh oh!

Uh oh!

TimWolla Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

DanielEScherzer Aug 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[RFC] Add `#[\DelayedTargetValidation]` attribute #18817

[RFC] Add `#[\DelayedTargetValidation]` attribute #18817

DanielEScherzer commented Jun 9, 2025 •

edited

Loading

TimWolla left a comment •

edited

Loading

iluuu1994 left a comment •

edited

Loading