Skip to content

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

Open
AlessioGiacobbe wants to merge 1 commit into
swoole:masterfrom
AlessioGiacobbe:fix/trait-composition
Open

fix(translator): correct trait composition precedence and alias modifiers#19
AlessioGiacobbe wants to merge 1 commit into
swoole:masterfrom
AlessioGiacobbe:fix/trait-composition

Conversation

@AlessioGiacobbe

Copy link
Copy Markdown
Contributor

Problem

Three defects in composeTraitAst diverge from PHP trait semantics:

1. A concrete method fulfilling another trait's abstract requirement is dropped (order-dependent):

trait NeedsName { abstract public function name(): string; }
trait HasName { public function name(): string { return "HasName"; } }
class C { use NeedsName, HasName; }   // valid PHP

The branch unsets the new concrete statement while the already-merged abstract one stays in the class AST, so the compiled class keeps only the abstract declaration → Cannot instantiate abstract class C at runtime. use HasName, NeedsName; (reverse order) works.

2. Alias adaptations wipe static/final/abstract:

trait Maker { public static function make(): string { return "made"; } }
class Factory { use Maker { make as protected; } }

$traitStmt->flags = $alias['newModifier'] replaces the whole flag set with just the visibility bits, so make loses static and compiles as an instance method. PHP replaces only the visibility bits (verified via Reflection: make as protected keeps isStatic(), and m as final keeps the original visibility).

3. A class-defined method doesn't suppress trait-vs-trait conflicts:

trait A { function who() { return "A"; } }
trait B { function who() { return "B"; } }
class C { use A, B; function who() { return "C"; } }   // valid PHP, class wins

dies with the spurious fatal Trait `B` method `who` already exists because the class-method check ran after conflict resolution, and suppressed trait copies were still registered as trait methods.

Fix

  • Concrete-fulfills-abstract: drop the already-collected abstract declaration (from the class AST or the pending alias list) and keep the concrete method.
  • New applyTraitAliasModifier(): a new visibility replaces only Modifiers::VISIBILITY_MASK bits; a modifier without visibility keeps the original one.
  • The class-own-method check now runs before conflict resolution, and suppressed copies are not registered into $traitMethods.

Tests

  • New tests/compiler/trait/trait-composition-precedence.phpt covers all three (both trait orders for the abstract case, alias visibility change with same and new name calling through static::, class method winning over two conflicting traits). Expected output verified against PHP 8.4.
  • Full tests/compiler/trait/ suite: 41/41 pass. No new PHPStan errors on Translator.php.
  • On master, the new test's scenarios fail with Trait `WhoB` method `who` already exists at compile time and, with that scenario removed, Cannot instantiate abstract class AbstractFirst at runtime.

…iers

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.

@matyhtf matyhtf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for the detailed report and implementation. The three original defects are real, and the straightforward cases in the new PHPT are fixed correctly.

I found two remaining trait-semantics issues that need to be addressed before merging. These are implementation issues, not requests for additional test coverage.

  1. Abstract trait requirements are discarded without validating the concrete implementation.

The new early class-method check drops every same-name trait method, including abstract requirements:

trait RequiresValue
{
    abstract public function value(int $value): string;
}

class InvalidImplementation
{
    use RequiresValue;

    public function value(string $value): string
    {
        return $value;
    }
}

Zend rejects this because the class method is incompatible with the abstract trait method. The PR currently compiles and runs it. The same issue occurs when a concrete method from a later trait replaces an earlier abstract declaration: the abstract declaration is removed without checking the concrete signature.

Before dropping an abstract requirement, please validate the implementation using PHP-compatible method variance rules, including parameter contravariance, return covariance, visibility, static/by-reference/variadic behavior, and required parameters. The existing exact-string comparison used for two abstract declarations should not be reused blindly because a concrete implementation may be valid through variance without being textually identical.

  1. Multiple alias adaptations incorrectly depend on source order.
trait AliasSource
{
    public static function value(): string
    {
        return 'value';
    }
}

class AliasConsumer
{
    use AliasSource {
        value as protected;
        value as alias;
    }
}

In PHP, value becomes protected while alias remains public. In the PR, the first adaptation mutates $traitStmt, then the second adaptation clones that already-modified node, so alias incorrectly becomes protected and cannot be called externally.

Each adaptation should derive its flags independently from the immutable original method flags. A same-name visibility change must not affect the base flags used to create another alias.

I reproduced both differences against Zend PHP 8.4 and by compiling/running the PR head. All current CI jobs pass, but these two cases still diverge from Zend trait composition semantics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants