Validators return error messages

Author: DanielEScherzer

UPGRADING.INTERNALS

@@ -28,17 +28,16 @@ PHP 8.5 INTERNALS UPGRADE NOTES
     extra layer of indirection can be removed. In other cases a zval can
     be heap-allocated and stored in the pointer as a minimal change to keep
     compatibility.
+  * The validator callbacks for internal attribute now return `zend_string *`
+    rather than `void`; instead of emitting an error when an attribute is
+    applied incorrectly, the error message should be returned as a zend_string
+    pointer.
   . The `target` parameter for validator callbacks for internal attributes
-    can now include in the bitmask the new flags
-    ZEND_ATTRIBUTE_NO_TARGET_VALIDATION and
-    ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION. The first indicates that, if the
-    attribute is being applied to an invalid target, no error should be thrown
-    at compile time. The second indicates that validation is being performed
-    at runtime and that if an attribute is being applied to an invalid target,
-    a catchable error should be thrown. Target validation is different from
-    the actual functionality of the attribute; e.g. the #[\Override] attribute
-    will trigger an error if the function does not override a method from the
-    parent class or from an interface; that error does not get delayed.
+    can now include in the bitmask the new flag
+    ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION. The flag indicates that
+    validation is being performed at runtime and that other than returning an
+    error message, the validator should not do anything, most notably including
+    modification of any class properties.
     RFC: https://wiki.php.net/rfc/delayedtargetvalidation_attribute
 
 - Hash

Zend/zend_attributes.c

@@ -70,16 +70,10 @@ uint32_t zend_attribute_attribute_get_flags(zend_attribute *attr, zend_class_ent
 	return ZEND_ATTRIBUTE_TARGET_ALL;
 }
 
-static void validate_allow_dynamic_properties(
+static zend_string *validate_allow_dynamic_properties(
 		zend_attribute *attr, uint32_t target, zend_class_entry *scope)
 {
-	if (scope == NULL) {
-		/* Only reachable when validator is run but the attribute isn't applied
-		 * to a class; in the case of delayed target validation reflection will
-		 * complain about the target before running the validator. */
-		ZEND_ASSERT(target & ZEND_ATTRIBUTE_NO_TARGET_VALIDATION);
-		return;
-	}
+	ZEND_ASSERT(scope != NULL);
 	const char *msg = NULL;
 	if (scope->ce_flags & ZEND_ACC_TRAIT) {
 		msg = "Cannot apply #[\\AllowDynamicProperties] to trait %s";
@@ -91,27 +85,20 @@ static void validate_allow_dynamic_properties(
 		msg = "Cannot apply #[\\AllowDynamicProperties] to enum %s";
 	}
 	if (msg != NULL) {
-		if (target & ZEND_ATTRIBUTE_NO_TARGET_VALIDATION) {
-			return;
-		}
-		if (target & ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION) {
-			// Should not have passed the first time
-			ZEND_ASSERT((scope->ce_flags & ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES) == 0);
-			// Throw a catchable error at runtime
-			zend_throw_error(NULL, msg, ZSTR_VAL(scope->name));
-			return;
-		}
-		zend_error_noreturn(E_ERROR, msg, ZSTR_VAL(scope->name) );
+		smart_str str = {0};
+		smart_str_append_printf(&str, msg, ZSTR_VAL(scope->name));
+		return smart_str_extract(&str);
 	}
 	if (target & ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION) {
 		// Should have passed the first time
 		ZEND_ASSERT((scope->ce_flags & ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES) != 0);
-		return;
+		return NULL;
 	}
 	scope->ce_flags |= ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES;
+	return NULL;
 }
 
-static void validate_attribute(
+static zend_string *validate_attribute(
 	zend_attribute *attr, uint32_t target, zend_class_entry *scope)
 {
 	const char *msg = NULL;
@@ -125,15 +112,11 @@ static void validate_attribute(
 		msg = "Cannot apply #[\\Attribute] to abstract class %s";
 	}
 	if (msg != NULL) {
-		if (target & ZEND_ATTRIBUTE_NO_TARGET_VALIDATION) {
-			return;
-		}
-		if (target & ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION) {
-			zend_throw_error(NULL, msg, ZSTR_VAL(scope->name));
-			return;
-		}
-		zend_error_noreturn(E_ERROR, msg, ZSTR_VAL(scope->name));
+		smart_str str = {0};
+		smart_str_append_printf(&str, msg, ZSTR_VAL(scope->name));
+		return smart_str_extract(&str);
 	}
+	return NULL;
 }
 
 ZEND_METHOD(Attribute, __construct)
@@ -235,7 +218,7 @@ ZEND_METHOD(Deprecated, __construct)
 	}
 }
 
-static void validate_nodiscard(
+static zend_string *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
@@ -247,16 +230,14 @@ static void validate_nodiscard(
 		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;
+			return NULL;
 		}
 		// 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");
+		return ZSTR_INIT_LITERAL("#[\\NoDiscard] is not supported for property hooks", 0);
 	}
 	/* At runtime, no way to identify the target method; Reflection will handle
 	 * throwing the error if needed. */
+	 return NULL;
 }
 
 ZEND_METHOD(NoDiscard, __construct)

