From e90d6246ec41d91cb4206d67de2f32fa1de40aa4 Mon Sep 17 00:00:00 2001 From: Alessio Giacobbe Date: Wed, 2 Sep 2026 13:50:46 +0200 Subject: [PATCH] fix: enforce interface declaration and merge rules (#59) --skip-tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(preprocessor): enforce Zend interface member declaration rules 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. * fix(translator): validate same-name methods when interfaces merge 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. * fix(translator): reject an interface extending a class, validate merged 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. * fix(translator): guard the interface method table against extends cycles 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. --- .../interface_collision_unimplemented.php | 6 + phpunit/code/interface_collision_valid.php | 17 ++ phpunit/code/interface_extends_cycle.php | 8 + .../interface_multi_extends_incompatible.php | 6 + phpunit/code/interface_rule_abstract.php | 4 + phpunit/code/interface_rule_body.php | 4 + phpunit/code/interface_rule_const_private.php | 4 + phpunit/code/interface_rule_extends_class.php | 5 + .../interface_rule_extends_class_forward.php | 7 + phpunit/code/interface_rule_extends_dup.php | 5 + phpunit/code/interface_rule_final.php | 4 + phpunit/code/interface_rule_private.php | 4 + phpunit/code/interface_rule_prop_abstract.php | 4 + phpunit/src/InterfaceDeclarationRulesTest.php | 62 +++++++ phpunit/src/InterfaceMethodCollisionTest.php | 31 ++++ src/Preprocessor.php | 41 +++++ src/Translator.php | 156 +++++++++++++++++- 17 files changed, 366 insertions(+), 2 deletions(-) create mode 100644 phpunit/code/interface_collision_unimplemented.php create mode 100644 phpunit/code/interface_collision_valid.php create mode 100644 phpunit/code/interface_extends_cycle.php create mode 100644 phpunit/code/interface_multi_extends_incompatible.php create mode 100644 phpunit/code/interface_rule_abstract.php create mode 100644 phpunit/code/interface_rule_body.php create mode 100644 phpunit/code/interface_rule_const_private.php create mode 100644 phpunit/code/interface_rule_extends_class.php create mode 100644 phpunit/code/interface_rule_extends_class_forward.php create mode 100644 phpunit/code/interface_rule_extends_dup.php create mode 100644 phpunit/code/interface_rule_final.php create mode 100644 phpunit/code/interface_rule_private.php create mode 100644 phpunit/code/interface_rule_prop_abstract.php create mode 100644 phpunit/src/InterfaceDeclarationRulesTest.php create mode 100644 phpunit/src/InterfaceMethodCollisionTest.php diff --git a/phpunit/code/interface_collision_unimplemented.php b/phpunit/code/interface_collision_unimplemented.php new file mode 100644 index 00000000..b5b055a6 --- /dev/null +++ b/phpunit/code/interface_collision_unimplemented.php @@ -0,0 +1,6 @@ +exec('Interface function `Runner::run()` cannot contain body', 'interface_rule_body.php'); + } + + public function testInterfaceMethodMustNotBeFinal(): void + { + $this->exec('Interface method `Runner::run()` must not be final', 'interface_rule_final.php'); + } + + public function testInterfaceMethodMustBePublic(): void + { + $this->exec('Access type for interface method `Runner::run()` must be public', 'interface_rule_private.php'); + } + + public function testInterfaceMethodMustNotBeAbstract(): void + { + $this->exec('Interface method `Runner::run()` must not be abstract', 'interface_rule_abstract.php'); + } + + public function testInterfaceConstantMustBePublic(): void + { + $this->exec('Access type for interface constant `Runner::SPEED` must be public', 'interface_rule_const_private.php'); + } + + public function testInterfaceCannotExtendClass(): void + { + $this->exec('`Runner` cannot implement `Base` - it is not an interface', 'interface_rule_extends_class.php'); + } + + public function testExtendsClassDeclaredLaterIsRejected(): void + { + $this->exec('`Late` cannot implement `Impl` - it is not an interface', 'interface_rule_extends_class_forward.php'); + } + + public function testInterfaceCannotExtendSameInterfaceTwice(): void + { + $this->exec('Interface `Runner` cannot implement previously implemented interface `A`', 'interface_rule_extends_dup.php'); + } + + public function testInterfacePropertyCannotBeExplicitlyAbstract(): void + { + $this->exec('Property in interface cannot be explicitly abstract', 'interface_rule_prop_abstract.php'); + } + + public function testCyclicExtendsGraphIsRejectedPromptly(): void + { + // Zend cannot even declare such a graph (`Interface "B" not found`); + // ahead-of-time the cycle exists, so the merged-member table builders + // must detect it instead of recursing forever. + $this->exec('Interface inheritance cycle detected at `B`', 'interface_extends_cycle.php'); + } +} diff --git a/phpunit/src/InterfaceMethodCollisionTest.php b/phpunit/src/InterfaceMethodCollisionTest.php new file mode 100644 index 00000000..14a3d9d3 --- /dev/null +++ b/phpunit/src/InterfaceMethodCollisionTest.php @@ -0,0 +1,31 @@ +exec( + 'Declaration of `I1::f()` must be compatible with `I2::f()`', + 'interface_multi_extends_incompatible.php' + ); + } + + public function testUnimplementedCollisionOnClassIsRejected(): void + { + $this->exec( + 'Declaration of `I1::f()` must be compatible with `I2::f()`', + 'interface_collision_unimplemented.php' + ); + } + + public function testCompatibleAndSatisfiedCollisionsAreAccepted(): void + { + $this->compile('interface_collision_valid.php'); + } +} diff --git a/src/Preprocessor.php b/src/Preprocessor.php index 5bd3ecdc..408a56ad 100644 --- a/src/Preprocessor.php +++ b/src/Preprocessor.php @@ -2400,8 +2400,23 @@ class Preprocessor extends CompilerBase $interfaceName = $this->interfaceDef->getNamespacedName(false); $interfaceNameLower = strtolower($interfaceName); + $extendedInterfaces = []; foreach ($v->extends as $parent) { $parentName = $this->getNamespacedClassName($this->parseIdentifier($parent)); + // An interface may only extend interfaces. The parent's kind is + // only known once its declaration has been prepared; a parent + // declared later is validated by the Translator instead. + if ($this->hasClass($parentName) || $this->isInternalClass($parentName)) { + $this->fatalError($parent, "`{$interfaceName}` cannot implement `{$parentName}` - it is not an interface"); + } + $parentNameLower = strtolower($parentName); + if (isset($extendedInterfaces[$parentNameLower])) { + $this->fatalError( + $parent, + "Interface `{$interfaceName}` cannot implement previously implemented interface `{$parentName}`", + ); + } + $extendedInterfaces[$parentNameLower] = true; $this->interfaceDef->extendsList[] = $parentName; if ($this->interfaceDef->extends === '') { $this->interfaceDef->extends = $parentName; @@ -2423,6 +2438,12 @@ class Preprocessor extends CompilerBase if ($stmt instanceof Node\Stmt\ClassConst) { foreach ($stmt->consts as $const) { $constName = $this->parseIdentifier($const->name); + if ($stmt->flags & (Modifiers::PRIVATE | Modifiers::PROTECTED)) { + $this->fatalError( + $stmt, + "Access type for interface constant `{$interfaceName}::{$constName}` must be public", + ); + } if ($this->interfaceDef->hasConstant($constName)) { $this->fatalError($stmt, "Duplicate constant `{$constName}`"); } @@ -2445,6 +2466,20 @@ class Preprocessor extends CompilerBase if ($stmt instanceof Node\Stmt\ClassMethod) { $methodName = $this->getMethodName($stmt); $this->assertKeywordMethodMayBeDeclared($stmt, $methodName, false); + // Interface methods are implicitly public and abstract; Zend + // rejects the modifiers below in this exact precedence order. + if ($stmt->flags & (Modifiers::PRIVATE | Modifiers::PROTECTED)) { + $this->fatalError($stmt, "Access type for interface method `{$interfaceName}::{$methodName}()` must be public"); + } + if ($stmt->flags & Modifiers::ABSTRACT) { + $this->fatalError($stmt, "Interface method `{$interfaceName}::{$methodName}()` must not be abstract"); + } + if ($stmt->flags & Modifiers::FINAL) { + $this->fatalError($stmt, "Interface method `{$interfaceName}::{$methodName}()` must not be final"); + } + if ($stmt->stmts !== null) { + $this->fatalError($stmt, "Interface function `{$interfaceName}::{$methodName}()` cannot contain body"); + } if ($this->interfaceDef->hasMethod($methodName)) { $this->fatalError($stmt, "Duplicate method `{$methodName}`"); } @@ -2490,6 +2525,12 @@ class Preprocessor extends CompilerBase if ($property->hooks === []) { $this->fatalError($property, 'Interfaces may only include hooked properties'); } + if ($property->flags & Modifiers::ABSTRACT) { + $this->fatalError( + $property, + 'Property in interface cannot be explicitly abstract. All interface members are implicitly abstract', + ); + } if ($property->flags & (Modifiers::PRIVATE | Modifiers::PROTECTED)) { $this->fatalError($property, 'Property in interface cannot be protected or private'); } diff --git a/src/Translator.php b/src/Translator.php index dedd87fb..5b5bd0a0 100644 --- a/src/Translator.php +++ b/src/Translator.php @@ -2909,6 +2909,7 @@ CODE; } elseif ($v instanceof Node\Stmt\Interface_) { $this->validateInterfaceOverrideAttributes($v); $this->validateInterfaceConstants($v); + $this->validateInterfaceMethodCompatibility($v); } elseif (!$v instanceof Node\Stmt\Nop) { $this->unsupportedSyntax($v); } @@ -3084,6 +3085,7 @@ CODE; } elseif ($v2 instanceof Node\Stmt\Interface_) { $this->validateInterfaceOverrideAttributes($v2); $this->validateInterfaceConstants($v2); + $this->validateInterfaceMethodCompatibility($v2); } elseif (!$v2 instanceof Node\Stmt\Nop) { $this->unsupportedSyntax($v2); } @@ -4203,6 +4205,7 @@ CODE; $this->validateOverrideAttributes($class); $this->checkInterfaceImplementations($class); $this->checkInheritedConstantContracts($class); + $this->checkInterfaceMethodCollisions($class); $this->checkInheritedAbstractMethodsAreImplemented($class); } $code = $this->genNativeMethod($methodCodes); @@ -4762,9 +4765,10 @@ CODE; string $methodName, MethodDef $childMethodDef, MethodDef $parentMethodDef, - string $parentClass + string $parentClass, + ?string $childClass = null ): void { - $className = $this->getFullClassName(); + $className = $childClass ?? $this->getFullClassName(); // PHP allows widening visibility in overrides (e.g. protected -> public), // but forbids narrowing it. @@ -6111,6 +6115,154 @@ CODE; } } + /** + * Memoized effective method tables of interfaces (method name => def and + * its original declaring interface). + * + * @var array> + */ + private array $effectiveInterfaceMethodTables = []; + + /** @var array Interface method tables being constructed. */ + private array $effectiveInterfaceMethodTableVisiting = []; + + /** @return array */ + private function getEffectiveInterfaceMethodTable(InterfaceDef $def, NodeAbstract $errorNode): array + { + $ownName = $def->getNamespacedName(false); + $key = strtolower($ownName); + if (isset($this->effectiveInterfaceMethodTables[$key])) { + return $this->effectiveInterfaceMethodTables[$key]; + } + if (isset($this->effectiveInterfaceMethodTableVisiting[$key])) { + // A cyclic extends graph would recurse forever; fail promptly with + // the same diagnostic getEffectiveConstantTable() uses, so the + // helper stays safe regardless of which validation runs first. + $this->fatalError($errorNode, "Interface inheritance cycle detected at `{$ownName}`"); + } + + $this->effectiveInterfaceMethodTableVisiting[$key] = true; + try { + $table = []; + foreach ($def->methods as $name => $methodDef) { + $table[$name] = ['def' => $methodDef, 'origin' => $ownName]; + } + foreach ($def->extendsList ?: ($def->extends ? [$def->extends] : []) as $parentName) { + if (!$this->hasInterface($parentName)) { + continue; + } + foreach ($this->getEffectiveInterfaceMethodTable($this->getInterface($parentName), $errorNode) as $name => $entry) { + $table[$name] ??= $entry; + } + } + return $this->effectiveInterfaceMethodTables[$key] = $table; + } finally { + unset($this->effectiveInterfaceMethodTableVisiting[$key]); + } + } + + /** + * Zend's interface merge validates the FIRST-seen declaration of a method + * as an override of every LATER same-name declaration ("Declaration of + * I1::f() must be compatible with I2::f()"): the interface's own method, + * or the one inherited from the earliest-listed parent, is the child. + */ + private function validateInterfaceMethodCompatibility(Node\Stmt\Interface_ $interfaceStmt): void + { + $name = $this->parseIdentifier($interfaceStmt->name); + $interfaceName = $this->namespace === '' ? $name : $this->namespace . '\\' . $name; + if (!$this->hasInterface($interfaceName)) { + return; + } + $interfaceDef = $this->getInterface($interfaceName); + + // An interface can only extend other interfaces; naming a class here + // is a Zend fatal, not a lookup failure. + foreach ($interfaceDef->extendsList ?: ($interfaceDef->extends ? [$interfaceDef->extends] : []) as $parentName) { + if (!$this->hasInterface($parentName) && !$this->isInternalInterface($parentName) && $this->hasClass($parentName)) { + $this->fatalError($interfaceStmt, + "`{$interfaceName}` cannot implement `{$parentName}` - it is not an interface"); + } + } + + $table = []; + foreach ($interfaceDef->methods as $methodName => $methodDef) { + $table[$methodName] = ['def' => $methodDef, 'origin' => $interfaceName]; + } + foreach ($interfaceDef->extendsList ?: ($interfaceDef->extends ? [$interfaceDef->extends] : []) as $parentName) { + if (!$this->hasInterface($parentName)) { + continue; + } + foreach ($this->getEffectiveInterfaceMethodTable($this->getInterface($parentName), $interfaceStmt) as $methodName => $entry) { + if (!isset($table[$methodName])) { + $table[$methodName] = $entry; + continue; + } + $existing = $table[$methodName]; + if ($existing['origin'] === $entry['origin']) { + continue; // diamond: same original declaration + } + $this->validateMethodOverrideSignature( + $interfaceStmt, + $existing['def']->name, + $existing['def'], + $entry['def'], + $entry['origin'], + $existing['origin'], + ); + } + } + } + + /** + * When a class(-like) implements several interfaces declaring the same + * method and neither the class nor a userland ancestor defines it, Zend + * still validates the interface declarations against each other + * (first-seen as the child). A defined method silences this pairwise + * check — it is instead validated against every interface individually. + */ + private function checkInterfaceMethodCollisions(Node\Stmt\Class_|Node\Stmt\Enum_ $classStmt): void + { + $classDef = $this->classDef; + $definedInChain = function (string $methodName) use ($classDef): bool { + $current = $classDef; + while (true) { + if ($current->hasMethod($methodName) || $current->hasAbstractMethod($methodName)) { + return true; + } + if ($current->extends === '' || $current->inheritedFromInternalClass || !$this->hasClass($current->extends)) { + return false; + } + $current = $this->getClass($current->extends); + } + }; + + $table = []; + foreach ($this->getClassImplementedInterfaces($classDef) as $interfaceName) { + if (!$this->hasInterface($interfaceName)) { + continue; + } + foreach ($this->getEffectiveInterfaceMethodTable($this->getInterface($interfaceName), $classStmt) as $methodName => $entry) { + if (!isset($table[$methodName])) { + $table[$methodName] = $entry; + continue; + } + $existing = $table[$methodName]; + if ($existing['origin'] === $entry['origin'] || $definedInChain($methodName)) { + continue; + } + $this->validateMethodOverrideSignature( + $classStmt, + $existing['def']->name, + $existing['def'], + $entry['def'], + $entry['origin'], + $existing['origin'], + ); + } + } + } + private function checkConstantOverride(Node\Stmt\Class_|Node\Stmt\Trait_|Node\Stmt\Enum_ $classStmt): void { $classDef = $this->classDef;