fix(ir_lower): resolve a constructor signature through its owner class - #990
Merged
Merged
Conversation
Closes #868. A private method is not inherited, so `ClassInfo::methods` deliberately carries no `__construct` entry on a descendant of a class with a private constructor; `types::constructor_owner` exists to walk to the ancestor that still owns it, and #796 routed the checker and `new` codegen through it. Two IR-lowering sites kept reading `class_info.methods.get("__construct")` directly, so such a descendant looked like it had NO constructor at all: - `object_construction::constructor_signature` — with no signature, omitted defaults were never padded and `fixed_new` then rejected the arity: `new Child()` on `private function __construct(int $n = 1)` failed with `constructor call to Child::__construct with 0 args for 1 params`. - `reflection_new_instance::constructor_signature_for_class_name` — `ReflectionClass::newInstance()` on the descendant saw none either. Both now resolve through `constructor_owner`. An inherited public or protected constructor is unaffected: the walk stops at the instantiated class itself whenever its own map has the entry, and a descendant that REPLACES the constructor keeps its own for the same reason. Tests pin the padding on the named path, an explicit argument winning over a default, the reflection half, and both unchanged shapes (inherited public constructor, descendant-declared constructor). The `new static()` form in the issue's reproduction is blocked earlier by #797, which rejects a `new static()` whose constructor has omitted defaults even for a PUBLIC constructor with no inheritance involved; that path is out of scope here exactly as the issue states. Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
Greptile SummaryResolves constructor signatures through
Confidence Score: 5/5The PR appears safe to merge, with both previous findings addressed and no new blocking findings. The private-ancestor reflection fixture now distinguishes owner lookup from direct lookup, and the public-inheritance docblock correctly states that traversal stops when the constructor entry exists. The stopping-condition thread was resolved without an explanatory reply; the code confirms its correction. There are no file changes since the previous review.
|
| Filename | Overview |
|---|---|
| src/ir_lower/expr/object_construction.rs | Resolves fixed-class constructor metadata through the owner lookup so inherited private-constructor defaults can be padded. |
| src/ir_lower/expr/reflection_new_instance.rs | Uses the same owner lookup for known-class reflection constructor signatures. |
| tests/codegen/oop/relative_types.rs | Adds six regression and control tests, including the previously missing reflection fallback regression, and corrects the stopping-condition documentation. |
Reviews (4): Last reviewed commit: "test(oop): add the reflection regression..." | Re-trigger Greptile
…ion test's claim Review follow-up on #990. The public-constructor docblock stated the owner walk stops at the instantiated class when its own map LACKS the entry, which is the opposite of what `constructor_owner` does and of what the test verifies. The reflection fixture was also labelled a regression test it cannot be: a public inherited constructor is already copied into the descendant's `methods` map, so the previous direct lookup supplies its signature too. The two lookups differ only for a class whose own map lacks `__construct` — the inherited-PRIVATE-constructor shape — and PHP rejects `ReflectionClass::newInstance()` on a non-public constructor outright, so no valid program reaches it. The fixture is now labelled the control it is, and says why the site was changed anyway. Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
Guikingone
added a commit
that referenced
this pull request
Sep 13, 2026
…ion test's claim Review follow-up on #990. The public-constructor docblock stated the owner walk stops at the instantiated class when its own map LACKS the entry, which is the opposite of what `constructor_owner` does and of what the test verifies. The reflection fixture was also labelled a regression test it cannot be: a public inherited constructor is already copied into the descendant's `methods` map, so the previous direct lookup supplies its signature too. The two lookups differ only for a class whose own map lacks `__construct` — the inherited-PRIVATE-constructor shape — and PHP rejects `ReflectionClass::newInstance()` on a non-public constructor outright, so no valid program reaches it. The fixture is now labelled the control it is, and says why the site was changed anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
Review follow-up on #990. The reviewer was right that the reflection fixture could not catch a regression: an inherited PUBLIC constructor IS copied into the descendant's own `methods` map, so the previous direct lookup supplied its signature too. My reply then claimed no valid shape could tell the two lookups apart, because php-src rejects `ReflectionClass::newInstance()` on a non-public constructor. That was wrong about ELEPHC: it does not yet enforce constructor visibility there, so the private-ancestor shape is reachable, and the lookup difference is plainly observable. Measured both ways on the new fixture: - with the owner walk: `6` — the default is padded and the ancestor's constructor runs; - reading the descendant's own map: `0` — no signature, so no padding and no constructor call at all, leaving the promoted property at its zero value. Added as `test_reflection_new_instance_resolves_an_inherited_private_constructor`, verified to FAIL on the unpatched lowering. Its docblock records that the shape depends on elephc's separate `newInstance()` visibility gap, and that closing that gap should turn this into a rejection test rather than delete it — the lookup it pins is what decides which class's constructor the visibility check would then be asked about. The public-inheritance fixture stays as the control it is, relabelled accordingly. Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
Guikingone
added a commit
that referenced
this pull request
Sep 13, 2026
Review follow-up on #990. The reviewer was right that the reflection fixture could not catch a regression: an inherited PUBLIC constructor IS copied into the descendant's own `methods` map, so the previous direct lookup supplied its signature too. My reply then claimed no valid shape could tell the two lookups apart, because php-src rejects `ReflectionClass::newInstance()` on a non-public constructor. That was wrong about ELEPHC: it does not yet enforce constructor visibility there, so the private-ancestor shape is reachable, and the lookup difference is plainly observable. Measured both ways on the new fixture: - with the owner walk: `6` — the default is padded and the ancestor's constructor runs; - reading the descendant's own map: `0` — no signature, so no padding and no constructor call at all, leaving the promoted property at its zero value. Added as `test_reflection_new_instance_resolves_an_inherited_private_constructor`, verified to FAIL on the unpatched lowering. Its docblock records that the shape depends on elephc's separate `newInstance()` visibility gap, and that closing that gap should turn this into a rejection test rather than delete it — the lookup it pins is what decides which class's constructor the visibility check would then be asked about. The public-inheritance fixture stays as the control it is, relabelled accordingly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
Guikingone
force-pushed
the
fix/868-inherited-private-ctor-defaults
branch
from
September 13, 2026 19:39
bb1091d to
744dff6
Compare
Guikingone
added a commit
that referenced
this pull request
Sep 13, 2026
Closes #871. #796 introduced `types::constructor_owner` — the walk that finds the class whose `__construct` PHP runs when a descendant of a class with a non-public constructor is instantiated — and the existing coverage exercised only the 0-argument private-ancestor success path. The listed gaps are behaviours that are already CORRECT on main; what was missing is the pins that keep them so. A new `tests/codegen/oop/constructor_owner.rs` groups the member-visibility half, where private and protected deliberately diverge: - a PROTECTED ancestor constructor IS inherited: it runs through the descendant, and `method_exists()` is `true` on both classes; - it is still not public, so the descendant is not instantiable and the reflected declaring class is the ancestor; - a PRIVATE ancestor constructor is NOT inherited: `method_exists()` is `false` on the descendant while staying `true` on the declaring class — the half that makes the owner walk necessary at all; - `get_class_methods()` agrees, with an inherited public method alongside so the assertion is a contrast rather than a tautology (the function is scope-aware, so a private constructor is omitted from its own declaring class too); - a descendant's own PUBLIC constructor replaces a private ancestor's. Two error tests cover the hiding direction, which the walk must NOT take: a descendant that declares its own PRIVATE constructor hides an inherited public one, so `new static()` from the ancestor's scope and a named `new Child()` both name the DESCENDANT's constructor, exactly as php-src does (`Call to private HideChild::__construct() from scope HideOwner`). Every fixture's output was diffed against reference PHP 8.5 and matches. The remaining items on the issue — omitted defaults on the owner path, a descendant replacing the constructor, and `method_exists`/`getMethods` staying false — are covered by the tests in PRs #990 and #991. Claude-Session: https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr
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.
Closes #868.
Cause
A private method is not inherited, so
ClassInfo::methodsdeliberately carries no__constructentry on a descendant of a class with a private constructor.types::constructor_ownerexists to walk to the ancestor that still owns it, and #796 routed the checker andnewcodegen through it.Two IR-lowering sites — both named in the issue — kept reading
class_info.methods.get("__construct")directly, so such a descendant looked like it had no constructor at all:object_construction::constructor_signaturefixed_newrejects the arity:constructor call to Child::__construct with 0 args for 1 paramsreflection_new_instance::constructor_signature_for_class_nameReflectionClass::newInstance()pads nothing and runs no constructor, leaving promoted properties at their zero valuesFix
Both sites resolve through
constructor_owner. An inherited public or protected constructor is unaffected: the walk stops at the instantiated class itself whenever its own map has the entry, and a descendant that replaces the constructor keeps its own for the same reason.Tests
Six tests in
tests/codegen/oop/relative_types.rs, next to the existingconstructor_ownercoverage:test_reflection_new_instance_resolves_an_inherited_private_constructor: measured6with the owner walk,0without it, and verified to fail on the unpatched loweringThe reflection regression fixture depends on a separate elephc gap:
newInstance()does not yet enforce constructor visibility, where php-src raisesError: Call to private Owner::__construct(). Its docblock says so, and says that closing that gap should turn the fixture into a rejection test rather than delete it — the lookup it pins is what decides which class's constructor the visibility check would then be asked about.--test codegen_tests oop(810),relative_types(26),--test error_tests(1518) andcargo test --libpass;cargo buildis warning-free. One unrelated flake,elephc-pcntl'spriority_failure_warnings_name_the_operation_and_errno, fails only under the parallel suite and passes in isolation — it lives in another crate that this change cannot reach.Scope
The
new static()form in the issue's reproduction is blocked earlier by #797, which rejects anew static()whose constructor has omitted defaults even for a public constructor with no inheritance at all:That path is out of scope here exactly as the issue states ("Not #797 … This is the named/
fixed_newowner path after #796"). The dynamic thunk builder inir_lower::function::lower_dynamic_constructor_thunkhas the samemethodslookup and will need the same treatment when #797 lands; it is left alone here because no shape can exercise it while #797 stands.🤖 Generated with Claude Code
https://claude.ai/code/session_01KSAAWPyNBq6dP2b5puN3wr