Repository navigation
Conversation
…:class PHP class references only became edges when a `use` imported them, and `Foo::class` mostly did not, because a class-constant access linked only to a label declared in the same file. Same-namespace classes (no `use` needed), leading-backslash FQCNs and `self`/`static` were not resolved either. Class-constant accesses and type hints now record the name as written (`_php_raw`) and `_resolve_php_type_references` resolves it once: `use` alias, same namespace, or absolute FQCN, to the one declaration with that FQN (EXTRACTED). Names imported from outside the corpus keep the shared FQN-labelled external stub; an unresolved `::class` is dropped, never guessed. `self`/`static` map to the enclosing class. Property defaults and top-level statements (routes, bootstrap) are now walked, attributed to the class and file node. Incremental builds resolve against unchanged files via the resolution context. AST cache schema 20 -> 21. Because the build keeps one edge per undirected node pair, the many new `references_constant` edges (a method naming its own class, a file that both imports and `::class`-references a class) would replace the `method` / `imports` edge on the same pair. `references_constant` is therefore added to the generic relations that never displace a specific one. Not covered: `::class` in method parameter defaults, qualified names in `new \A\B()`, multi-namespace files.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
|
Thanks for the pull request, @if12is. 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 4 advisory finding(s) below merit a look before merge.
Not checked: tests were not run; formal verification proved 0 of 4 changed function(s) (1 sampled, not proven, 3 not verified).
Formal verification. PR-changed functions: 1/4 verified (0 proven, 1 may-equivalent, 0 distinguished) · 3 not verified (2 vacuous, 1 unsupported).
Not verified on this run: \_extract\_generic (unsupported), \_php\_collect\_type\_refs (vacuous: never exercised), \_resolve\_php\_type\_references (vacuous: never exercised).
Graphify review — findings
Adds PHP name resolution for Foo::class, other Foo::BAR class constants, and type hints: _resolve_php_type_references now ties each reference to the one declaration matching its use alias, same-namespace name, or leading-\ FQCN as an EXTRACTED edge. The extractor records the name as written on the edge (_php_raw) and maps self/static to the enclosing class. A name imported from outside the corpus keeps the shared FQN-labelled external stub, an unresolved type hint keeps its legacy bare-name stub, and an unresolved ::class edge is dropped at the end of extract rather than guessed. Treats references_constant as a generic relation when the graph collapses a node pair to one edge, so a method naming its own class, or a file that both imports and ::class-references a class, keeps its method / imports edge. Bumps the AST cache schema to 21 so cached results without the written name are re-extracted.
Worth a look
- _php_class_owners is snapshotted before walk populates method_owner/initializer_nodes —
graphify/extractors/engine.py:8746· Escalate · high- NOT verified (no proof, no reproducing execution) — needs human review
- Resolver failure silently drops every PHP ::class edge —
graphify/extract.py:10827· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
- Unresolved
_php_rawedges leak dangling targets and private key into output —graphify/extractors/engine.py:10182· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
- Mixed global and named PHP namespaces are treated as a single namespace —
graphify/extractors/resolution.py:3995· Escalate · medium- NOT verified (no proof, no reproducing execution) — needs human review
Review partial — this diff was larger than one review pass covers, so later files were not reviewed; some findings may be missing.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 5498 functions depend on the 1109 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract()— 990 callers, 59 callees - worse:
build_from_json()— 269 callers, 22 callees - worse:
_extract_generic()— 20 callers, 47 callees - worse:
walk()— 1 callers, 77 callees - worse:
walk_calls()— 1 callers, 33 callees - worse:
_resolve_php_type_references()— 2 callers, 9 callees
Verification — 5498 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: 4950 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
437 of 437 test file(s) selected (100%) via static blast radius.
Escalated to a full run for safety — the selection is not trustworthy on its own (see below). CI should run the whole suite.
tests/pi.test.ts— full-run-safetytests/test_affected_cli.py— full-run-safetytests/test_affected_defines.py— full-run-safetytests/test_affected_member_seed.py— full-run-safetytests/test_affected_overrides.py— impact, full-run-safetytests/test_agents_platform.py— full-run-safetytests/test_alias_folding.py— impact, full-run-safetytests/test_analyze.py— impact, full-run-safetytests/test_anthropic_custom_endpoint.py— full-run-safetytests/test_antigravity_install.py— full-run-safetytests/test_apm_fallback_version.py— full-run-safetytests/test_architecture_doc.py— full-run-safetytests/test_astro_extraction.py— impact, full-run-safetytests/test_astro_import_ids.py— impact, full-run-safetytests/test_atomic_canvas_export.py— impact, full-run-safetytests/test_atomic_version_stamp.py— full-run-safetytests/test_atomic_writes.py— impact, full-run-safetytests/test_backend_env_isolation.py— full-run-safetytests/test_backend_extras.py— full-run-safetytests/test_backend_opt_in.py— impact, full-run-safetytests/test_benchmark.py— impact, full-run-safetytests/test_benchmark_raw_graph.py— impact, full-run-safetytests/test_benchmark_real_query.py— impact, full-run-safetytests/test_blade_extractor.py— impact, full-run-safetytests/test_build.py— impact, full-run-safetytests/test_build_located_semantic_identity.py— impact, full-run-safetytests/test_build_merge_dedup_scope.py— impact, full-run-safetytests/test_build_merge_hyperedges_and_prune.py— impact, full-run-safetytests/test_build_merge_shrink_guard.py— impact, full-run-safetytests/test_builtin_global_type_refs.py— impact, full-run-safetytests/test_c_include_resolution.py— impact, full-run-safetytests/test_cache.py— impact, full-run-safetytests/test_cache_stale_import_target.py— impact, full-run-safetytests/test_callflow_html.py— full-run-safetytests/test_cargo_introspect.py— full-run-safetytests/test_cargo_missing_manifest.py— full-run-safetytests/test_carried_hyperedge_remap.py— impact, full-run-safetytests/test_case_sensitive_resolution.py— impact, full-run-safetytests/test_charmap_encoding.py— impact, full-run-safetytests/test_chunking.py— impact, full-run-safetytests/test_cjs_module_extension.py— impact, full-run-safetytests/test_claude_cli_backend.py— impact, full-run-safetytests/test_claude_md.py— full-run-safetytests/test_cli_broken_pipe.py— full-run-safetytests/test_cli_export.py— impact, full-run-safetytests/test_cli_help.py— full-run-safetytests/test_cloud_cta.py— full-run-safetytests/test_cluster.py— impact, full-run-safetytests/test_cluster_ambiguous_scale.py— full-run-safetytests/test_cluster_exclude_hubs.py— full-run-safety- … and 387 more
non-code file(s) changed (
ARCHITECTURE.md) → running the full suite for safety (a code graph can't see config/fixture/data deps)
changed code file(s) with no mapped test (
ARCHITECTURE.md) — a coverage gap or a missing link — running the full suite rather than only the selected tests
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
No difference found (not proven): No behavior difference found in extract (not a proof).
The verifier ran both versions of extract 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.
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
Could not verify: Could not verify \_php\_collect\_type\_refs.
The verifier did not have enough to check \_php\_collect\_type\_refs, 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: not verifiable: all 234 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly AttributeError — names the real obstacle, not a sampling gap)
Could not verify: Could not verify \_resolve\_php\_type\_references.
The verifier did not have enough to check \_resolve\_php\_type\_references, 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: non-vacuity: domain too small (only 1 distinct inputs exercised, need 3) — 'no divergence' would be near-vacuous
· 1 grounded finding(s) anchored inline below; 5 more finding(s) on lines outside this diff (see the check run).
There was a problem hiding this comment.
_resolve_php_type_references()
fans out to 9 callees (efferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
Branch:
upstream/php-class-name-resolution(base:upstream/v8at b90b9df). One commit.Related: #2615 (PHP support, consolidated; this PR does not close it). I found no issue that
describes this specific defect.
Problem
In a PHP corpus, a class reference only became an edge when the file had a
usefor it, andFoo::classmostly did not become an edge at all:Foo::class,Foo::BAR) linked only to a class label declared in thesame file, so
Client::classinbelongsTo(Client::class), casts, container bindings, routetuples and similar produced nothing when
Clientlives in another file;useneeded in PHP) were not resolved, and a leading-backslash FQCN(
\App\Models\Client) lost its namespace;self/staticin a type position became a sourcelessselfstub node;protected $casts = ['status' => Status::class]) and top-level statements(routes, bootstrap files) were not walked for class-constant accesses;
references_constantedge on the same node pairas a
methodorimportsedge replaced it (the graph keeps one edge per undirected pair).Implementation
graphify/extractors/engine.py: class-constant accesses and type hints record the name as written(
_php_raw);self/staticmap to the enclosing class; property defaults and top-levelstatements are walked (top-level ones are attributed to the file node).
graphify/extractors/resolution.py(_resolve_php_type_references): resolves the written nameonce, in PHP's order:
usealias, same namespace, absolute FQCN, to the one declaration with thatFQN (
EXTRACTED). A name imported from outside the corpus keeps the shared FQN-labelled externalstub (no per-file stubs). A
::classthat resolves to neither is dropped, not guessed. Incrementalbuilds resolve against unchanged files through the resolution context.
graphify/extract.py: passes the resolution context through; drops unresolved::classedges;strips the
_php_rawmarker from output.graphify/build.py:references_constantis added to_GENERIC_RELATIONS, so it never replaces aspecific relation (
method,imports,calls, ...) on the same pair, the same rulereferencesalready follows.
graphify/cache.py: AST cache schema 20 -> 21 (per-file output now carries the written name).ARCHITECTURE.md: documents the rules and the cases not covered.Verification
New tests:
tests/test_php_class_name_resolution.py(10 tests; every case has a same-named decoyclass in another namespace, so a bare-name coincidence cannot satisfy an assertion) and
references_constantadded toGENERICintests/test_relation_collapse_precedence.py(18 addedparametrized cases).
Fails before, passes after. Running both test files against the unmodified
graphify/ofupstream/v8(git checkout upstream/v8 -- graphify, then the same command):With the change:
Full checks on this branch:
Bare
upstream/v8(b90b9df) gives1 failed, 8469 passed, 15 skipped, ruff clean and the same641 errorsfrom pyright, so there is no new pyright error count. The one pre-existing failure istests/test_skill_no_backtick_substitution.py::test_control_a_backticked_unknown_command_is_caught;it fails identically on
v8. The second failure here,tests/test_pr10_watch_overrides.py::test_ignored_root_configuration_still_rebuilds_the_graph[False](a filesystem-watch timing test), passed when run alone (
2 passed); the full run shared the machinewith several other full runs.
Before/after on two real Laravel applications (private copies,
uv run graphify update <copy> --forcefrom this branch and from bare
upstream/v8; counts read fromgraphify-out/graph.json):references_constantedgesmethodedges lost to areferences_constanton the same pairThe
callsedges of app A are identical before and after (4705 each, same sources, targets andlines). In app B one
callsedge becamereferences_constantbecause it had been created for aFoo::classargument, which is not a call, and onereferences_constantedge became the correctcallsedge (self::DEMOhad been matched to a method nameddemo).Limitations
Foo::classin a method parameter default, and anew \A\B()written with a qualified name, stillresolve by bare name.
$class::class) gets no edge.