Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collision of interface constant and trait constant should not produce fatal error #16198

Open
Renji-FR opened this issue Oct 3, 2024 · 7 comments · May be fixed by #15878
Open

Collision of interface constant and trait constant should not produce fatal error #16198

Renji-FR opened this issue Oct 3, 2024 · 7 comments · May be fixed by #15878

Comments

@Renji-FR
Copy link

Renji-FR commented Oct 3, 2024

Description

The following code:

<?php
trait T1
{
	final public const string C1 = 'T1';
}

interface I1
{
	public const ?string C1 = null;
	public const ?string C2 = null;
}

final class O1 implements I1
{
	final public const string C2 = 'O1';
}

final class O2 implements I1
{
	use T1;
}

abstract class A1 implements I1
{
}

final class O3 extends A1
{
	final public const string C2 = 'O3';
}

final class O4 extends A1
{
	use T1;
}

Resulted in this output:

Fatal error: I1 and T1 define the same constant (C1) in the composition of O4. However, the definition differs and is considered incompatible. Class was composed in /home/user/scripts/code.php on line 32

But I expected this output instead:

No runtime error

This bug is related to the ticket #9271.


The behavior of a trait is to copy code into class, so if we can do something without trait and without error, it should be possible to do the same thing but with trait. I don't understand why I get this error with my code.

PHP Version

PHP 8.3.9

Operating System

Ubuntu 22.04

@cmb69
Copy link
Member

cmb69 commented Oct 3, 2024

Well, could go beyond this error with the following patch

 Zend/zend_inheritance.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c
index 88f1d3447c..fed00589f4 100644
--- a/Zend/zend_inheritance.c
+++ b/Zend/zend_inheritance.c
@@ -2689,7 +2689,7 @@ static void emit_incompatible_trait_constant_error(
 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;
+	uint32_t flags_mask = ZEND_ACC_PPP_MASK;
 
 	zval *zv = zend_hash_find_known_hash(&ce->constants_table, name);
 	if (zv == NULL) {

But then the compilation would fail, because trait constants are apparently invariant:

inheritance_status status1 = zend_perform_covariant_type_check(ce, existing_constant->type, traits[current_trait], trait_constant->type);
inheritance_status status2 = zend_perform_covariant_type_check(traits[current_trait], trait_constant->type, ce, existing_constant->type);
if (status1 == INHERITANCE_ERROR || status2 == INHERITANCE_ERROR) {

When lifting the restriction, the compilation would then fail because the constant values are not compatible (i.e. not the same, as is checked in this case):

if (!check_trait_property_or_constant_value_compatibility(ce, &trait_constant->value, &existing_constant->value)) {
/* There is an existing constant of the same name, and it conflicts with the new one, so let's throw a fatal error */
emit_incompatible_trait_constant_error(ce, existing_constant, trait_constant, name, traits, current_trait);
return false;
}

@Renji-FR
Copy link
Author

Renji-FR commented Oct 4, 2024

@cmb69 Thank you, so the fix requires more dev or it can not be fixed?
The behavior is different with trait and could not be fixed?

@cmb69
Copy link
Member

cmb69 commented Oct 4, 2024

Hmm, I think it can be "fixed", but I'm not sure whether the current implementation is by design. Maybe @kocsismate can explain the rationale of the current implementation.

@iluuu1994
Copy link
Member

Likely related to #15753, with a fix here. I quickly applied it to 8.3 and tried the test, and it indeed passes.

@Renji-FR
Copy link
Author

Renji-FR commented Oct 8, 2024

related to #16198

Thank you @iluuu1994

iluuu1994 added a commit to iluuu1994/php-src that referenced this issue Oct 14, 2024
@iluuu1994 iluuu1994 linked a pull request Oct 16, 2024 that will close this issue
@Renji-FR
Copy link
Author

What is the next step to have the fix in the next minor release please?

@iluuu1994
Copy link
Member

Nothing on your side. I need to think through the consequences of my patch when I have the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants