Merge pull request #19 from AlessioGiacobbe/fix/trait-composition

fix(translator): correct trait composition precedence and alias modifiers

phpunit/code/trait_abstract_alias_incompatible.php

@@ -0,0 +1,24 @@
+<?php
+trait RequiresValue
+{
+    abstract public function value(int $value): string;
+}
+
+// Aliasing an abstract method creates the requirement under the new name;
+// the class method defined under that name must satisfy it.
+class AliasImplementation
+{
+    use RequiresValue { value as renamed; }
+
+    public function renamed(string $value): string
+    {
+        return $value;
+    }
+
+    public function value(int $value): string
+    {
+        return "$value";
+    }
+}
+
+function main() {}

phpunit/code/trait_abstract_class_incompatible.php

@@ -0,0 +1,17 @@
+<?php
+trait RequiresValue
+{
+    abstract public function value(int $value): string;
+}
+
+class InvalidImplementation
+{
+    use RequiresValue;
+
+    public function value(string $value): string
+    {
+        return $value;
+    }
+}
+
+function main() {}

phpunit/code/trait_abstract_extra_required_param.php

@@ -0,0 +1,17 @@
+<?php
+trait RequiresValue
+{
+    abstract public function value(int $value): string;
+}
+
+class GreedyImplementation
+{
+    use RequiresValue;
+
+    public function value(int $value, int $extra): string
+    {
+        return "$value:$extra";
+    }
+}
+
+function main() {}

phpunit/code/trait_abstract_return_widened.php

@@ -0,0 +1,17 @@
+<?php
+trait RequiresValue
+{
+    abstract public function value(): string;
+}
+
+class WideningImplementation
+{
+    use RequiresValue;
+
+    public function value(): string|int
+    {
+        return 'value';
+    }
+}
+
+function main() {}

phpunit/code/trait_abstract_static_mismatch.php

@@ -0,0 +1,17 @@
+<?php
+trait RequiresValue
+{
+    abstract public function value(): string;
+}
+
+class StaticImplementation
+{
+    use RequiresValue;
+
+    public static function value(): string
+    {
+        return 'value';
+    }
+}
+
+function main() {}

phpunit/code/trait_abstract_trait_concrete_first_incompatible.php

@@ -0,0 +1,22 @@
+<?php
+trait NeedsName
+{
+    abstract public function name(int $id): string;
+}
+
+trait HasName
+{
+    public function name(string $id): string
+    {
+        return $id;
+    }
+}
+
+// The concrete method is collected first; the later abstract requirement is
+// dropped in its favor but must still be satisfied by it.
+class ConcreteFirst
+{
+    use HasName, NeedsName;
+}
+
+function main() {}

phpunit/code/trait_abstract_trait_concrete_incompatible.php

@@ -0,0 +1,22 @@
+<?php
+trait NeedsName
+{
+    abstract public function name(int $id): string;
+}
+
+trait HasName
+{
+    public function name(string $id): string
+    {
+        return $id;
+    }
+}
+
+// The abstract requirement is collected first; the later concrete method
+// replaces it and must be validated against it.
+class AbstractFirst
+{
+    use NeedsName, HasName;
+}
+
+function main() {}

phpunit/code/trait_abstract_variance_ok.php

@@ -0,0 +1,51 @@
+<?php
+trait RequiresConversion
+{
+    abstract public function convert(int $value): iterable;
+
+    abstract public function label(int|string $value): string;
+}
+
+// A valid implementation does not need to be textually identical to the
+// requirement: parameters are contravariant, returns are covariant, extra
+// optional parameters are allowed, and Zend places no visibility constraint
+// on the implementation of an abstract trait requirement.
+class ValidImplementation
+{
+    use RequiresConversion;
+
+    public function convert(int|float $value, int $extra = 0): array
+    {
+        return [$value, $extra];
+    }
+
+    protected function label(int|string|float $value): string
+    {
+        return "$value";
+    }
+}
+
+trait NeedsMaker
+{
+    abstract public function make(int $value): iterable;
+}
+
+trait HasMaker
+{
+    public function make(int|string $value): array
+    {
+        return [$value];
+    }
+}
+
+class TraitFulfillsTrait
+{
+    use NeedsMaker, HasMaker;
+}
+
+class TraitFulfillsTraitReversed
+{
+    use HasMaker, NeedsMaker;
+}
+
+function main() {}

phpunit/src/TraitAbstractRequirementTest.php

@@ -0,0 +1,75 @@
+<?php
+
+/**
+ * Trait composition drops an abstract requirement once a concrete method is
+ * available for the same name. These tests cover the validation that must
+ * happen before the requirement is dropped: the implementation — whether the
+ * class's own method or a concrete method from another trait — has to satisfy
+ * the abstract declaration under PHP's method variance rules.
+ */
+class TraitAbstractRequirementTest extends BaseTest
+{
+    public function testClassMethodMustSatisfyAbstractTraitRequirement(): void
+    {
+        $this->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');
+    }
+}

src/Translator.php

@@ -3067,7 +3067,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) {
@@ -3130,26 +3131,66 @@ 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 = $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 = $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;
                         }
+                        if (isset($methods[$methodName])) {
+                            // The class's own method always wins: suppressed
+                            // trait copies must not take part in trait-vs-trait
+                            // 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;
+                        }
                         if (isset($traitMethods[$methodName])) {
                             [$existingTraitName, $existingStmt] = $traitMethods[$methodName];
                             $newAbstract = $traitStmt->isAbstract();
@@ -3167,24 +3208,50 @@ 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) {
-                                // New concrete replaces existing abstract
+                                // The new concrete method fulfills the abstract
+                                // 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]);
+                                    }
+                                }
+                                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) {
@@ -3249,6 +3316,201 @@ 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;
+    }
+
+    /**
+     * 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<Node\Param> $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();

tests/compiler/trait/trait-composition-precedence.phpt

@@ -0,0 +1,80 @@
+--TEST--
+Trait composition: concrete fulfills abstract, alias keeps static, class method wins
+--FILE--
+<?php
+
+// Multiple adaptations of the same method each derive from the original
+// method: the same-name visibility change must not leak into the alias,
+// regardless of the order the adaptations are listed in.
+trait AliasSource {
+    public static function value(): string { return "value"; }
+}
+class AliasConsumer {
+    use AliasSource { value as protected; value as alias; }
+    public static function callValue(): string { return static::value(); }
+}
+class AliasConsumerReversed {
+    use AliasSource { value as alias; value as protected; }
+    public static function callValue(): string { return static::value(); }
+}
+
+// A concrete trait method fulfills an abstract requirement from another
+// trait, regardless of the order the traits are listed in.
+trait NeedsName { abstract public function name(): string; }
+trait HasName { public function name(): string { return "HasName"; } }
+class AbstractFirst { use NeedsName, HasName; }
+class ConcreteFirst { use HasName, NeedsName; }
+
+// An alias visibility change keeps the `static` flag.
+trait Maker {
+    public static function make(): string { return "made"; }
+}
+class Factory {
+    use Maker { make as protected; }
+    public static function build(): string { return static::make(); }
+}
+
+// An alias under a new name keeps the `static` flag too.
+trait Counter {
+    public static function count7(): int { return 7; }
+}
+class Stats {
+    use Counter { count7 as protected seven; }
+    public static function total(): int { return static::seven(); }
+}
+
+// The class's own method wins over two same-name trait methods without
+// this counting as a trait-vs-trait conflict.
+trait WhoA { public function who(): string { return "WhoA"; } }
+trait WhoB { public function who(): string { return "WhoB"; } }
+class Self1 { use WhoA, WhoB; public function who(): string { return "Self1"; } }
+
+// A method fulfilling an abstract requirement does not need to be textually
+// identical: contravariant parameters and covariant returns are valid.
+trait NeedsLabel { abstract public function label(int|string $v): iterable; }
+class WiderLabel {
+    use NeedsLabel;
+    public function label(int|string|float $v): array { return ["label:$v"]; }
+}
+
+function main(): void
+{
+    echo (new AbstractFirst())->name(), "\n";
+    echo (new ConcreteFirst())->name(), "\n";
+    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--
+HasName
+HasName
+made
+7
+Self1
+value value
+value value
+label:1