Skip to content

fix(translator): trait member value comparison and adaptation validation - #60

Open
AlessioGiacobbe wants to merge 7 commits into
swoole:masterfrom
AlessioGiacobbe:split/trait-composition-checks
Open

fix(translator): trait member value comparison and adaptation validation#60
AlessioGiacobbe wants to merge 7 commits into
swoole:masterfrom
AlessioGiacobbe:split/trait-composition-checks

Conversation

@AlessioGiacobbe

Copy link
Copy Markdown
Contributor

Two trait-composition gaps:

  • Trait constant/property conflicts were compared by pretty-printed source text: two traits declaring const int X = 1 + 1; and const int X = 2; (or defaults spelled [1, 2] vs array(1, 2)) were rejected as conflicting, while Zend compares evaluated values. Values are now compared by evaluation — with Zend's declaration-time int→float coercion for float-typed members — falling back to source-text equality only when a value cannot be evaluated at compile time. Different values, visibility, or types still conflict.
  • Trait adaptations referencing nonexistent methods were silently ignored: use A { missing as g; } and use A, B { B::f insteadof A; } (with no B::f) both compiled — a typo in an adaptation did nothing. Every alias and precedence rule is now verified against the composed methods, with Zend's diagnostics; an unqualified alias is registered under every used trait, so its variants share one group satisfied by any match, and precedence rules also verify the named traits are actually used.

Verified against Zend 8.4.13; nested-trait aliasing covered.

Part of the split of #39.

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

Two invalid programs still compile.

  1. traitIgnored stores a single rule under the loser Trait::method key. A later rule for the same loser overwrites the earlier rule, so not every preferred method is validated:
trait A { public function other(): void {} }
trait B { public function f(): void {} }
trait C { public function f(): void {} }
class X {
    use A, B, C {
        A::f insteadof B;
        C::f insteadof B;
    }
}

Zend reports A precedence rule was defined for A::f but this method does not exist; this PR compiles it. Preserve every precedence-rule record separately from (or as a list behind) the ignored-method lookup, and validate every record.

  1. Trait constant identity short-circuits on ConstantDef::$value. Different enum classes with the same case name are both normalized to the same string and are incorrectly accepted:
enum E1 { case Value; }
enum E2 { case Value; }
trait T1 { const X = E1::Value; }
trait T2 { const X = E2::Value; }
class X { use T1, T2; }

Zend rejects these distinct case objects; this PR compiles them. Do not use the normalized case-name string as an identity proof. Retain/compare the enum class and case identity, and add positive coverage for the same enum case plus negative coverage for different enum classes.

@AlessioGiacobbe
AlessioGiacobbe force-pushed the split/trait-composition-checks branch from f2b57d0 to d453c5e Compare September 2, 2026 08:14
@AlessioGiacobbe

Copy link
Copy Markdown
Contributor Author

Both fixed, rebased on current master:

  1. Precedence rules — every rule record is now preserved as a list behind the loser key (the isset()-based ignore lookup is unchanged), prepareTraitUse merges per-key instead of dropping earlier use clauses' rules, and validation iterates every record: your repro fails with Zend's exact wording (A precedence rule was defined for A::f but this method does not exist). Keeping all records also let me implement Zend's duplicate-exclusion rule ("Method of trait B was defined to be excluded multiple times" — probed), reported after the existence checks in Zend's order. One winner excluding several losers stays legal (probed).
  2. Enum case identity — the trait-member value comparison no longer collapses cases to a scalar: the evaluation path returns a collision-proof (enum class)::(case) identity used only by the trait comparator (general constant folding untouched), and the premature value === value short-circuit — which treated every non-literal initializer as equal because they all share an empty compiled value — now defers to the evaluated comparison. All six probed combinations match Zend: same enum + same case compiles; different enums, same enum different cases, different backed enums with equal backing, and array-wrapped cases all reject.

17/17 focused tests green; sweep and full suite identical to base. One scope note: a case reached indirectly through another class constant still collapses to its scalar in this comparator — happy to cover that in a follow-up if you'd like it here.

@AlessioGiacobbe
AlessioGiacobbe force-pushed the split/trait-composition-checks branch from 7dd9185 to bb781a5 Compare September 2, 2026 09:19

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

Please rebase this branch onto the latest master; #45, #52, #59, #63, and #66 have now been merged.

The enum-case identity marker is still representable by user code. PHP strings are binary-safe, so the NUL prefix does not make this value collision-free:

enum E { case A; }
trait T1 { const X = E::A; }
trait T2 { const X = "\0enum-case\0e::A"; }
class C { use T1, T2; }

Zend rejects the composition because an enum-case object and a string are different values. This branch evaluates both definitions to the same marker string and may accept them.

Please use an internal typed identity value that PHP constant expressions cannot construct, with stable/interned identity for repeated references to the same enum case, and add the collision above as a negative regression test. Arrays containing enum cases must preserve the same distinction recursively.

Zend decides trait constant/property compatibility by comparing the
EVALUATED definition (zend_is_identical on the resolved zvals plus
matching flags and declared type), so `const int X = 1 + 1` in one
trait and `const int X = 2` in another are the same definition, as are
`[1, 2]` and `array(1, 2)` property defaults. composeTraitAst()
compared pretty-printed source text and isCompatibleTraitConstant()
compared lowered value strings, rejecting these valid compositions.

Evaluate both initializers with the existing evaluateClassConstValue()
machinery and compare with identity semantics (1 vs 1.0 or 1 vs '1'
still conflict, matching Zend), coercing an integer initializer to
float first when the member's declared type is float — Zend performs
that coercion at declaration time, so `public float $f = 1` and
`= 1.0` are identical. When a value cannot be evaluated at compile
time the previous source-text comparison remains as the fallback.
Flag, declared-type and (for properties) presence-of-default equality
checks are unchanged.
composeTraitAst() consumed matching traitAliases/traitIgnored entries
but never verified that every adaptation matched anything, silently
ignoring rules Zend rejects while binding traits:

  - `use A { missing as g; }` — "An alias (g) was defined for method
    missing(), but this method does not exist";
  - `use A { A::missing as g; }` — "An alias was defined for
    A::missing but this method does not exist";
  - an alias or precedence rule naming a trait outside the class's use
    list — "Required Trait B wasn't added to C";
  - `use A, B { B::f insteadof A; }` with no B::f — "A precedence
    rule was defined for B::f but this method does not exist" (the
    OVERRIDDEN trait need not declare the method — only the preferred
    one, matching Zend).

Composition now records which "trait::method" keys were seen (methods
arriving from nested traits are keyed under the directly-used trait,
matching how adaptations are registered) and validates every adaptation
afterwards. Because the Preprocessor registers an unqualified alias
under EVERY used trait's key, entries now carry the source adaptation's
group id — a group is satisfied when any variant matched — plus the
method name and explicit qualifier for diagnostics; precedence entries
record the rule for winner-existence validation (consumers only isset()
the key, so the value change is compatible).
…ethod

traitIgnored kept ONE rule per loser Trait::method key, so a later
`C::f insteadof B` overwrote an earlier `A::f insteadof B` and skipped
its validation: Zend fatals with 'A precedence rule was defined for A::f
but this method does not exist' (each rule's winner must exist on its
own), while the overwriting rule let the program compile.

Store a LIST of rule records behind the ignored-method key - consumers
only isset() the key, so the ignore behavior is unchanged - append per
key across several `use` clauses (array_merge dropped earlier clauses'
rules for the same loser), and validate every record.

