Skip to content

Fix #74872 - Disambiguate the source of a trait alias when possible #2616

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Zend/tests/traits/bug55554e.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,4 @@ class ReportCollision {
echo "ReportCollision: ";
$o = new ReportCollision;
--EXPECTF--
Fatal error: Trait method ReportCollision has not been applied, because there are collisions with other trait methods on ReportCollision in %s on line %d
Fatal error: Trait method TC2::ReportCollision has not been applied as ReportCollision::ReportCollision, because of collision with TC1::ReportCollision in %s on line %d
59 changes: 59 additions & 0 deletions Zend/tests/traits/bug74872.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
--TEST--
Bug #74872 (First Trait wins on importing same methods with diff alias)
--FILE--
<?php

trait Trait1
{
public function init()
{
echo("Trait1 - init\n");
}
}

trait Trait2
{
public function init()
{
echo("Trait2 - init\n");
}
}

class Test
{
use Trait1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we make this use Trait1, Trait3 and Trait3 also has an init method? If I'm reading your code right, it's just going to pick one of them. But I think this should be an error.

I'm wondering if an absolute trait method reference shouldn't be required when multiple traits are used in one block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now it still picks one of them in that case. The currrent behavior is to disambiguate when possible and keep the old behavior (pick one) when it's not possible to know. But yes, I think it's a good idea to error in those cases.

I've started porting this to 7.4 but still have to redo the opcache part. So I'll also rework that.

init as public method1trait1;
}

use Trait2 {
init as public method1trait2;
}

final public function __construct()
{
$this->init();
$this->method1trait1();
$this->method1trait2();
}

public function init()
{
echo("Test - init\n");
}
}

$test = new Test();

$reflection = new ReflectionClass( $test );

