Conversation
Assisted-by: OpenAI Codex (GPT-6)
|
Thanks for the pull request, @xiehuanyi. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 2 advisory finding(s) below merit a look before merge.
Formal verification. PR-changed functions: 1/2 verified (0 proven, 1 may-equivalent, 0 distinguished) · 1 not verified (1 unsupported).
Not verified on this run: \_extract\_generic (unsupported).
Graphify review — findings
Resolves PHP $this->name() calls whose method name collides with a language construct or builtin (list, die, open, …) to the caller's own class method through _self_call_target, matching PHP's case-insensitive method names. The new require_method_owner flag drops the bare-label fallback, so these calls never bind to a free function or another class's method. Without an enclosing class the call stays unresolved, and keyword uses like list($x) = … still produce no edge.
Worth a look
- PHP owner-required self calls can still fall back to an unrelated class method —
graphify/extractors/engine.py:1438· Escalate · high- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- PHP method table is casefolded but non-builtin
$thislookups are not —graphify/extractors/engine.py:7367· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 812 functions depend on the 288 functions this change touches.
Health — this change adds coupling hotspots:
- new:
_extract_generic()— 18 callers, 31 callees - new:
extract_js()— 87 callers, 4 callees - new:
extract_xaml()— 19 callers, 17 callees - new:
extract_objc()— 27 callers, 9 callees - new:
extract_julia()— 19 callers, 7 callees - new:
extract_svelte()— 14 callers, 7 callees - new:
extract_cpp()— 32 callers, 3 callees - new:
extract_vue()— 10 callers, 7 callees - …and 10 more — each is listed as a finding
Verification — 812 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 721 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
30 of 329 test file(s) selected (9%) via static blast radius.
tests/test_astro_extraction.py— impacttests/test_build.py— impacttests/test_cjs_module_extension.py— impacttests/test_cpp_nested_and_cli.py— impacttests/test_dotnet.py— impacttests/test_extract.py— impacttests/test_extract_php_closures.py— impacttests/test_import_extension_resolution.py— impacttests/test_indirect_call_block_scoped_shadow.py— impacttests/test_indirect_dispatch.py— impacttests/test_indirect_dispatch_assign_return.py— impacttests/test_indirect_dispatch_getattr.py— impacttests/test_js_exported_scalar_bindings.py— impacttests/test_languages.py— impacttests/test_multilang.py— impacttests/test_php_language_construct_calls.py— impact, changed-testtests/test_python_underscore_resolution.py— impacttests/test_rationale.py— impacttests/test_ruby_resolution.py— impacttests/test_scala_context_bounds.py— impacttests/test_scala_self_type.py— impacttests/test_scala_top_level_binding.py— impacttests/test_scala_type_definition.py— impacttests/test_svelte_extraction.py— impacttests/test_swift_computed_properties.py— impacttests/test_swift_protocol_requirements.py— impacttests/test_trailing_newline_not_a_syntax_error.py— impacttests/test_ts_new_expression_calls.py— impacttests/test_typescript_module_extensions.py— impacttests/test_vue_extraction.py— impact
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Could not verify: Could not verify \_extract\_generic.
The verifier did not have enough to check \_extract\_generic, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: parameter `config` is annotated `LanguageConfig` — outside the synthesizable primitive/collection set
No difference found (not proven): No behavior difference found in \_self\_call\_target (not a proof).
The verifier ran both versions of \_self\_call\_target on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
· 18 more finding(s) on lines outside this diff (see the check run).
|
Landed in v0.9.77 via an authorship-preserving cherry-pick, so your commit is on |
What does this PR do?
The construct-boundary coverage requested in #4075 exposes a missing real call: PHP
$this->list()is deferred by the shared cross-language builtin-name guard, even whenlistis a method in the current class.Resolve builtin-named methods on a known PHP
$thisreceiver through the existing owning-class method table, with PHP case matching. Require an enclosing method owner; a free or dynamically bound closure must not bind another class's method. Free functions and unknown receivers retain their existing boundaries.Closes #4075.
Related: #2617 handles broader PHP typed-receiver resolution. This change targets the plain
$thisbuiltin-name defer arm, which that diff retains. It does not add the broader typed-receiver feature.Type of change
Verification & Invariants
Language constructs never bind to same-named methods, while actual known-self method calls remain callable. Class identity and method case matching select the owning method. An unknown owner cannot fall back to a file-wide class match.
listand uppercase known-self calls on unmodified v8.How was this tested?
graphify update .: passed.git diff --check: passed.Graphify-specific checklist