fix: enforce interface declaration and merge rules - #59
Conversation
matyhtf
left a comment
matyhtf
left a comment
There was a problem hiding this comment.
getEffectiveInterfaceMethodTable() recursively memoizes only after the parent table is complete, but it has no in-progress/cycle guard. A cyclic interface graph therefore recurses indefinitely instead of producing a compiler diagnostic:
interface A extends B {}
interface B extends A {}
function main(): void {}Please add a visiting set (cleared with finally) or validate the inheritance graph before recursive table construction, and add a negative test proving this input fails promptly with a stable fatal diagnostic.
PR #58 now adds cycle protection to the analogous effective constant table. Please rebase on current master, but do not rely only on the constant-validation call order: this method-table helper should remain safe independently.
parseInterface accepted several declarations Zend rejects at compile
time (all wordings probed on 8.4.13, which renamed the modifier errors
to "must not be abstract/final"):
- interface method with a body ("Interface function I::f() cannot
contain body")
- private/protected interface method ("Access type for interface
method I::f() must be public")
- explicit `abstract` modifier on an interface method ("Interface
method I::f() must not be abstract")
- `final` interface method ("Interface method I::f() must not be
final")
- private/protected interface constant ("Access type for interface
constant I::X must be public"); `final` interface constants remain
legal per PHP 8.1
- explicit `abstract` on an interface hooked property ("Property in
interface cannot be explicitly abstract...")
- `interface I extends A` where A is a known class, enum, or trait
("I cannot implement A - it is not an interface"); only checked when
A's declaration has already been prepared - a parent declared later
is left to the Translator (deferred to integrator)
- the same interface listed twice in extends ("Interface I cannot
implement previously implemented interface A")
Zend's precedence for combined modifier violations (visibility, then
abstract, then final, then body) is preserved.
Two interfaces declaring the same method were never cross-checked: `interface J extends I1, I2` and a class implementing both compiled even when the declarations were mutually incompatible (Zend: "Declaration of I1::f(): int must be compatible with I2::f(): string"). The first-seen declaration is now validated as an override of every later one, mirroring Zend's merge order; diamond inheritance of one original declaration never conflicts, and a method the class chain defines silences the pairwise check (it is validated against each interface individually instead) — all probed against Zend 8.4.
…ed methods An interface can only extend other interfaces: naming a class either fataled with a misleading missing-symbol message (declaration seen earlier) or compiled silently (declaration appearing later). The translation phase now rejects both with Zend's wording. Same-name methods arriving from several extended interfaces (or from several interfaces a class implements without defining the method) were never cross-checked; the first-seen declaration is now validated as an override of every later one, matching Zend's merge order, with diamond inheritance of one original declaration exempt.
getEffectiveInterfaceMethodTable() recursed forever on a cyclic extends
graph (interface A extends B; interface B extends A). Zend never reaches
this state - declarations are linked one at a time, so the first one
already fails with 'Interface "B" not found' - but ahead-of-time the
whole graph exists before linking, so the cycle must be detected.
Track the tables being built in a visiting set (cleared with
try/finally) and fail promptly with the same stable diagnostic the
constants table uses ('Interface inheritance cycle detected at ...'),
so the helper is safe regardless of which validation pass reaches the
cycle first. Diamond (non-cyclic) graphs still converge through the
memoized table.
Covered by a negative test on the two-interface cycle; the diamond case
is already exercised by interface_collision_valid.php.
3dc8fb6 to
d13b16b
Compare
|
Fixed and rebased on current master (the rebase adopted master's constants machinery from #58 verbatim, including its new cycle guard, and re-layered only this PR's method-table work).
13/13 focused tests green; full suite and sweep failure sets byte-identical to base. |
2 participants
Interface declaration and merge rules were unenforced:
final,private/protected, and explicitabstractmodifiers were accepted; non-public interface constants and explicitly-abstract hooked properties too. All are Zend compile fatals (each probed for the exact rule and message).interface I extends SomeClasseither fataled with a misleading missing-symbol message or compiled when the class was declared later; both cases now fail with Zend's "cannot implement … it is not an interface".interface J extends I1, I2compiled with mutually incompatiblef()declarations. The first-seen declaration is now validated as an override of every later one, matching Zend's merge order; diamond inheritance of one original declaration never conflicts, and a method the class chain defines silences the pairwise check (Zend-probed: aneverreturn satisfying both incompatible declarations compiles).Part of the split of #39.