8.3 - cannot set finality and visibility for method from trait at the same time

[ondrejmirtes]

Description

The following code:

<?php

trait SimpleTrait
{
    private function foo() {}
}

class TraitFixtureWithFinalAndProtected
{
    use SimpleTrait {
        foo as final protected;
    }
}

$rm = new ReflectionMethod(TraitFixtureWithFinalAndProtected::class, 'foo');
var_dump($rm->isFinal());
var_dump($rm->isPrivate());
var_dump($rm->isProtected());
var_dump($rm->isPublic());

Resulted in this output:

bool(false)
bool(true)
bool(false)
bool(false)

But I expected this output instead:

bool(true)
bool(false)
bool(true)
bool(false)

Looks like as final protected is completely ignored, does not change the method but does not throw an error either.

Related to: #12854

PHP Version

PHP 8.3

Operating System

No response

[ndossche]

This is because of how this is parsed AFAICT (production rule trait_method_reference T_AS member_modifier identifier).
The parser sees this as "create a final alias of foo with the name protected".

That's also why this doesn't work:

<?php
trait SimpleTrait
{
    private function foo() {}
}

class TraitFixtureWithFinalAndProtected
{
    use SimpleTrait {
        foo as final protected alias;
    }
}

EDIT: and as visibility modifiers are semi-reserved, the parser can't be extended because it would be ambiguous. It would require changing the modifiers to be fully reserved.

[DanielEScherzer]

Just noting that since 8.3.2 the as final protected now also results in a warning

Warning: Private methods cannot be final as they are never overridden by other classes in /in/MGLuB on line 8

https://3v4l.org/MGLuB

Of the member modifiers

php-src/Zend/zend_language_parser.y

Lines 1068 to 1079 in edc94a8

	member_modifier:
	T_PUBLIC { $$ = T_PUBLIC; }
	| T_PROTECTED { $$ = T_PROTECTED; }
	| T_PRIVATE { $$ = T_PRIVATE; }
	| T_PUBLIC_SET { $$ = T_PUBLIC_SET; }
	| T_PROTECTED_SET { $$ = T_PROTECTED_SET; }
	| T_PRIVATE_SET { $$ = T_PRIVATE_SET; }
	| T_STATIC { $$ = T_STATIC; }
	| T_ABSTRACT { $$ = T_ABSTRACT; }
	| T_FINAL { $$ = T_FINAL; }
	| T_READONLY { $$ = T_READONLY; }
	;

the `T_READONLY` and `*_SET` cannot be applied to any methods

php-src/Zend/zend_compile.c

Lines 891 to 895 in edc94a8

	case T_READONLY:
	if (target == ZEND_MODIFIER_TARGET_PROPERTY || target == ZEND_MODIFIER_TARGET_CPP) {
	return ZEND_ACC_READONLY;
	}
	break;

php-src/Zend/zend_compile.c

Lines 914 to 928 in edc94a8

	case T_PUBLIC_SET:
	if (target == ZEND_MODIFIER_TARGET_PROPERTY || target == ZEND_MODIFIER_TARGET_CPP) {
	return ZEND_ACC_PUBLIC_SET;
	}
	break;
	case T_PROTECTED_SET:
	if (target == ZEND_MODIFIER_TARGET_PROPERTY || target == ZEND_MODIFIER_TARGET_CPP) {
	return ZEND_ACC_PROTECTED_SET;
	}
	break;
	case T_PRIVATE_SET:
	if (target == ZEND_MODIFIER_TARGET_PROPERTY || target == ZEND_MODIFIER_TARGET_CPP) {
	return ZEND_ACC_PRIVATE_SET;
	}
	break;

`T_STATIC` and `T_ABSTRACT` cannot be applied as method modifiers in trait aliases

php-src/Zend/zend_compile.c

Lines 8735 to 8742 in edc94a8

	static void zend_check_trait_alias_modifiers(uint32_t attr) /* {{{ */
	{
	if (attr & ZEND_ACC_STATIC) {
	zend_error_noreturn(E_COMPILE_ERROR, "Cannot use \"static\" as method modifier in trait alias");
	} else if (attr & ZEND_ACC_ABSTRACT) {
	zend_error_noreturn(E_COMPILE_ERROR, "Cannot use \"abstract\" as method modifier in trait alias");
	}
	}

meaning that the only modifiers that are actually considered are the 3 visibility modifiers, and T_FINAL. You can use any 1 of those. Given that you can't combine multiple visibility modifiers, the only thing that is not supported is combining final with visibility

and as visibility modifiers are semi-reserved, the parser can't be extended because it would be ambiguous. It would require changing the modifiers to be fully reserved.

That explains why we couldn't make foo as final protected apply visibility and finality, but is there any ambiguity if we allow foo as final protected foo? (or, your example foo as final protected alias) https://3v4l.org/0mdjo shows that this has always been a syntax error, so there wouldn't be anything currently using this that would have its meaning changed.

[DanielEScherzer]

Not sure why I had assigned myself but I'm not currently planning to work on this soon, presumably would require an RFC to change the grammar