Delay validation of #[\NoDiscard] on property hooks

Author: DanielEScherzer

Zend/tests/attributes/delayed_target_validation/errors_from_validator.phpt

@@ -26,10 +26,10 @@ class DemoClass {
 
 	public string $hooked {
 		#[DelayedTargetValidation]
-		// #[NoDiscard] // Does nothing here
+		#[NoDiscard] // Does nothing here
 		get => $this->hooked;
 		#[DelayedTargetValidation]
-		// #[NoDiscard] // Does nothing here
+		#[NoDiscard] // Does nothing here
 		set => $value;
 	}
 }
@@ -39,8 +39,8 @@ $cases = [
 	new ReflectionClass('DemoInterface'),
 	new ReflectionClass('DemoReadonly'),
 	new ReflectionClass('DemoEnum'),
-	// new ReflectionProperty('DemoClass', 'hooked')->getHook(PropertyHookType::Get),
-	// new ReflectionProperty('DemoClass', 'hooked')->getHook(PropertyHookType::Set),
+	new ReflectionProperty('DemoClass', 'hooked')->getHook(PropertyHookType::Get),
+	new ReflectionProperty('DemoClass', 'hooked')->getHook(PropertyHookType::Set),
 ];
 foreach ($cases as $r) {
 	echo str_repeat("*", 20) . "\n";
@@ -195,3 +195,48 @@ array(2) {
   }
 }
 Error: Cannot apply #[AllowDynamicProperties] to enum DemoEnum
+********************
+Method [ <user> public method $hooked::get ] {
+  @@ %s %d - %d
+
+  - Parameters [0] {
+  }
+  - Return [ string ]
+}
+
+array(2) {
+  [0]=>
+  object(ReflectionAttribute)#%d (1) {
+    ["name"]=>
+    string(23) "DelayedTargetValidation"
+  }
+  [1]=>
+  object(ReflectionAttribute)#%d (1) {
+    ["name"]=>
+    string(9) "NoDiscard"
+  }
+}
+Error: #[\NoDiscard] is not supported for property hooks
+********************
+Method [ <user> public method $hooked::set ] {
+  @@ %s %d - %d
+
+  - Parameters [1] {
+    Parameter #0 [ <required> string $value ]
+  }
+  - Return [ void ]
+}
+
+array(2) {
+  [0]=>
+  object(ReflectionAttribute)#%d (1) {
+    ["name"]=>
+    string(23) "DelayedTargetValidation"
+  }
+  [1]=>
+  object(ReflectionAttribute)#%d (1) {
+    ["name"]=>
+    string(9) "NoDiscard"
+  }
+}
+Error: #[\NoDiscard] is not supported for property hooks

Zend/tests/attributes/delayed_target_validation/with_NoDiscard.phpt

@@ -12,10 +12,10 @@ class DemoClass {
 
 	public string $hooked {
 		#[DelayedTargetValidation]
-		// #[NoDiscard] // Does nothing here
+		#[NoDiscard] // Does nothing here
 		get => $this->hooked;
 		#[DelayedTargetValidation]
-		// #[NoDiscard] // Does nothing here
+		#[NoDiscard] // Does nothing here
 		set => $value;
 	}
 

Zend/zend_attributes.c

@@ -210,6 +210,31 @@ ZEND_METHOD(Deprecated, __construct)
 	}
 }
 
+static void validate_nodiscard(
+	zend_attribute *attr, uint32_t target, zend_class_entry *scope)
+{
+	/* There isn't an easy way to identify the *method* that the attribute is
+	 * applied to in a manner that works during both compilation (normal
+	 * validation) and runtime (delayed validation). So, handle them separately.
+	 */
+	if (CG(in_compilation)) {
+		ZEND_ASSERT((target & ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION) == 0);
+		zend_op_array *op_array = CG(active_op_array);
+		const zend_string *prop_info_name = CG(context).active_property_info_name;
+		if (prop_info_name == NULL) {
+			op_array->fn_flags |= ZEND_ACC_NODISCARD;
+			return;
+		}
+		// Applied to a hook, either throw or ignore
+		if (target & ZEND_ATTRIBUTE_NO_TARGET_VALIDATION) {
+			return;
+		}
+		zend_error_noreturn(E_COMPILE_ERROR, "#[\\NoDiscard] is not supported for property hooks");
+	}
+	/* At runtime, no way to identify the target method; Reflection will handle
+	 * throwing the error if needed. */
+}
+
 ZEND_METHOD(NoDiscard, __construct)
 {
 	zend_string *message = NULL;
@@ -569,6 +594,7 @@ void zend_register_attribute_ce(void)
 
 	zend_ce_nodiscard = register_class_NoDiscard();
 	attr = zend_mark_internal_attribute(zend_ce_nodiscard);
+	attr->validator = validate_nodiscard;
 
 	zend_ce_delayed_target_validation = register_class_DelayedTargetValidation();
 	attr = zend_mark_internal_attribute(zend_ce_delayed_target_validation);

Zend/zend_compile.c

@@ -7568,15 +7568,17 @@ static void zend_compile_attributes(
 				continue;
 			}
 
-			if (delayed_target_validation == NULL) {
-				if (!(target & (config->flags & ZEND_ATTRIBUTE_TARGET_ALL))) {
+			bool run_validator = true;
+			if (!(target & (config->flags & ZEND_ATTRIBUTE_TARGET_ALL))) {
+				if (delayed_target_validation == NULL) {
 					zend_string *location = zend_get_attribute_target_names(target);
 					zend_string *allowed = zend_get_attribute_target_names(config->flags);
 
 					zend_error_noreturn(E_ERROR, "Attribute \"%s\" cannot target %s (allowed targets: %s)",
 						ZSTR_VAL(attr->name), ZSTR_VAL(location), ZSTR_VAL(allowed)
 					);
 				}
+				run_validator = false;
 			}
 
 			if (!(config->flags & ZEND_ATTRIBUTE_IS_REPEATABLE)) {
@@ -7585,7 +7587,8 @@ static void zend_compile_attributes(
 				}
 			}
 
-			if (config->validator != NULL) {
+			// Validators are not run if the target is already invalid
+			if (run_validator && config->validator != NULL) {
 				config->validator(attr, target | extra_flags, CG(active_class_entry));
 			}
 		} ZEND_HASH_FOREACH_END();
@@ -8427,6 +8430,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;
+
 	if (decl->child[4]) {
 		int target = ZEND_ATTRIBUTE_TARGET_FUNCTION;
 
@@ -8456,15 +8463,7 @@ static zend_op_array *zend_compile_func_decl_ex(
 			op_array->fn_flags |= ZEND_ACC_DEPRECATED;
 		}
 
-		zend_attribute *nodiscard_attribute = zend_get_attribute_str(
-			op_array->attributes,
-			"nodiscard",
-			sizeof("nodiscard")-1
-		);
-
-		if (nodiscard_attribute) {
-			op_array->fn_flags |= ZEND_ACC_NODISCARD;
-		}
+		// ZEND_ACC_NODISCARD is added via an attribute validator
 	}
 
 	/* Do not leak the class scope into free standing functions, even if they are dynamically
@@ -8478,10 +8477,6 @@ static zend_op_array *zend_compile_func_decl_ex(
 		op_array->fn_flags |= ZEND_ACC_TOP_LEVEL;
 	}
 
-	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;
-
 	{
 		/* Push a separator to the loop variable stack */
 		zend_loop_var dummy_var;
@@ -8516,9 +8511,11 @@ static zend_op_array *zend_compile_func_decl_ex(
 	}
 
 	if (op_array->fn_flags & ZEND_ACC_NODISCARD) {
-		if (is_hook) {
-			zend_error_noreturn(E_COMPILE_ERROR, "#[\\NoDiscard] is not supported for property hooks");
-		}
+		/* ZEND_ACC_NODISCARD gets added by the attribute validator, but only
+		 * if the method is not a hook; if it is a hook, then the validator
+		 * should have either thrown an error or done nothing due to delayed
+		 * target validation. */
+		ZEND_ASSERT(!is_hook);
 
 		if (op_array->fn_flags & ZEND_ACC_HAS_RETURN_TYPE) {
 			zend_arg_info *return_info = CG(active_op_array)->arg_info - 1;

ext/reflection/php_reflection.c

@@ -157,6 +157,7 @@ typedef struct _attribute_reference {
 	zend_class_entry *scope;
 	zend_string *filename;
 	uint32_t target;
+	bool on_property_hook;
 } attribute_reference;
 
 typedef enum {
@@ -1254,7 +1255,7 @@ static void _extension_string(smart_str *str, const zend_module_entry *module, c
 
 /* {{{ reflection_attribute_factory */
 static void reflection_attribute_factory(zval *object, HashTable *attributes, zend_attribute *data,
-		zend_class_entry *scope, uint32_t target, zend_string *filename)
+		zend_class_entry *scope, uint32_t target, zend_string *filename, bool on_property_hook)
 {
 	reflection_object *intern;
 	attribute_reference *reference;
@@ -1267,14 +1268,16 @@ static void reflection_attribute_factory(zval *object, HashTable *attributes, ze
 	reference->scope = scope;
 	reference->filename = filename ? zend_string_copy(filename) : NULL;
 	reference->target = target;
+	reference->on_property_hook = on_property_hook;
 	intern->ptr = reference;
 	intern->ref_type = REF_TYPE_ATTRIBUTE;
 	ZVAL_STR_COPY(reflection_prop_name(object), data->name);
 }
 /* }}} */
 
 static int read_attributes(zval *ret, HashTable *attributes, zend_class_entry *scope,
-		uint32_t offset, uint32_t target, zend_string *name, zend_class_entry *base, zend_string *filename) /* {{{ */
+		uint32_t offset, uint32_t target, zend_string *name, zend_class_entry *base, zend_string *filename,
+		bool on_property_hook) /* {{{ */
 {
 	ZEND_ASSERT(attributes != NULL);
 
@@ -1287,7 +1290,7 @@ static int read_attributes(zval *ret, HashTable *attributes, zend_class_entry *s
 
 		ZEND_HASH_PACKED_FOREACH_PTR(attributes, attr) {
 			if (attr->offset == offset && zend_string_equals(attr->lcname, filter)) {
-				reflection_attribute_factory(&tmp, attributes, attr, scope, target, filename);
+				reflection_attribute_factory(&tmp, attributes, attr, scope, target, filename, on_property_hook);
 				add_next_index_zval(ret, &tmp);
 			}
 		} ZEND_HASH_FOREACH_END();
@@ -1319,7 +1322,7 @@ static int read_attributes(zval *ret, HashTable *attributes, zend_class_entry *s
 			}
 		}
 
-		reflection_attribute_factory(&tmp, attributes, attr, scope, target, filename);
+		reflection_attribute_factory(&tmp, attributes, attr, scope, target, filename, on_property_hook);
 		add_next_index_zval(ret, &tmp);
 	} ZEND_HASH_FOREACH_END();
 
@@ -1328,7 +1331,7 @@ static int read_attributes(zval *ret, HashTable *attributes, zend_class_entry *s
 /* }}} */
 
 static void reflect_attributes(INTERNAL_FUNCTION_PARAMETERS, HashTable *attributes,
-		uint32_t offset, zend_class_entry *scope, uint32_t target, zend_string *filename) /* {{{ */
+		uint32_t offset, zend_class_entry *scope, uint32_t target, zend_string *filename, bool on_property_hook) /* {{{ */
 {
 	zend_string *name = NULL;
 	zend_long flags = 0;
@@ -1361,7 +1364,7 @@ static void reflect_attributes(INTERNAL_FUNCTION_PARAMETERS, HashTable *attribut
 
 	array_init(return_value);
 
-	if (FAILURE == read_attributes(return_value, attributes, scope, offset, target, name, base, filename)) {
+	if (FAILURE == read_attributes(return_value, attributes, scope, offset, target, name, base, filename, on_property_hook)) {
 		RETURN_THROWS();
 	}
 }
@@ -2066,15 +2069,21 @@ ZEND_METHOD(ReflectionFunctionAbstract, getAttributes)
 
 	GET_REFLECTION_OBJECT_PTR(fptr);
 
+	bool on_property_hook = false;
+
 	if (fptr->common.scope && (fptr->common.fn_flags & (ZEND_ACC_CLOSURE|ZEND_ACC_FAKE_CLOSURE)) != ZEND_ACC_CLOSURE) {
 		target = ZEND_ATTRIBUTE_TARGET_METHOD;
+		if (fptr->common.prop_info != NULL) {
+			on_property_hook = true;
+		}
 	} else {
 		target = ZEND_ATTRIBUTE_TARGET_FUNCTION;
 	}
 
 	reflect_attributes(INTERNAL_FUNCTION_PARAM_PASSTHRU,
 		fptr->common.attributes, 0, fptr->common.scope, target,
-		fptr->type == ZEND_USER_FUNCTION ? fptr->op_array.filename : NULL);
+		fptr->type == ZEND_USER_FUNCTION ? fptr->op_array.filename : NULL,
+		on_property_hook);
 }
 /* }}} */
 
@@ -2916,7 +2925,8 @@ ZEND_METHOD(ReflectionParameter, getAttributes)
 
 	reflect_attributes(INTERNAL_FUNCTION_PARAM_PASSTHRU,
 		attributes, param->offset + 1, scope, ZEND_ATTRIBUTE_TARGET_PARAMETER,
-		param->fptr->type == ZEND_USER_FUNCTION ? param->fptr->op_array.filename : NULL);
+		param->fptr->type == ZEND_USER_FUNCTION ? param->fptr->op_array.filename : NULL,
+		false);
 }
 
 /* {{{ Returns the index of the parameter, starting from 0 */
@@ -4038,7 +4048,8 @@ ZEND_METHOD(ReflectionClassConstant, getAttributes)
 
 	reflect_attributes(INTERNAL_FUNCTION_PARAM_PASSTHRU,
 		ref->attributes, 0, ref->ce, ZEND_ATTRIBUTE_TARGET_CLASS_CONST,
-		ref->ce->type == ZEND_USER_CLASS ? ref->ce->info.user.filename : NULL);
+		ref->ce->type == ZEND_USER_CLASS ? ref->ce->info.user.filename : NULL,
+		false);
 }
 /* }}} */
 
@@ -4443,7 +4454,8 @@ ZEND_METHOD(ReflectionClass, getAttributes)
 
 	reflect_attributes(INTERNAL_FUNCTION_PARAM_PASSTHRU,
 		ce->attributes, 0, ce, ZEND_ATTRIBUTE_TARGET_CLASS,
-		ce->type == ZEND_USER_CLASS ? ce->info.user.filename : NULL);
+		ce->type == ZEND_USER_CLASS ? ce->info.user.filename : NULL,
+		false);
 }
 /* }}} */
 
