From a0be8bf3499e3ff2aba2576604aefa0d7bbdfa98 Mon Sep 17 00:00:00 2001 From: Alessio Giacobbe Date: Fri, 28 Aug 2026 17:49:58 +0200 Subject: [PATCH 1/2] fix(translator): correct trait composition precedence and alias modifiers Three defects in composeTraitAst diverged from PHP trait semantics: 1. When a later trait supplied a concrete method for a name an earlier trait declared abstract, the branch unset the new concrete statement while the already-merged abstract one stayed in the class AST. The composed class kept only the abstract declaration, so instantiating it failed with "Cannot instantiate abstract class" even though the method body existed. The reverse trait order worked. Now the merged abstract declaration is dropped and the concrete method is kept. 2. Alias adaptations assigned the new modifier over the whole flag set, wiping static/final/abstract: `use Maker { make as protected; }` turned a static method into an instance method, and static:: calls on it miscompiled. PHP replaces only the visibility bits (and keeps the original visibility when the modifier carries none, e.g. `as final`). 3. A method defined by the class itself did not suppress the trait-vs-trait conflict check, so `class C { use A, B; function f(){} }` with f() in both traits died with a spurious "method already exists" fatal. The class-method check now runs before conflict resolution and suppressed trait copies are no longer registered as trait methods. --- src/Translator.php | 43 +++++++++++++--- .../trait/trait-composition-precedence.phpt | 51 +++++++++++++++++++ 2 files changed, 87 insertions(+), 7 deletions(-) create mode 100644 tests/compiler/trait/trait-composition-precedence.phpt diff --git a/src/Translator.php b/src/Translator.php index 57fb2b8a..fe561c62 100644 --- a/src/Translator.php +++ b/src/Translator.php @@ -3128,13 +3128,13 @@ CODE; $aliasName = strtolower($alias['newName']); if ($aliasName === $methodName) { if ($alias['newModifier']) { - $traitStmt->flags = $alias['newModifier']; + $traitStmt->flags = $this->applyTraitAliasModifier($traitStmt->flags, $alias['newModifier']); } } elseif (!isset($methods[$aliasName]) && !isset($traitMethods[$aliasName])) { $aliasStmt = clone $traitStmt; $aliasStmt->name = new Node\Identifier($alias['newName']); if ($alias['newModifier']) { - $aliasStmt->flags = $alias['newModifier']; + $aliasStmt->flags = $this->applyTraitAliasModifier($aliasStmt->flags, $alias['newModifier']); } $aliasStmts[] = $aliasStmt; $traitMethods[$aliasName] = [$traitFullName, $aliasStmt]; @@ -3144,6 +3144,13 @@ CODE; unset($traitStmts[$k1]); continue; } + if (isset($methods[$methodName])) { + // The class's own method always wins: suppressed + // trait copies must not take part in trait-vs-trait + // conflict resolution. + unset($traitStmts[$k1]); + continue; + } if (isset($traitMethods[$methodName])) { [$existingTraitName, $existingStmt] = $traitMethods[$methodName]; $newAbstract = $traitStmt->isAbstract(); @@ -3167,18 +3174,26 @@ CODE; } if (!$newAbstract && $existingAbstract) { - // New concrete replaces existing abstract + // The new concrete method fulfills the abstract + // requirement: drop the already-collected + // abstract declaration and keep this one. + foreach ($stmt->stmts as $k3 => $mergedStmt) { + if ($mergedStmt === $existingStmt) { + unset($stmt->stmts[$k3]); + } + } + foreach ($aliasStmts as $k3 => $pendingAliasStmt) { + if ($pendingAliasStmt === $existingStmt) { + unset($aliasStmts[$k3]); + } + } $traitMethods[$methodName] = [$traitFullName, $traitStmt]; - unset($traitStmts[$k1]); continue; } // Both concrete — error $this->fatalError($classStmt, "Trait `{$traitFullName}` method `{$methodName}` already exists"); } - if (isset($methods[$methodName])) { - unset($traitStmts[$k1]); - } $traitMethods[$methodName] = [$traitFullName, $traitStmt]; } if ($traitStmt instanceof Node\Stmt\ClassConst) { @@ -3243,6 +3258,20 @@ CODE; } + /** + * Apply a trait alias modifier the way PHP does: a new visibility replaces + * only the visibility bits, and every other flag (static, final, abstract) + * is kept. A modifier without visibility (e.g. `as final`) keeps the + * original visibility. + */ + private function applyTraitAliasModifier(int $flags, int $newModifier): int + { + if ($newModifier & Modifiers::VISIBILITY_MASK) { + $flags &= ~Modifiers::VISIBILITY_MASK; + } + return $flags | $newModifier; + } + private function cloneAstNode(Node $node): Node { $traverser = new NodeTraverser(); diff --git a/tests/compiler/trait/trait-composition-precedence.phpt b/tests/compiler/trait/trait-composition-precedence.phpt new file mode 100644 index 00000000..28f38b94 --- /dev/null +++ b/tests/compiler/trait/trait-composition-precedence.phpt @@ -0,0 +1,51 @@ +--TEST-- +Trait composition: concrete fulfills abstract, alias keeps static, class method wins +--FILE-- +name(), "\n"; + echo (new ConcreteFirst())->name(), "\n"; + echo Factory::build(), "\n"; + echo Stats::total(), "\n"; + echo (new Self1())->who(), "\n"; +} +?> +--EXPECT-- +HasName +HasName +made +7 +Self1 From ed65cd2a6b56c9e0d0fef1d17ac0f692bf141cd4 Mon Sep 17 00:00:00 2001 From: Alessio Giacobbe Date: Mon, 31 Aug 2026 11:22:58 +0200 Subject: [PATCH 2/2] fix(translator): validate abstract trait requirements, isolate alias flags Address both review findings on the trait composition fix: 1. An abstract trait requirement was discarded without validating the concrete implementation. composeTraitAst now validates the implementation - the class's own method, a concrete method from another trait (in either collection order), or a class method matching an aliased abstract - against the abstract declaration before dropping it, following Zend's trait-composition rules: matching staticness, a kept by-reference return, no additional required parameters, contravariant parameter types, and a covariant return type. Type variance reuses the existing override-check machinery on the preprocessed definitions; late-bound self/static/ parent keywords are unified through the recorded type keywords, and self-in-trait resolutions are remapped to the consuming class. Visibility is intentionally not restricted: Zend allows an implementation of any visibility to fulfill an abstract trait requirement (verified against Zend 8.4). 2. Multiple alias adaptations of the same method depended on source order because a same-name visibility change mutated the statement that later adaptations cloned. Every adaptation now derives its flags from the immutable original flags, and the original statement is only mutated after all adaptations are processed, so `value as protected; value as alias;` leaves `alias` public in both adaptation orders (matching Zend, where each adaptation derives from the original and the last same-name adaptation wins). Both behaviors were pinned against Zend PHP 8.4 before implementing. Co-Authored-By: Claude Fable 5 --- .../trait_abstract_alias_incompatible.php | 24 ++ .../trait_abstract_class_incompatible.php | 17 ++ .../trait_abstract_extra_required_param.php | 17 ++ .../code/trait_abstract_return_widened.php | 17 ++ .../code/trait_abstract_static_mismatch.php | 17 ++ ...ract_trait_concrete_first_incompatible.php | 22 ++ ...t_abstract_trait_concrete_incompatible.php | 22 ++ phpunit/code/trait_abstract_variance_ok.php | 51 ++++ phpunit/src/TraitAbstractRequirementTest.php | 75 ++++++ src/Translator.php | 247 +++++++++++++++++- .../trait/trait-composition-precedence.phpt | 29 ++ 11 files changed, 531 insertions(+), 7 deletions(-) create mode 100644 phpunit/code/trait_abstract_alias_incompatible.php create mode 100644 phpunit/code/trait_abstract_class_incompatible.php create mode 100644 phpunit/code/trait_abstract_extra_required_param.php create mode 100644 phpunit/code/trait_abstract_return_widened.php create mode 100644 phpunit/code/trait_abstract_static_mismatch.php create mode 100644 phpunit/code/trait_abstract_trait_concrete_first_incompatible.php create mode 100644 phpunit/code/trait_abstract_trait_concrete_incompatible.php create mode 100644 phpunit/code/trait_abstract_variance_ok.php create mode 100644 phpunit/src/TraitAbstractRequirementTest.php diff --git a/phpunit/code/trait_abstract_alias_incompatible.php b/phpunit/code/trait_abstract_alias_incompatible.php new file mode 100644 index 00000000..309ad472 --- /dev/null +++ b/phpunit/code/trait_abstract_alias_incompatible.php @@ -0,0 +1,24 @@ +exec( + 'Declaration of `InvalidImplementation::value()` must be compatible with `RequiresValue::value()`', + 'trait_abstract_class_incompatible.php' + ); + } + + public function testLaterConcreteTraitMethodMustSatisfyEarlierAbstract(): void + { + $this->exec( + 'Declaration of `HasName::name()` must be compatible with `NeedsName::name()`', + 'trait_abstract_trait_concrete_incompatible.php' + ); + } + + public function testEarlierConcreteTraitMethodMustSatisfyLaterAbstract(): void + { + $this->exec( + 'Declaration of `HasName::name()` must be compatible with `NeedsName::name()`', + 'trait_abstract_trait_concrete_first_incompatible.php' + ); + } + + public function testStaticnessMustMatchAbstractTraitRequirement(): void + { + $this->exec( + 'Cannot make non static method `RequiresValue::value()` static in class `StaticImplementation`', + 'trait_abstract_static_mismatch.php' + ); + } + + public function testImplementationCannotRequireMoreParameters(): void + { + $this->exec( + 'Declaration of `GreedyImplementation::value()` must be compatible with `RequiresValue::value()`', + 'trait_abstract_extra_required_param.php' + ); + } + + public function testReturnTypeCannotBeWidened(): void + { + $this->exec( + 'Declaration of `WideningImplementation::value()` must be compatible with `RequiresValue::value()`', + 'trait_abstract_return_widened.php' + ); + } + + public function testAliasedAbstractRequirementIsValidatedAgainstClassMethod(): void + { + $this->exec( + 'Declaration of `AliasImplementation::renamed()` must be compatible with `RequiresValue::value()`', + 'trait_abstract_alias_incompatible.php' + ); + } + + public function testValidVarianceIsAccepted(): void + { + // Contravariant parameters, covariant returns, extra optional + // parameters, and visibility changes are all valid ways to fulfill + // an abstract trait requirement. + $this->compile('trait_abstract_variance_ok.php'); + } +} diff --git a/src/Translator.php b/src/Translator.php index fe561c62..5cebcc81 100644 --- a/src/Translator.php +++ b/src/Translator.php @@ -3065,7 +3065,8 @@ CODE; $traitMethods = []; $traitConstants = []; $traitProperties = []; - $classDef = $this->getClass($className->toString()); + $consumingClass = $className->toString(); + $classDef = $this->getClass($consumingClass); foreach ($stmt->stmts as $classStmt) { if ($classStmt instanceof Node\Stmt\ClassMethod) { @@ -3124,22 +3125,44 @@ CODE; // passes ZendVM's runtime signature-compatibility checks. The // alias clones below inherit this rewrite. $this->reresolveTraitMethodAstLateBoundTypes($classDef, $traitFullName, $traitStmt); + // Every adaptation derives its flags from the original + // method's flags: a same-name visibility change must not + // leak into aliases of the same method that are processed + // after it, so the original statement is only mutated once + // all adaptations have been handled. + $originalFlags = $traitStmt->flags; + $adaptedFlags = $originalFlags; foreach ($classDef->traitAliases[$fullMethodName] ?? [] as $alias) { $aliasName = strtolower($alias['newName']); if ($aliasName === $methodName) { if ($alias['newModifier']) { - $traitStmt->flags = $this->applyTraitAliasModifier($traitStmt->flags, $alias['newModifier']); + $adaptedFlags = $this->applyTraitAliasModifier($originalFlags, $alias['newModifier']); } - } elseif (!isset($methods[$aliasName]) && !isset($traitMethods[$aliasName])) { + } elseif (isset($methods[$aliasName])) { + // The class defines the alias name itself. An + // abstract source is still a requirement the + // class method has to satisfy. + if ($traitStmt->isAbstract()) { + [$requirementSource, $requirementDef] = $this->resolveTraitStmtMethodDef($traitStmt, $traitFullName); + $this->validateTraitAbstractImplementation( + $classStmt, + $traitStmt, $requirementSource, $requirementDef, + $methods[$aliasName], $consumingClass, + $classDef->methods[$aliasName] ?? $classDef->abstractMethodDefs[$aliasName] ?? null, + $consumingClass, + ); + } + } elseif (!isset($traitMethods[$aliasName])) { $aliasStmt = clone $traitStmt; $aliasStmt->name = new Node\Identifier($alias['newName']); if ($alias['newModifier']) { - $aliasStmt->flags = $this->applyTraitAliasModifier($aliasStmt->flags, $alias['newModifier']); + $aliasStmt->flags = $this->applyTraitAliasModifier($originalFlags, $alias['newModifier']); } $aliasStmts[] = $aliasStmt; $traitMethods[$aliasName] = [$traitFullName, $aliasStmt]; } } + $traitStmt->flags = $adaptedFlags; if (isset($classDef->traitIgnored[$fullMethodName])) { unset($traitStmts[$k1]); continue; @@ -3147,7 +3170,18 @@ CODE; if (isset($methods[$methodName])) { // The class's own method always wins: suppressed // trait copies must not take part in trait-vs-trait - // conflict resolution. + // conflict resolution. An abstract trait method is + // still a requirement the class method must satisfy. + if ($traitStmt->isAbstract()) { + [$requirementSource, $requirementDef] = $this->resolveTraitStmtMethodDef($traitStmt, $traitFullName); + $this->validateTraitAbstractImplementation( + $classStmt, + $traitStmt, $requirementSource, $requirementDef, + $methods[$methodName], $consumingClass, + $classDef->methods[$methodName] ?? $classDef->abstractMethodDefs[$methodName] ?? null, + $consumingClass, + ); + } unset($traitStmts[$k1]); continue; } @@ -3168,15 +3202,33 @@ CODE; } if ($newAbstract && !$existingAbstract) { - // Existing concrete wins over new abstract + // Existing concrete wins over new abstract, but + // it must satisfy the abstract requirement. + [$requirementSource, $requirementDef] = $this->resolveTraitStmtMethodDef($traitStmt, $traitFullName); + [$implementationSource, $implementationDef] = $this->resolveTraitStmtMethodDef($existingStmt, $existingTraitName); + $this->validateTraitAbstractImplementation( + $classStmt, + $traitStmt, $requirementSource, $requirementDef, + $existingStmt, $implementationSource, $implementationDef, + $consumingClass, + ); unset($traitStmts[$k1]); continue; } if (!$newAbstract && $existingAbstract) { // The new concrete method fulfills the abstract - // requirement: drop the already-collected + // requirement: validate it against the + // requirement, then drop the already-collected // abstract declaration and keep this one. + [$requirementSource, $requirementDef] = $this->resolveTraitStmtMethodDef($existingStmt, $existingTraitName); + [$implementationSource, $implementationDef] = $this->resolveTraitStmtMethodDef($traitStmt, $traitFullName); + $this->validateTraitAbstractImplementation( + $classStmt, + $existingStmt, $requirementSource, $requirementDef, + $traitStmt, $implementationSource, $implementationDef, + $consumingClass, + ); foreach ($stmt->stmts as $k3 => $mergedStmt) { if ($mergedStmt === $existingStmt) { unset($stmt->stmts[$k3]); @@ -3272,6 +3324,187 @@ CODE; return $flags | $newModifier; } + /** + * Resolve the trait a flattened method statement originated from and its + * preprocessed method definition. Recursive trait composition tags every + * copied statement with its true origin, which may be a nested trait + * rather than the trait currently being applied. + * + * @return array{string, ?MethodDef} + */ + private function resolveTraitStmtMethodDef(Node\Stmt\ClassMethod $stmt, string $fallbackTrait): array + { + $origin = $stmt->getAttribute(self::TRAIT_ORIGIN_ATTRIBUTE); + if (!is_string($origin) || $origin === '') { + $origin = $fallbackTrait; + } + $originalName = $stmt->getAttribute(self::TRAIT_METHOD_ATTRIBUTE); + if (!is_string($originalName) || $originalName === '') { + $originalName = $stmt->name->toString(); + } + $def = null; + if ($this->hasClass($origin)) { + $originDef = $this->getClass($origin); + $lower = strtolower($originalName); + $def = $originDef->methods[$lower] ?? $originDef->abstractMethodDefs[$lower] ?? null; + } + return [$origin, $def]; + } + + /** + * Validate that a concrete method satisfies an abstract requirement + * declared by a trait, following Zend's trait-composition rules: the + * static modifier must match, an abstract by-reference return must be + * kept, the implementation cannot require more parameters, parameter + * types are contravariant, and the return type is covariant. Visibility + * is deliberately not restricted — Zend allows an implementation of any + * visibility to fulfill an abstract trait requirement. + */ + private function validateTraitAbstractImplementation( + Node $errorNode, + Node\Stmt\ClassMethod $requirement, + string $requirementSource, + ?MethodDef $requirementDef, + Node\Stmt\ClassMethod $implementation, + string $implementationSource, + ?MethodDef $implementationDef, + string $consumingClass, + ): void { + $requirementName = $requirement->name->toString(); + $implementationName = $implementation->name->toString(); + + if ($requirement->isStatic() !== $implementation->isStatic()) { + $this->fatalError($errorNode, $requirement->isStatic() + ? "Cannot make static method `{$requirementSource}::{$requirementName}()` non static in class `{$consumingClass}`" + : "Cannot make non static method `{$requirementSource}::{$requirementName}()` static in class `{$consumingClass}`"); + } + + $incompatible = function () use ($errorNode, $implementationSource, $implementationName, $requirementSource, $requirementName): never { + $this->fatalError( + $errorNode, + "Declaration of `{$implementationSource}::{$implementationName}()` must be compatible " . + "with `{$requirementSource}::{$requirementName}()`" + ); + }; + + // The requirement's by-reference return must be kept; the + // implementation may add one. + if ($requirement->byRef && !$implementation->byRef) { + $incompatible(); + } + + if ($this->countRequiredParams($implementation->params) > $this->countRequiredParams($requirement->params)) { + $incompatible(); + } + $implParamCount = count($implementation->params); + $lastImplParam = $implParamCount > 0 ? $implementation->params[$implParamCount - 1] : null; + foreach ($requirement->params as $i => $requiredParam) { + // A trailing variadic accepts every remaining requirement position. + $implParam = $implementation->params[$i] + ?? ($lastImplParam?->variadic ? $lastImplParam : null); + if ($implParam === null + || $implParam->byRef !== $requiredParam->byRef + || ($requiredParam->variadic && !$implParam->variadic) + ) { + $incompatible(); + } + } + foreach ($implementation->params as $i => $implParam) { + if ($i >= count($requirement->params) && !$implParam->default && !$implParam->variadic) { + $incompatible(); + } + } + + // Type variance is checked on the preprocessed definitions, whose + // names were resolved in each declaration's own lexical context. + $requirementFunc = $requirementDef?->functionDef; + $implementationFunc = $implementationDef?->functionDef; + if (!$requirementFunc || !$implementationFunc) { + return; + } + + $implArgs = $implementationFunc->argInfoList; + $lastImplArg = $implArgs === [] ? null : $implArgs[count($implArgs) - 1]; + foreach ($requirementFunc->argInfoList as $i => $requiredArg) { + $implArg = $implArgs[$i] ?? ($lastImplArg?->variadic ? $lastImplArg : null); + if ($implArg === null) { + continue; + } + // Late-bound keywords (`self`, `static`, `parent`) were resolved + // against different declaring types but denote the same type once + // both methods are flattened into the consuming class. + if ($requiredArg->typeKeyword !== '' && $requiredArg->typeKeyword === $implArg->typeKeyword) { + continue; + } + if (!$this->isParameterTypeOverrideCompatible($implArg, $requiredArg)) { + $incompatible(); + } + } + + if ($requirementFunc->returnTypeUndeclared) { + return; + } + if ($implementationFunc->returnTypeUndeclared) { + $incompatible(); + } + if ($requirementFunc->returnTypeKeyword !== '' + && $requirementFunc->returnTypeKeyword === $implementationFunc->returnTypeKeyword + ) { + return; + } + $requirementTypes = $this->remapInstanceofClass( + $this->getReturnAcceptedTypes($requirementFunc, $consumingClass), + $requirementSource, + $consumingClass, + ); + $implementationTypes = $this->remapInstanceofClass( + $this->getReturnAcceptedTypes($implementationFunc, $consumingClass), + $implementationSource, + $consumingClass, + ); + foreach ($implementationTypes as $implementationType) { + if (!$this->isReturnTypeCoveredBy($implementationType, $requirementTypes)) { + $incompatible(); + } + } + } + + /** + * @param array $params + */ + private function countRequiredParams(array $params): int + { + $required = 0; + foreach (array_values($params) as $i => $param) { + if (!$param->default && !$param->variadic) { + $required = $i + 1; + } + } + return $required; + } + + /** + * Replace `instanceof` references to a trait in a DNF type-check list with + * the consuming class. `self` (and `parent`) inside a trait were resolved + * to the trait itself during preprocessing, but once the method is + * flattened they denote the consuming class, and no object can ever be an + * instance of a trait. + */ + private function remapInstanceofClass(array $types, string $from, string $to): array + { + if ($from === $to) { + return $types; + } + foreach ($types as &$type) { + if (($type['kind'] ?? null) === 'allOf') { + $type['types'] = $this->remapInstanceofClass($type['types'], $from, $to); + } elseif (($type['kind'] ?? null) === 'instanceof' && ($type['class'] ?? null) === $from) { + $type['class'] = $to; + } + } + return $types; + } + private function cloneAstNode(Node $node): Node { $traverser = new NodeTraverser(); diff --git a/tests/compiler/trait/trait-composition-precedence.phpt b/tests/compiler/trait/trait-composition-precedence.phpt index 28f38b94..2c9dd7b4 100644 --- a/tests/compiler/trait/trait-composition-precedence.phpt +++ b/tests/compiler/trait/trait-composition-precedence.phpt @@ -3,6 +3,21 @@ Trait composition: concrete fulfills abstract, alias keeps static, class method --FILE-- name(), "\n"; @@ -41,6 +64,9 @@ function main(): void echo Factory::build(), "\n"; echo Stats::total(), "\n"; echo (new Self1())->who(), "\n"; + echo AliasConsumer::alias(), " ", AliasConsumer::callValue(), "\n"; + echo AliasConsumerReversed::alias(), " ", AliasConsumerReversed::callValue(), "\n"; + foreach ((new WiderLabel())->label(1) as $v) { echo $v, "\n"; } } ?> --EXPECT-- @@ -49,3 +75,6 @@ HasName made 7 Self1 +value value +value value +label:1