foreach($reflection->getTraitAliases() as $k => $v)
{
echo $k.' => '.$v."\n";
}
?>
--EXPECT--
Test - init
Trait1 - init
Trait2 - init
method1trait1 => Trait1::init
method1trait2 => Trait2::init
2 changes: 1 addition & 1 deletion Zend/tests/traits/bugs/case-sensitive.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,4 @@ class MyClass {
}
?>
--EXPECTF--
Fatal error: Trait method M1 has not been applied, because there are collisions with other trait methods on MyClass in %s on line %d
Fatal error: Trait method B::M1 has not been applied as MyClass::M1, because of collision with A::M1 in %s
2 changes: 1 addition & 1 deletion Zend/tests/traits/conflict001.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,4 @@ class TraitsTest {
}
?>
--EXPECTF--
Fatal error: Trait method hello has not been applied, because there are collisions with other trait methods on TraitsTest in %s on line %d
Fatal error: Trait method THello2::hello has not been applied as TraitsTest::hello, because of collision with THello1::hello in %s on line %d
2 changes: 1 addition & 1 deletion Zend/tests/traits/conflict003.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,4 @@ class Talker {

?>
--EXPECTF--
Fatal error: Trait method smallTalk has not been applied, because there are collisions with other trait methods on Talker in %s on line %d
Fatal error: Trait method B::smallTalk has not been applied as Talker::smallTalk, because of collision with A::smallTalk in %s on line %d
2 changes: 1 addition & 1 deletion Zend/tests/traits/error_011.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,4 @@ var_dump($x->test());

?>
--EXPECTF--
Fatal error: Trait method test has not been applied, because there are collisions with other trait methods on bar in %s on line %d
Fatal error: Trait method c::test has not been applied as bar::test, because of collision with foo::test in %s
2 changes: 1 addition & 1 deletion Zend/tests/traits/error_015.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,4 @@ var_dump($x->test());

?>
--EXPECTF--
Fatal error: Trait method test has not been applied, because there are collisions with other trait methods on bar in %s on line %d
Fatal error: Trait method baz::test has not been applied as bar::test, because of collision with foo::test in %s
2 changes: 1 addition & 1 deletion Zend/tests/traits/language010.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,4 @@ $o->world();

?>
--EXPECTF--
Fatal error: Trait method world has not been applied, because there are collisions with other trait methods on MyClass in %s on line %d
Fatal error: Trait method World::world has not been applied as MyClass::world, because of collision with Hello::hello in %s on line %d
2 changes: 1 addition & 1 deletion Zend/tests/traits/language011.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,4 @@ $o->sayWorld();

?>
--EXPECTF--
Fatal error: Trait method sayHello has not been applied, because there are collisions with other trait methods on MyClass in %s on line %d
Fatal error: Trait method World::sayHello has not been applied as MyClass::sayHello, because of collision with Hello::sayHello in %s on line %d
2 changes: 1 addition & 1 deletion Zend/tests/traits/language014.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,4 @@ $o->world();

?>
--EXPECTF--
Fatal error: Trait method hello has not been applied, because there are collisions with other trait methods on MyClass in %s on line %d
Fatal error: Trait method World::world has not been applied as MyClass::hello, because of collision with Hello::hello in %s on line %d
2 changes: 1 addition & 1 deletion Zend/tests/traits/methods_002.phpt
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,4 @@ var_dump(clone $o);

?>
--EXPECTF--
Fatal error: Trait method __clone has not been applied, because there are collisions with other trait methods on bar in %s on line %d
Fatal error: Trait method baz::__clone has not been applied as bar::__clone, because of collision with foo::__clone in %s
5 changes: 5 additions & 0 deletions Zend/zend.h
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,11 @@ typedef struct _zend_trait_alias {
* modifiers to be set on trait method
*/
uint32_t modifiers;

/**
* names of the traits for this alias
*/
zend_string **trait_names;
} zend_trait_alias;

struct _zend_class_entry {
Expand Down
9 changes: 7 additions & 2 deletions Zend/zend_compile.c
Original file line number Diff line number Diff line change
Expand Up @@ -6150,7 +6150,7 @@ static void zend_compile_trait_precedence(zend_ast *ast) /* {{{ */
}
/* }}} */

static void zend_compile_trait_alias(zend_ast *ast) /* {{{ */
static void zend_compile_trait_alias(zend_ast *ast, zend_ast *traits_ast) /* {{{ */
{
zend_ast *method_ref_ast = ast->child[0];
zend_ast *alias_ast = ast->child[1];
Expand All @@ -6169,6 +6169,11 @@ static void zend_compile_trait_alias(zend_ast *ast) /* {{{ */
alias = emalloc(sizeof(zend_trait_alias));
alias->trait_method = zend_compile_method_ref(method_ref_ast);
alias->modifiers = modifiers;
if (!alias->trait_method->class_name) {
alias->trait_names = zend_compile_name_list(traits_ast);
} else {
alias->trait_names = NULL;
}

if (alias_ast) {
alias->alias = zend_string_copy(zend_ast_get_str(alias_ast));
Expand Down Expand Up @@ -6227,7 +6232,7 @@ void zend_compile_use_trait(zend_ast *ast) /* {{{ */
zend_compile_trait_precedence(adaptation_ast);
break;
case ZEND_AST_TRAIT_ALIAS:
zend_compile_trait_alias(adaptation_ast);
zend_compile_trait_alias(adaptation_ast, ast->child[0]);
break;
EMPTY_SWITCH_DEFAULT_CASE()
}
Expand Down
70 changes: 50 additions & 20 deletions Zend/zend_inheritance.c
Original file line number Diff line number Diff line change
Expand Up @@ -1192,15 +1192,10 @@ static void zend_add_trait_method(zend_class_entry *ce, const char *name, zend_s
return;
} else if (UNEXPECTED(existing_fn->common.scope->ce_flags & ZEND_ACC_TRAIT)) {
/* two traits can't define the same non-abstract method */
#if 1
zend_error_noreturn(E_COMPILE_ERROR, "Trait method %s has not been applied, because there are collisions with other trait methods on %s",
name, ZSTR_VAL(ce->name));
#else /* TODO: better error message */
zend_error_noreturn(E_COMPILE_ERROR, "Trait method %s::%s has not been applied as %s::%s, because of collision with %s::%s",
ZSTR_VAL(fn->common.scope->name), ZSTR_VAL(fn->common.function_name),
ZSTR_VAL(ce->name), name,
ZSTR_VAL(existing_fn->common.scope->name), ZSTR_VAL(existing_fn->common.function_name));
#endif
} else {
/* inherited members are overridden by members inserted by traits */
/* check whether the trait method fulfills the inheritance requirements */
Expand Down Expand Up @@ -1245,7 +1240,7 @@ static int zend_traits_copy_functions(zend_string *fnname, zend_function *fn, ze
zend_string *lcname;
zend_function fn_copy;

/* apply aliases which are qualified with a class name, there should not be any ambiguity */
/* aliases which are qualified with a class name won't have any ambiguity. method names try to match with alias->trait_names */
if (ce->trait_aliases) {
alias_ptr = ce->trait_aliases;
alias = *alias_ptr;
Expand All @@ -1255,20 +1250,38 @@ static int zend_traits_copy_functions(zend_string *fnname, zend_function *fn, ze
&& (!alias->trait_method->ce || fn->common.scope == alias->trait_method->ce)
&& ZSTR_LEN(alias->trait_method->method_name) == ZSTR_LEN(fnname)
&& (zend_binary_strcasecmp(ZSTR_VAL(alias->trait_method->method_name), ZSTR_LEN(alias->trait_method->method_name), ZSTR_VAL(fnname), ZSTR_LEN(fnname)) == 0)) {
fn_copy = *fn;

/* if it is 0, no modifieres has been changed */
if (alias->modifiers) {
fn_copy.common.fn_flags = alias->modifiers | (fn->common.fn_flags ^ (fn->common.fn_flags & ZEND_ACC_PPP_MASK));
if (alias->trait_names) {
size_t i = 0;
while (alias->trait_names[i]) {
if (zend_binary_strcasecmp(
ZSTR_VAL(fn->common.scope->name),
ZSTR_LEN(fn->common.scope->name),
ZSTR_VAL(alias->trait_names[i]),
ZSTR_LEN(alias->trait_names[i])) == 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use zend_string_equals_ci here.


alias->trait_method->class_name = alias->trait_names[i];
}
i++;
}
}

lcname = zend_string_tolower(alias->alias);
zend_add_trait_method(ce, ZSTR_VAL(alias->alias), lcname, &fn_copy, overriden);
zend_string_release(lcname);
if (alias->trait_method->ce || alias->trait_method->class_name) {
fn_copy = *fn;

/* Record the trait from which this alias was resolved. */
if (!alias->trait_method->ce) {
alias->trait_method->ce = fn->common.scope;
/* if it is 0, no modifiers have been changed */
if (alias->modifiers) {
fn_copy.common.fn_flags = alias->modifiers | (fn->common.fn_flags ^ (fn->common.fn_flags & ZEND_ACC_PPP_MASK));
}

lcname = zend_string_tolower(alias->alias);
zend_add_trait_method(ce, ZSTR_VAL(alias->alias), lcname, &fn_copy, overriden);
zend_string_release(lcname);

/* Record the trait from which this alias was resolved. */
if (!alias->trait_method->ce) {
alias->trait_method->ce = fn->common.scope;
}
}
}
alias_ptr++;
Expand All @@ -1292,11 +1305,28 @@ static int zend_traits_copy_functions(zend_string *fnname, zend_function *fn, ze
&& (ZSTR_LEN(alias->trait_method->method_name) == ZSTR_LEN(fnname))
&& (zend_binary_strcasecmp(ZSTR_VAL(alias->trait_method->method_name), ZSTR_LEN(alias->trait_method->method_name), ZSTR_VAL(fnname), ZSTR_LEN(fnname)) == 0)) {

fn_copy.common.fn_flags = alias->modifiers | (fn->common.fn_flags ^ (fn->common.fn_flags & ZEND_ACC_PPP_MASK));
if (alias->trait_names) {
size_t i = 0;
while (alias->trait_names[i]) {
if (zend_binary_strcasecmp(
ZSTR_VAL(fn->common.scope->name),
ZSTR_LEN(fn->common.scope->name),
ZSTR_VAL(alias->trait_names[i]),
ZSTR_LEN(alias->trait_names[i])) == 0) {

/** Record the trait from which this alias was resolved. */
if (!alias->trait_method->ce) {
alias->trait_method->ce = fn->common.scope;
alias->trait_method->class_name = alias->trait_names[i];
}
i++;
}
}

if (alias->trait_method->ce || alias->trait_method->class_name) {
fn_copy.common.fn_flags = alias->modifiers | (fn->common.fn_flags ^ (fn->common.fn_flags & ZEND_ACC_PPP_MASK));

/** Record the trait from which this alias was resolved. */
if (!alias->trait_method->ce) {
alias->trait_method->ce = fn->common.scope;
}
}
}
alias_ptr++;
Expand Down
10 changes: 10 additions & 0 deletions Zend/zend_opcode.c
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,16 @@ void _destroy_zend_class_traits_info(zend_class_entry *ce)
zend_string_release(ce->trait_aliases[i]->alias);
}

if (ce->trait_aliases[i]->trait_names) {
size_t j = 0;
while (ce->trait_aliases[i]->trait_names[j]) {
zend_string_release(ce->trait_aliases[i]->trait_names[j]);
j++;
}

efree(ce->trait_aliases[i]->trait_names);
}

efree(ce->trait_aliases[i]);
i++;
}
Expand Down
1 change: 1 addition & 0 deletions ext/opcache/zend_accelerator_util_funcs.c
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@ static void zend_class_copy_ctor(zend_class_entry **pce)
memcpy(trait_aliases[i], ce->trait_aliases[i], sizeof(zend_trait_alias));
trait_aliases[i]->trait_method = emalloc(sizeof(zend_trait_method_reference));
memcpy(trait_aliases[i]->trait_method, ce->trait_aliases[i]->trait_method, sizeof(zend_trait_method_reference));
trait_aliases[i]->trait_names = ce->trait_aliases[i]->trait_names;
i++;
}
trait_aliases[i] = NULL;
Expand Down
25 changes: 25 additions & 0 deletions ext/opcache/zend_file_cache.c
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,19 @@ static void zend_file_cache_serialize_class(zval *zv,
}
}

if (q->trait_names) {
zend_string **r;

SERIALIZE_PTR(q->trait_names);
r = q->trait_names;
UNSERIALIZE_PTR(r);

while (*r) {
SERIALIZE_STR(*r);
r++;
}
}

if (q->alias) {
SERIALIZE_STR(q->alias);
}
Expand Down Expand Up @@ -1278,6 +1291,18 @@ static void zend_file_cache_unserialize_class(zval *zv,
}
}

if (q->trait_names) {
zend_string **r;

UNSERIALIZE_PTR(q->trait_names);
r = q->trait_names;

while (*r) {
UNSERIALIZE_STR(*r);
r++;
}
}

if (q->alias) {
UNSERIALIZE_STR(q->alias);
}
Expand Down
8 changes: 8 additions & 0 deletions ext/opcache/zend_persist.c
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,14 @@ static void zend_persist_class_entry(zval *zv)
sizeof(zend_trait_method_reference));
}

if (ce->trait_aliases[i]->trait_names) {
int j = 0;
while (ce->trait_aliases[i]->trait_names[j]) {
zend_accel_store_interned_string(ce->trait_aliases[i]->trait_names[j]);
j++;
}
}

if (ce->trait_aliases[i]->alias) {
zend_accel_store_interned_string(ce->trait_aliases[i]->alias);
}
Expand Down
8 changes: 8 additions & 0 deletions ext/opcache/zend_persist_calc.c
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,14 @@ static void zend_persist_class_entry_calc(zval *zv)
ADD_SIZE(sizeof(zend_trait_method_reference));
}

if (ce->trait_aliases[i]->trait_names) {
int j = 0;
while (ce->trait_aliases[i]->trait_names[j]) {
ADD_INTERNED_STRING(ce->trait_aliases[i]->trait_names[j], 0);
j++;
}
}

if (ce->trait_aliases[i]->alias) {
ADD_INTERNED_STRING(ce->trait_aliases[i]->alias, 0);
}
Expand Down