Keeping all records also exposes Zend's exclusion-duplication rule: a
trait method may be excluded only once, even by rules whose winners all
exist ('Failed to evaluate a trait precedence (f). Method of trait B was
defined to be excluded multiple times', probed on 8.4.13 within one and
across several use clauses). Reported after the existence checks, in
Zend's order. One winner excluding two different losers stays legal.
The trait-member value comparison evaluated an enum case fetch to its
case name (or backing scalar), so `const X = E1::Value` and
`const X = E2::Value` in two traits looked identical and the class
composed. Zend compares the resolved zvals, and every enum case is a
distinct object: same-named cases of different enums, different cases of
one enum, and cases of different backed enums sharing a backing scalar
are all incompatible definitions (probed on 8.4.13); only the SAME
enum's same case is compatible.

Evaluation for this comparison now maps an enum case to a marker string
carrying its (enum class, case name) identity, NUL-prefixed so it cannot
collide with a real string value, and working inside array initializers
too. The general constant-folding path is untouched: only
evaluateTraitMemberValue() asks for identity semantics.

isCompatibleTraitConstant() also returned early when the two lowered
value strings matched - every non-literal initializer shares '' there,
which bypassed the value comparison entirely. The evaluated comparison
is now authoritative whenever both initializer expressions are known;
the string equality remains the fallback.
The trait-member value comparison evaluates constant initializers against
a class name that is already fully qualified (the composing class or the
defining trait). getClassConstValue() treated it as relative and
prepended the current file's namespace again, so evaluating
`[...self::PARTS, 9]` while converting a namespaced class looked up
`App\App\C::PARTS` and aborted the compile — the compiler's own
self-build died on `TypePhp\TypePhp\CompilerBase::PYTHON_CONSTRUCTOR_CLASSES`.
Mark the resolved self/parent/static target absolute before the lookup.
The absolute-name guard compared the raw AST spelling with the resolved
name, but Name::toString() strips the leading backslash of a
`\App3\C::NAME` fetch, so the two matched and the name was resolved as
relative again — tests/compiler/trait/010.phpt failed with
`App3\App3\TraitsTest` not found. A Name\FullyQualified class node is
absolute regardless of the string comparison.
…ruct

The identity a trait-member comparison assigned to an enum case was a
NUL-prefixed marker STRING. PHP strings are binary-safe, so user code
can spell that exact byte sequence:

    trait T1 { const X = E::A; }
    trait T2 { const X = "\0enum-case\0e::A"; }

Zend rejects composing the two (an enum-case object and a string are
different values); both definitions evaluated to the same marker here,
so the collision was accepted (probed on 8.4.13).

Enum cases now evaluate, under identity semantics, to an interned
EnumCaseIdentity object - one instance per (enum class, case name)
pair, private constructor. ConstExprEvaluator can only produce scalars,
arrays and null from user constant expressions, never an object, so no
user value can collide with it, and interning keeps === a stable
identity test across repeated references - recursively inside arrays
too, since array === compares each element with === again. The float
coercion in isSameTraitMemberValue() only touches is_int() values and
ignores the objects.

Regression tests: the marker-string collision above must conflict,
arrays holding the same enum case still compose, and arrays holding
same-named cases of different enums conflict (each probed on 8.4.13).
@AlessioGiacobbe
AlessioGiacobbe force-pushed the split/trait-composition-checks branch from bb781a5 to 9b84928 Compare September 2, 2026 13:17
@AlessioGiacobbe

Copy link
Copy Markdown
Contributor Author

Rebased onto current master. The identity marker is no longer a string:

  • Enum cases now evaluate (under identity semantics) to an interned compiler-internal object — EnumCaseIdentity::intern(enumClass, caseName), one singleton per (lowercased FQ enum, case-sensitive case name) pair. ConstExprEvaluator can only produce scalars, arrays and null from user constant expressions, so no user-spellable value can ever be === to an instance; interning makes === a stable identity for repeated references, and PHP's === on arrays compares elements with === again, so the distinction nests recursively.
  • ENUM_CASE_IDENTITY_PREFIX and the NUL-marker code are gone.

Tests added (each first verified against Zend 8.4.13): the exact collision program from the review (E::A vs "\0enum-case\0e::A") now conflicts; [E::A, 1] in both traits composes; [E1::Value] vs [E2::Value] conflicts.

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