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.
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.

1 participant