Zend/zend_attributes.h

@@ -34,13 +34,9 @@
 #define ZEND_ATTRIBUTE_IS_REPEATABLE		(1<<7)
 #define ZEND_ATTRIBUTE_FLAGS				((1<<8) - 1)
 
-/* Not a real flag, just passed to validators when target validation is
- * suppressed; must not conflict with any of the real flags above. */
-#define ZEND_ATTRIBUTE_NO_TARGET_VALIDATION (1<<8)
-
 /* Not a real flag, just passed to validators when target validation is
  * being run at runtime; must not conflict with any of the real flags above. */
-#define ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION (1<<9)
+#define ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION (1<<8)
 
 /* Flags for zend_attribute.flags */
 #define ZEND_ATTRIBUTE_PERSISTENT   (1<<0)
@@ -79,7 +75,7 @@ typedef struct _zend_attribute {
 typedef struct _zend_internal_attribute {
 	zend_class_entry *ce;
 	uint32_t flags;
-	void (*validator)(zend_attribute *attr, uint32_t target, zend_class_entry *scope);
+	zend_string* (*validator)(zend_attribute *attr, uint32_t target, zend_class_entry *scope);
 } zend_internal_attribute;
 
 ZEND_API zend_attribute *zend_get_attribute(HashTable *attributes, zend_string *lcname);

Zend/zend_compile.c

@@ -7575,7 +7575,6 @@ static void zend_compile_attributes(
 				strlen("delayedtargetvalidation")
 			);
 		}
-		uint32_t extra_flags = delayed_target_validation ? ZEND_ATTRIBUTE_NO_TARGET_VALIDATION : 0;
 		/* Validate attributes in a secondary loop (needed to detect repeated attributes). */
 		ZEND_HASH_PACKED_FOREACH_PTR(*attributes, attr) {
 			if (attr->offset != offset || NULL == (config = zend_internal_attribute_get(attr->lcname))) {
@@ -7603,7 +7602,13 @@ static void zend_compile_attributes(
 
 			// 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_string *error = config->validator(attr, target, CG(active_class_entry));
+				if (error != NULL) {
+					if (delayed_target_validation == false) {
+						zend_error_noreturn(E_COMPILE_ERROR, ZSTR_VAL(error));
+					}
+					zend_string_efree(error);
+				}
 			}
 		} ZEND_HASH_FOREACH_END();
 	}

ext/reflection/php_reflection.c

@@ -7375,12 +7375,14 @@ ZEND_METHOD(ReflectionAttribute, newInstance)
 	if (delayed_target_validation && ce->type == ZEND_INTERNAL_CLASS) {
 		zend_internal_attribute *config = zend_internal_attribute_get(attr->data->lcname);
 		if (config != NULL && config->validator != NULL) {
-			config->validator(
+			zend_string *error = config->validator(
 				attr->data,
 				attr->target | ZEND_ATTRIBUTE_DELAYED_TARGET_VALIDATION,
 				attr->scope
 			);
-			if (EG(exception)) {
+			if (error != NULL) {
+				zend_throw_exception(zend_ce_error, ZSTR_VAL(error), 0);
+				zend_string_efree(error);
 				RETURN_THROWS();
 			}
 		}
@@ -7389,7 +7391,7 @@ ZEND_METHOD(ReflectionAttribute, newInstance)
 		 * 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");;
+			zend_throw_error(NULL, "#[\\NoDiscard] is not supported for property hooks");
 			RETURN_THROWS();
 		}
 	}

ext/zend_test/test.c

@@ -958,11 +958,12 @@ static zend_function *zend_test_class_static_method_get(zend_class_entry *ce, ze
 	return zend_std_get_static_method(ce, name, NULL);
 }
 
-void zend_attribute_validate_zendtestattribute(zend_attribute *attr, uint32_t target, zend_class_entry *scope)
+zend_string *zend_attribute_validate_zendtestattribute(zend_attribute *attr, uint32_t target, zend_class_entry *scope)
 {
 	if (target != ZEND_ATTRIBUTE_TARGET_CLASS) {
-		zend_error(E_COMPILE_ERROR, "Only classes can be marked with #[ZendTestAttribute]");
+		return ZSTR_INIT_LITERAL("Only classes can be marked with #[ZendTestAttribute]", 0);
 	}
+	return NULL;
 }
 
 static ZEND_METHOD(_ZendTestClass, __toString)