[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

See php#19061

iluuu1994

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

iluuu1994 · 2025-08-16T00:02:03Z

ext/reflection/php_reflection.c


+	/* Report the delayed validator error for internal attributes */
+	if (delayed_target_validation && ce->type == ZEND_INTERNAL_CLASS) {


Don't think we need to load this at all. When validation_error is set, we can just throw the error.

iluuu1994 · 2025-08-16T00:06:22Z

ext/reflection/php_reflection.c

+	 * - the attribute is a user attribute, and neither target nor repetition
+	 *   have been validated.
+	 */
+	uint32_t flags = zend_attribute_attribute_get_flags(marker, ce);


Instead of these changes, could we not just attach target & repetition validation to validation_error in zend_compile.c? Then errors would all be handled the same way for internal attributes.

iluuu1994 · 2025-08-16T00:08:47Z

Zend/zend_compile.c

+				"delayedtargetvalidation",
+				strlen("delayedtargetvalidation"),


Nit:

Suggested change

"delayedtargetvalidation",

strlen("delayedtargetvalidation"),

ZEND_STRL("delayedtargetvalidation"),

iluuu1994 · 2025-08-16T00:15:43Z

Zend/zend_attributes.c

+{
+	ZEND_ASSERT(CG(in_compilation));
+	const zend_string *prop_info_name = CG(context).active_property_info_name;
+	if (prop_info_name == NULL) {


Nit: No hard rules on this, but most similar code puts the happy path at the bottom. This way, more conditions can be added without nesting.

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 added 2 commits August 15, 2025 13:44

Add #[\DelayedTargetValidation] attribute

43d65c1

Avoid uninitialized memory

e216d1b

DanielEScherzer added 16 commits August 15, 2025 13:44

Run validators for delayed validators

fcc5447

Fix for reflection

a27c8a0

Test application of all internal attributes, fix 2 bugs

5e8d1cd

Missing comment

34e041a

Tests for property hooks

225d899

Delay validation of #[\NoDiscard] on property hooks

700af01

Update tests; tweak comments

9b42ed8

#[\Attribute]

f1f5bea

UPGRADING

3026c9e

without

c5bfb65

Update for #[\Override] on properties

e3f6c2f

See php#19061

Validators return error messages

d409119

%s

6a1fd78

Only run validators once, remember the error

5b1bbee

-on_property_hook

48c21ed

Cleanup and tweaks

254926f

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


		/* Report the delayed validator error for internal attributes */
		if (delayed_target_validation && ce->type == ZEND_INTERNAL_CLASS) {

		"delayedtargetvalidation",
		strlen("delayedtargetvalidation"),

	"delayedtargetvalidation",
	strlen("delayedtargetvalidation"),
	ZEND_STRL("delayedtargetvalidation"),

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

Are you sure you want to change the base?

[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!

iluuu1994 Aug 16, 2025

Choose a reason for hiding this comment

Uh oh!

iluuu1994 Aug 16, 2025

Choose a reason for hiding this comment

Uh oh!

iluuu1994 Aug 16, 2025

Choose a reason for hiding this comment

Uh oh!

iluuu1994 Aug 16, 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