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