[RFC] Implement constants in traits #8888

sj-i · 2022-06-28T22:21:19Z

RFC: https://wiki.php.net/rfc/constants_in_traits
Discussion thread at internals: https://externals.io/message/118039
Voting thread at internals (contains the final discussions): https://externals.io/message/118202
This also closes https://bugs.php.net/bug.php?id=75060

Zend/tests/traits/constant_001.phpt

Zend/zend_inheritance.c

sj-i

FWIW, I commented myself on the intent of each modification in the hope that it might help the review.

In this process, I noticed some unnecessary codes and some test cases that would be better to have, so I have also added a few commits to fix them.

sj-i · 2022-07-31T08:35:31Z

Zend/Optimizer/pass1.c

@@ -164,7 +164,7 @@ void zend_optimizer_pass1(zend_op_array *op_array, zend_optimizer_ctx *ctx)
 				if (ce) {
 					zend_class_constant *cc = zend_hash_find_ptr(
 						&ce->constants_table, Z_STR(ZEND_OP2_LITERAL(opline)));
-					if (cc && (ZEND_CLASS_CONST_FLAGS(cc) & ZEND_ACC_PPP_MASK) == ZEND_ACC_PUBLIC) {
+					if (cc && (ZEND_CLASS_CONST_FLAGS(cc) & ZEND_ACC_PPP_MASK) == ZEND_ACC_PUBLIC && !(ce->ce_flags & ZEND_ACC_TRAIT)) {


Commentary

This part is needed to prevent accessing trait constants directly when opcache is enabled.
The checking is usually done on the handler of ZEND_FETCH_CLASS_CONSTANT at runtime.

sj-i · 2022-07-31T08:38:11Z

Zend/zend_compile.c

+	if (c->ce->ce_flags & ZEND_ACC_TRAIT) {
+		return 0;
+	} else if (ZEND_CLASS_CONST_FLAGS(c) & ZEND_ACC_PUBLIC) {


Commentary

This condition is added to prevent accessing trait constants directly.
The current implementation checks direct accesses on runtime, the handler of ZEND_FETCH_CLASS_CONSTANT.
Trait constants are copied to the constants table of each composing class on zend_do_traits_constant_binding(), and their ce is replaced to the class entry of the composing class on that copying. So this condition is only met on directly accessing trait constants.

Can you inline this comment dirrectly into the code?

sj-i · 2022-07-31T08:39:10Z

Zend/zend_constants.c

+			if (UNEXPECTED(ce->ce_flags & ZEND_ACC_TRAIT)) {
+				if ((flags & ZEND_FETCH_CLASS_SILENT) == 0) {
+					zend_throw_error(NULL, "Cannot access trait constant %s::%s directly", ZSTR_VAL(class_name), ZSTR_VAL(constant_name));
+				}
+				goto failure;
+			}


Commentary

This part is added to prevent accessing trait constants directly on cases like \defined() or \constant(), etc.
On some cases trait constants are referenced outside of the handler of ZEND_FETCH_CLASS_CONSTANT.

Inline this comment into the code please.

sj-i · 2022-07-31T08:39:32Z

Zend/zend_inheritance.c

+static zend_always_inline bool check_trait_property_or_constant_value_compatibility(zend_class_entry *ce, zval *op1, zval *op2) /* {{{ */
+{
+	bool is_compatible;
+	zval op1_tmp, op2_tmp;
+
+	/* if any of the values is a constant, we try to resolve it */
+	if (UNEXPECTED(Z_TYPE_P(op1) == IS_CONSTANT_AST)) {
+		ZVAL_COPY_OR_DUP(&op1_tmp, op1);
+		zval_update_constant_ex(&op1_tmp, ce);
+		op1 = &op1_tmp;
+	}
+	if (UNEXPECTED(Z_TYPE_P(op2) == IS_CONSTANT_AST)) {
+		ZVAL_COPY_OR_DUP(&op2_tmp, op2);
+		zval_update_constant_ex(&op2_tmp, ce);
+		op2 = &op2_tmp;
+	}
+
+	is_compatible = fast_is_identical_function(op1, op2);
+
+	if (op1 == &op1_tmp) {
+		zval_ptr_dtor_nogc(&op1_tmp);
+	}
+	if (op2 == &op2_tmp) {
+		zval_ptr_dtor_nogc(&op2_tmp);
+	}
+
+	return is_compatible;
+}
+/* }}} */


Commentary

This function is extracted out from the implementation of zend_do_traits_property_binding() to share the implementation of the value compatibility check between properties and constants.

sj-i · 2022-07-31T08:41:35Z

Zend/zend_inheritance.c

+static zend_class_entry* find_first_constant_definition(zend_class_entry *ce, zend_class_entry **traits, size_t current_trait, zend_string *constant_name, zend_class_entry *colliding_ce) /* {{{ */
+{
+	size_t i;
+
+	if (colliding_ce == ce) {
+		for (i = 0; i < current_trait; i++) {
+			if (traits[i]
+				&& zend_hash_exists(&traits[i]->constants_table, constant_name)) {
+				return traits[i];
+			}
+		}
+	}
+
+	return colliding_ce;
+}
+/* }}} */


Commentary

This function is used to show the place of the existing conflicting definition in error messages when conflicts occur.
Since trait constants are flattened into the constants table of the composing class, and thus we lose information about which constant was defined in which trait, a process like this is needed to find the location of the first definition of the constant from traits.
The implementation is in line with the existing function for properties, which is now renamed to find_first_property_definition() from find_first_definition().

sj-i · 2022-07-31T15:43:30Z

Zend/zend_inheritance.c

+static bool do_trait_constant_check(zend_class_entry *ce, zend_class_constant *trait_constant, zend_string *name, zend_class_entry **traits, size_t current_trait) /* {{{ */
+{
+	uint32_t flags_mask = ZEND_ACC_PPP_MASK | ZEND_ACC_FINAL;
+
+	zval *zv = zend_hash_find_known_hash(&ce->constants_table, name);
+	if (zv == NULL) {
+		// No existing constant of the same name, so this one can be added
+		return true;
+	}
+
+	zend_class_constant *existing_constant = Z_PTR_P(zv);
+
+	if ((ZEND_CLASS_CONST_FLAGS(trait_constant) & flags_mask) == (ZEND_CLASS_CONST_FLAGS(existing_constant) & flags_mask)) {
+		if (check_trait_property_or_constant_value_compatibility(ce, &trait_constant->value, &existing_constant->value)) {
+			// There is an existing constant which is compatible with the new one, so no need to add it
+			return false;
+		}
+	}
+
+	// There is an existing constant of the same name, and it conflicts with the new one, so let's throw a fatal error
+	zend_error_noreturn(E_COMPILE_ERROR,
+		"%s and %s define the same constant (%s) in the composition of %s. However, the definition differs and is considered incompatible. Class was composed",
+		ZSTR_VAL(find_first_constant_definition(ce, traits, current_trait, name, existing_constant->ce)->name),
+		ZSTR_VAL(trait_constant->ce->name),
+		ZSTR_VAL(name),
+		ZSTR_VAL(ce->name));
+}
+/* }}} */


Commentary

This function checks for trait constants to be added to the composing class and has three possible outcomes:

There is no existing constant of the same name, so it can be added.

It is compatible with an existing constant of the same name, so it can be skipped without adding a new one.

It conflicts with an existing constant of the same name, so triggers a fatal error.

I'm confused here, doesn't the compiler complain that no value is returned here (in case of an error)?

And to be sure, true means "add constant to the hashtable", false do not, and otherwise error?

doesn't the compiler complain that no value is returned here

The zend_error_noreturn() is marked as ZEND_NORETURN, so the compiler doesn't complain about it.
But yeah, I'm not sure if it's OK or not, since php-src only does the same thing in a few places.

And to be sure, true means "add constant to the hashtable", false do not, and otherwise error?

Yes!

The current form clearly states without redundancy that no value is returned to the caller in this condition, but I am not strongly attached to this part.

Would it be clearer to invert the condition and put the error handling in the if-block so that it returns false on the face of the code, or add an explicit abort in the if-block?

I think the current order is fine, I was just surprised that the compiler didn't complain about not returning in case of an error, and I didn't know how you would handle it with just a boolean (but then you could still reutrn false as an error should not add the constant anyway)

I've changed the order, as I'm starting to feel that there should be fewer surprises in minor areas.

sj-i · 2022-07-31T16:25:35Z

Zend/zend_inheritance.c

+static void zend_do_traits_constant_binding(zend_class_entry *ce, zend_class_entry **traits) /* {{{ */
+{
+	size_t i;
+
+	for (i = 0; i < ce->num_traits; i++) {
+		zend_string *constant_name;
+		zend_class_constant *constant;
+
+		if (!traits[i]) {
+			continue;
+		}
+
+		ZEND_HASH_MAP_FOREACH_STR_KEY_PTR(&traits[i]->constants_table, constant_name, constant) {
+			if (do_trait_constant_check(ce, constant, constant_name, traits, i)) {
+				zend_class_constant *ct = NULL;
+
+				ct = zend_arena_alloc(&CG(arena),sizeof(zend_class_constant));
+				memcpy(ct, constant, sizeof(zend_class_constant));
+				constant = ct;
+
+				if (Z_TYPE(constant->value) == IS_CONSTANT_AST) {
+					ce->ce_flags &= ~ZEND_ACC_CONSTANTS_UPDATED;
+					ce->ce_flags |= ZEND_ACC_HAS_AST_CONSTANTS;
+				}
+
+				constant->ce = ce;
+				Z_TRY_ADDREF(constant->value);
+				constant->doc_comment = constant->doc_comment ? zend_string_copy(constant->doc_comment) : NULL;
+				if (constant->attributes && (!(GC_FLAGS(constant->attributes) & IS_ARRAY_IMMUTABLE))) {
+					GC_ADDREF(constant->attributes);
+				}
+
+				zend_hash_update_ptr(&ce->constants_table, constant_name, constant);
+			}
+		} ZEND_HASH_FOREACH_END();
+	}
+}
+/* }}} */


Commentary

This function flattens trait constants to the constants table of the composing class.

The constant definition is copied only if there is no existing constant of the same name.

The memory area for the copied constants is allocated by the arena allocator and is released at the end of the request.

Updating ce_flags of the composing class when trait constants hold a constant expression is the same as in interface implementations and inheritances.

The ce of the copied zend_class_constant is replaced by the class entry of the composing class.

This is because, unlike interface implementations and class inheritances, access control is done by the scope of the composing class.

Also, as a result of this, the values, doc comments, and attributes associated with the copied zend_class_constant are destroyed at the time of destroying the class entry of the composing class.

The handling of doc comments and attributes is in line with the behavior of trait properties.

Girgias

Other than some minor comments this LGTM

I'll let some time for other people to look at this but will merge before beta 2 is tagged if noone comes allong

Girgias · 2022-07-31T18:00:45Z

Zend/tests/traits/constant_016.phpt

+
+define('CONSTANT', 2);
+


Have a similar test with a runtime defined constant to see if this borks?

Actually, I chose define() with the intention of defining a constant at runtime here, but after the optimization of opcache, it seems to compile to the same code as const.
I will use eval to ensure it is defined at runtime.

You could use something from the $_SERVER array

Thanks for pointing this out! I've changed to use $_ENV

Girgias · 2022-07-31T18:01:28Z

Zend/tests/traits/constant_002.phpt

+class ComposingClass {
+    use TestTrait1;
+    use TestTrait3;
+    public const A = 42;
+}


Isn't it better to test that the composing class works by using two unrelated traits (i.e. a trait A and B where B doesn't use A)? Or is there already such a test case?

Girgias · 2022-07-31T18:02:09Z

Zend/zend_compile.c

+	if (c->ce->ce_flags & ZEND_ACC_TRAIT) {
+		return 0;
+	} else if (ZEND_CLASS_CONST_FLAGS(c) & ZEND_ACC_PUBLIC) {


Can you inline this comment dirrectly into the code?

Girgias · 2022-07-31T18:02:33Z

Zend/zend_constants.c

+			if (UNEXPECTED(ce->ce_flags & ZEND_ACC_TRAIT)) {
+				if ((flags & ZEND_FETCH_CLASS_SILENT) == 0) {
+					zend_throw_error(NULL, "Cannot access trait constant %s::%s directly", ZSTR_VAL(class_name), ZSTR_VAL(constant_name));
+				}
+				goto failure;
+			}


Inline this comment into the code please.

Girgias · 2022-07-31T18:06:17Z

Zend/zend_inheritance.c

+static bool do_trait_constant_check(zend_class_entry *ce, zend_class_constant *trait_constant, zend_string *name, zend_class_entry **traits, size_t current_trait) /* {{{ */
+{
+	uint32_t flags_mask = ZEND_ACC_PPP_MASK | ZEND_ACC_FINAL;
+
+	zval *zv = zend_hash_find_known_hash(&ce->constants_table, name);
+	if (zv == NULL) {
+		// No existing constant of the same name, so this one can be added
+		return true;
+	}
+
+	zend_class_constant *existing_constant = Z_PTR_P(zv);
+
+	if ((ZEND_CLASS_CONST_FLAGS(trait_constant) & flags_mask) == (ZEND_CLASS_CONST_FLAGS(existing_constant) & flags_mask)) {
+		if (check_trait_property_or_constant_value_compatibility(ce, &trait_constant->value, &existing_constant->value)) {
+			// There is an existing constant which is compatible with the new one, so no need to add it
+			return false;
+		}
+	}
+
+	// There is an existing constant of the same name, and it conflicts with the new one, so let's throw a fatal error
+	zend_error_noreturn(E_COMPILE_ERROR,
+		"%s and %s define the same constant (%s) in the composition of %s. However, the definition differs and is considered incompatible. Class was composed",
+		ZSTR_VAL(find_first_constant_definition(ce, traits, current_trait, name, existing_constant->ce)->name),
+		ZSTR_VAL(trait_constant->ce->name),
+		ZSTR_VAL(name),
+		ZSTR_VAL(ce->name));
+}
+/* }}} */


I'm confused here, doesn't the compiler complain that no value is returned here (in case of an error)?

And to be sure, true means "add constant to the hashtable", false do not, and otherwise error?

Girgias · 2022-08-02T14:55:46Z

Zend/zend_inheritance.c

+				/* Unlike interface implementations and class inheritances,
+				 * access control of the trait constants is done by the scope
+				 * of the composing class. So let's replace the ce here.
+				 * And the result of this, the values, doc comments, and
+				 * attributes associated with the copied zend_class_constant
+				 * are destroyed at the time of destroying the class entry of
+				 * the composing class.
+				 */
+				constant->ce = ce;


So basically attributes on trait constants are "useless"? Would it be possible to check at compile time no attributes are used?

So basically attributes on trait constants are "useless"?

Hmm? No, it's not. Maybe my wording is incorrect.

The sentences beginning with "And the result of this" are just to indicate the timing of the destruction of the data associated with the constant. Attributes on trait constants can be retrieved via either the trait itself or the composing class without problems.

For inheritance and interface implementation, the ce of zend_class_constant is kept as it was, while the same zend_class_constant is also inserted into the constants table of the inheriting child.

Destruction of class entries, such as destroy_zend_class(), has a step to check if the ce of zend_class_constant in the constants table matches the class entry about to be destroyed. So the destruction timing of associated data, like attributes or values, is at the time of the destruction of the parent classes or interfaces that define the constants, not at the time of the destruction of the inheriting child. OTH, for trait constants, the destruction timing of each copied zend_class_constant is when each composing class is destroyed.

Thinking again, I decided to remove the part that begins with "And the result of this".

It seems unlikely that this line will be overlooked when someone changes the implementation of access controls of trait constants. But it might be possible when changing the destruction of class entries.

On PR comments, it may now help people read the implementation, but if embedded into the code, it may cause unnecessary misunderstandings in the future. And if one wants to know the timing of destruction, seeing the actual implementation of destructing class entries would be needed anyway.

Removed that part and also added test cases for attributes and doc comments.

I think I got confused because you are talking about destroying the content, when in fact it was already copied into the class the trait is used.

Girgias · 2022-08-04T12:36:19Z

Test failure is unrelated, but could you rebase + force push, as beta 2 has already landed :( I just want to be sure CI mostly works before merging this

sj-i · 2022-08-04T14:30:18Z

@Girgias I just rebased this onto master, should I squash it too?

Edit: Squashed it too.

https://wiki.php.net/rfc/constants_in_traits php#8888

sj-i · 2022-08-04T19:20:33Z

@Girgias Thanks for reviewing and merging!

see GH-8888

> Constants in Traits > > It is now possible to define constants in traits. This adds a new sniff to detect this. Includes unit tests. Includes docs. Refs: * https://wiki.php.net/rfc/constants_in_traits * https://www.php.net/manual/en/migration82.new-features.php#migration82.new-features.core.constant-in-traits * https://www.php.net/manual/en/language.oop5.traits.php#language.oop5.traits.constants * php/php-src#8888

github-actions bot added the Category: Engine label Jun 28, 2022

sj-i force-pushed the constants-in-traits branch from f52f0ab to 0e3e89c Compare June 28, 2022 22:44

github-actions bot added the Category: Optimizer label Jun 29, 2022

TimWolla added the RFC label Jun 29, 2022

sj-i marked this pull request as draft June 30, 2022 23:18

sj-i marked this pull request as ready for review June 30, 2022 23:18

TimWolla added the Waiting on Review label Jul 19, 2022

TimWolla added this to the PHP 8.2 milestone Jul 19, 2022

guilliamxavier reviewed Jul 19, 2022

View reviewed changes

Zend/tests/traits/constant_001.phpt Outdated Show resolved Hide resolved

guilliamxavier reviewed Jul 19, 2022

View reviewed changes

Zend/zend_inheritance.c Outdated Show resolved Hide resolved

sj-i force-pushed the constants-in-traits branch from cea4096 to 15ffed3 Compare July 31, 2022 08:33

sj-i commented Jul 31, 2022

View reviewed changes

Girgias approved these changes Jul 31, 2022

View reviewed changes

Girgias reviewed Aug 2, 2022

View reviewed changes

sj-i force-pushed the constants-in-traits branch from c231bdf to b171f84 Compare August 4, 2022 13:53

Implement constants in traits

0813127

https://wiki.php.net/rfc/constants_in_traits php#8888

sj-i force-pushed the constants-in-traits branch from b171f84 to 0813127 Compare August 4, 2022 15:29

Girgias merged commit 3b62d66 into php:master Aug 4, 2022

sj-i mentioned this pull request Aug 4, 2022

[ci skip] NEWS and UPGRADING for constants in traits #9251

Merged

TimWolla pushed a commit that referenced this pull request Aug 4, 2022

[ci skip] NEWS and UPGRADING for constants in traits (#9251)

038c451

see GH-8888

TimWolla removed the Waiting on Review label Aug 4, 2022

jrfnl mentioned this pull request Dec 25, 2022

PHP 8.2 | New PHPCompatibility.Constants.NewConstantsInTraits sniff PHPCompatibility/PHPCompatibility#1443

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Implement constants in traits #8888

[RFC] Implement constants in traits #8888

sj-i commented Jun 28, 2022 •

edited

Loading

sj-i left a comment

sj-i Jul 31, 2022

sj-i Jul 31, 2022

Girgias Jul 31, 2022

sj-i Aug 4, 2022

sj-i Jul 31, 2022

Girgias Jul 31, 2022

sj-i Aug 4, 2022

sj-i Jul 31, 2022

sj-i Jul 31, 2022

sj-i Jul 31, 2022

Girgias Jul 31, 2022

sj-i Aug 1, 2022

Girgias Aug 2, 2022

sj-i Aug 2, 2022

sj-i Jul 31, 2022

Girgias left a comment

Girgias Jul 31, 2022

sj-i Aug 2, 2022 •

edited

Loading

Girgias Aug 2, 2022

sj-i Aug 4, 2022

Girgias Jul 31, 2022

sj-i Aug 4, 2022

Girgias Jul 31, 2022

Girgias Jul 31, 2022

Girgias Jul 31, 2022

Girgias Aug 2, 2022

sj-i Aug 2, 2022

sj-i Aug 3, 2022 •

edited

Loading

sj-i Aug 4, 2022

Girgias Aug 4, 2022

Girgias commented Aug 4, 2022

sj-i commented Aug 4, 2022 •

edited

Loading

sj-i commented Aug 4, 2022


		define('CONSTANT', 2);

[RFC] Implement constants in traits #8888

[RFC] Implement constants in traits #8888

Conversation

sj-i commented Jun 28, 2022 • edited Loading

sj-i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Girgias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sj-i Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sj-i Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Girgias commented Aug 4, 2022

sj-i commented Aug 4, 2022 • edited Loading

sj-i commented Aug 4, 2022

sj-i commented Jun 28, 2022 •

edited

Loading

sj-i Aug 2, 2022 •

edited

Loading

sj-i Aug 3, 2022 •

edited

Loading

sj-i commented Aug 4, 2022 •

edited

Loading