Repository navigation
Conversation
…erited method satisfies
… it satisfies Skipping a trait's abstract method satisfied by an inherited implementation also skipped the signature check, so an incompatible parent method was silently accepted where PHP reports 'Declaration of P::foo() must be compatible with T::foo()'. The inherited signature is now validated with the rules enums already apply to trait requirements: parameters may only widen and a declared return type must be honoured.
phcorp
marked this pull request as ready for review
November 8, 2026 20:08
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Trait flattening merges a trait's abstract method into the class like any other member. The class schema therefore read
abstract function invalidate()as if the class itself declared it. As a result, three valid PHP forms failed when an inherited method satisfied the requirement:Cannot override final method Widget::invalidatewhen the inherited method isfinal.Cannot make concrete method abstract: Panel::invalidatewhen it is not final.Panel::rowPath() has #[\Override] attribute, but no matching parent method was found. PHP 8.3+ accepts#[\Override]on a method that implements a used trait's abstract method.With this PR, a used trait's abstract method that an inherited implementation already satisfies adds nothing to the schema. The inherited signature is still checked against the requirement, as PHP does.
#[\Override]also finds its target in a trait requirement.Repro
error: Cannot override final method Widget::invalidateinv:made:r2inv:made:r2An incompatible inherited method is still rejected:
Cannot override method C::foo with incompatible …1): skipping the requirement also skipped the signature checkCannot narrow parameter $x when implementing trait method: C::fooDeclaration of P::foo(string $x): int must be compatible with T::foo(int $x): stringHow it works
flowchart TD F["flatten traits<br/>(abstract trait methods merged into class)"] --> COL["collect_trait_method_requirements<br/>required = abstract keys of used traits (nested)<br/>declared = keys written in the class body"] COL --> M{"apply method:<br/>abstract, from a trait (required ∖ declared),<br/>and an inherited concrete implementation<br/>of the same kind exists?"} M -- no --> N["normal path: final / kind / visibility /<br/>override-signature / re-abstraction checks"] M -- yes --> V["validate_inherited_implementation<br/>(params may only widen, declared return honoured)"] V -- ok --> SKIP["skip: requirement adds nothing"] V -- incompatible --> ERR[error] N --> O{"#[\Override] with no parent/interface target?"} O -- "key is a trait requirement" --> OK[accepted] O -- otherwise --> ERR2["no matching parent method"]collect_trait_method_requirementsruns right after flattening, next tocollect_trait_default_origins. For each trait-using class it records which method keys the used traits require (through nesteduse) and which keys the class body declares. The table is stored asChecker::trait_method_requirements.inherited_method_satisfies_requirementis called at the top ofapply_instance_methodandapply_static_methodwith the inherited signature of the same kind. It applies only when that kind has a concrete implementation (method_impl_classes/static_method_impl_classes). A static/instance mismatch therefore still takes the normal path and reportsCannot change method kind.validate_inherited_implementationreusesvalidate_abstract_trait_signature, the helper enums already use for trait requirements, and adds a declared-return check. Late-static (static) requirement returns are exempt.satisfies_trait_requirementlets#[\Override]target a trait requirement, alongside parent and interface methods.abstractitself (declaredcontains the key), it is still an error, as in PHP.Tests
test_trait_requirements_satisfied_by_inherited_methods_and_override(tests/codegen/oop/traits.rs#L189): a final instance method, a static method, and a protected requirement implemented with#[\Override], all reached through a nested traituse. It printsinv:made:r2, as php 8.5 does (ELEPHC_PHP_CHECK=1). It fails on main.test_error_trait_using_class_cannot_make_inherited_method_abstract(#L78): a trait-using class may still not re-declare an inherited concrete method abstract.test_error_inherited_method_must_match_trait_requirement_signature(#L92): an inherited instance method that narrows a parameter, and an inherited static method with an incompatible return type, are both rejected. It fails on the first commit of this PR.test_error_override_in_trait_using_class_requires_a_target(#L114):#[\Override]still needs some target.Probed by hand against php 8.5 (all match): a requirement satisfied through a grandparent, in a namespace, in braced namespaces across
use Lib\Rows, withuse Rows { inv as protected; }, and with a widened inherited signature (foo(mixed $x = null, int $y = 2)forfoo(int $x), plus astaticreturn). Instance/static mismatches with the parent are rejected (Cannot change method kind).Runs (local; fork CI does not run):
ELEPHC_PHP_CHECK=1 cargo test --test codegen_tests trait: 101 passedcargo test --test codegen_tests override: 117 passed, 1 ignoredcargo test --test error_tests trait: 116 passed.override: 25 passed.abstract: 23 passed.cargo build: no new warnings (thefetch_updatewarning in elephc-magician is already on main).git diff --checkis clean.Note
CI status: this PR comes from a fork, so the
CI,PDO Live DatabasesandPHP extension host SDKworkflows are waiting for maintainer approval (action_required). The focused tests above ran locally on macOS arm64, and the codegen tests were also compared with PHP 8.5.12 (ELEPHC_PHP_CHECK=1). This is a front-end change (parser/checker only) with no target-specific code.Limits
class P { private function foo() {} } class C extends P { use T; }(withTrequiringfoo) is rejected withConcrete class C must implement abstract method C::foo. php 8.5 compiles it and only fails when the method is called (Call to private method P::foo() from scope C). elephc is stricter than php here, which is acceptable for an ahead-of-time compiler.(name, is_static), and it ignoresinsteadofexclusions. That only matters for#[\Override]acceptance in contrived cases, because mismatched kinds are rejected elsewhere.src/types/checker/schema/classes/methods.rsgrows from 484 to 538 lines. It stays one cohesive leaf (method schema rules), and the new logic lives intrait_requirements.rs.Follow-ups
fix(traits): an abstract trait method is a requirement, never a conflict with another trait's method): the two PRs complement each other and do not duplicate work. fix(traits): an abstract trait method is a requirement, never a conflict with another trait's method #2289 resolves requirement vs. another trait's method at flattening. This PR resolves requirement vs. inherited method in the class schema. They share no source file. Merged together on a scratch branch, they compile and pass the focused tests. A class that needs both (class Panel extends Widget { use Rows; use Hover; use Gives; }, where two traits require the inheritedinvalidate()and one provideslabel()) printsrinvLhLlike php. Each PR alone rejects it. Either merge order works, but the second PR gets textual conflicts intests/codegen/oop/traits.rs(both append a test aftertest_abstract_trait_property_can_be_satisfied_by_concrete_child) andtests/error_tests/classes_traits.rs(both insert aftertest_error_trait_method_conflict_requires_insteadof). Resolve by keeping both sides.fix(parser): accept semi-reserved keywords as method names in trait adaptations): no logic overlap. fix(parser): accept semi-reserved keywords as method names in trait adaptations #2286 also appends a test at the end oftests/codegen/oop/traits.rs, so whichever merges later gets a keep-both conflict there.validate_inherited_implementationinto a general class-level check of trait requirements against whatever implements them (the class body or another trait), asenum_traits::validate_requirementdoes for enums.