@@ -6385,7 +6397,8 @@ ZEND_METHOD(ReflectionProperty, getAttributes)
 
 	reflect_attributes(INTERNAL_FUNCTION_PARAM_PASSTHRU,
 		ref->prop->attributes, 0, ref->prop->ce, ZEND_ATTRIBUTE_TARGET_PROPERTY,
-		ref->prop->ce->type == ZEND_USER_CLASS ? ref->prop->ce->info.user.filename : NULL);
+		ref->prop->ce->type == ZEND_USER_CLASS ? ref->prop->ce->info.user.filename : NULL,
+		false);
 }
 /* }}} */
 
@@ -7348,13 +7361,21 @@ ZEND_METHOD(ReflectionAttribute, newInstance)
 		if (config != NULL && config->validator != NULL) {
 			config->validator(
 				attr->data,
-				flags | ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION,
+				attr->target | ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION,
 				attr->scope
 			);
 			if (EG(exception)) {
 				RETURN_THROWS();
 			}
 		}
+		/* For #[NoDiscard], the attribute does not work on property hooks, but
+		 * at runtime the validator has no way to access the method that an
+		 * attribute is applied to, attr->scope is just the overall class entry
+		 * that the method is a part of. */
+		if (ce == zend_ce_nodiscard && attr->on_property_hook) {
+			zend_throw_error(NULL, "#[\\NoDiscard] is not supported for property hooks");;
+			RETURN_THROWS();
+		}
 	}
 
 	/* Repetition validation is done even if #[DelayedTargetValidation] is used
@@ -7886,7 +7907,7 @@ ZEND_METHOD(ReflectionConstant, getAttributes)
 
 	reflect_attributes(INTERNAL_FUNCTION_PARAM_PASSTHRU,
 		const_->attributes, 0, NULL, ZEND_ATTRIBUTE_TARGET_CONST,
-		const_->filename);
+		const_->filename, false);
 }
 
 ZEND_METHOD(ReflectionConstant, __toString)