fix(translator): trait member value comparison and adaptation validation - #60
fix(translator): trait member value comparison and adaptation validation#60AlessioGiacobbe wants to merge 7 commits into
Conversation
matyhtf
left a comment
There was a problem hiding this comment.
Two invalid programs still compile.
traitIgnoredstores a single rule under the loserTrait::methodkey. 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.
- 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.
f2b57d0 to
d453c5e
Compare
|
Both fixed, rebased on current master:
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. |
7dd9185 to
bb781a5
Compare
matyhtf
left a comment
There was a problem hiding this comment.
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).
bb781a5 to
9b84928
Compare
|
Rebased onto current master. The identity marker is no longer a string:
Tests added (each first verified against Zend 8.4.13): the exact collision program from the review ( |
Two trait-composition gaps:
const int X = 1 + 1;andconst int X = 2;(or defaults spelled[1, 2]vsarray(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.use A { missing as g; }anduse A, B { B::f insteadof A; }(with noB::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.