Skip to content

fix(php): interface/trait/enum declarations mint canonical nodes; sourced declarations win over sourceless lookup rivals; refuse relative-scope callees - #2536

Open
filipechagas wants to merge 3 commits into
Graphify-Labs:v8from
lawnstarter:upstream-fix/php-node-identity
Open

fix(php): interface/trait/enum declarations mint canonical nodes; sourced declarations win over sourceless lookup rivals; refuse relative-scope callees#2536
filipechagas wants to merge 3 commits into
Graphify-Labs:v8from
lawnstarter:upstream-fix/php-node-identity

Conversation

@filipechagas

Copy link
Copy Markdown

Three PHP fixes for one failure family — node identity: types that never mint a declaration node leave sourceless stubs to shadow the real name, absorb its fan-in, and poison both lookup and call edges. Found and measured on a 46k-node Laravel corpus; adapted from lawnstarter#51 (each commit carries the provenance line).

1. fix(extract): PHP interface/trait/enum declarations mint canonical nodes (325d614)

_PHP_CONFIG.class_types held class_declaration alone, so none of the three ever minted a node — 291 declarations with no node on the measured corpus (142 interfaces / 30 traits / 119 enums). Their fan-in scattered: implements/extends/trait-use minted a bare sourceless stub that shadowed the real name; Foo::CONST fan-in fragmented across per-file stubs; imports/parameter-type references parked on the file node or an FQN stub. The three kinds join class_declaration (mirroring Java/Groovy, which always had interface_declaration), with two grammar details: enum bodies are enum_declaration_list, and the _resolve_php_type_references raw-scan now reads all four declaration kinds and both body shapes — so interface Reader extends Sub\Repo resolves as written instead of falling to a same-namespace guess.

Not backward compatible with an existing graph: interface/trait/enum methods move from file-scoped to type-scoped ids (labels gain the member dot). A full graphify update . lands it consistently; a hook-driven incremental rebuild against a pre-fix graph drops (never repoints) stale-id edges until the next full update. Since AST cache entries are content-hash-keyed within the version namespace, a same-version rebuild replays pre-fix nodes — this PR deliberately does not bump the version (matching #2502/#2503 convention), so you may want to pair the merge with one.

Behavior consequence worth stating: a member-call receiver typed by an interface/trait/enum can now bind (there is now a definition to find) — consistent with how Java/Groovy interfaces already behave here.

2. fix(serve): find_node_ambiguity must not collapse sourceless rivals into one group (9e73fc0)

The ambiguity check groups the winning tier by source_file — but every extractor-minted stub carries source_file == "", so N stubs collapsed into one bucket, no ambiguity was reported, and explain silently answered with matches[0] (graph-iteration order), while affected on the same name refused with "No unique node match". Now: the exact tier prefers a sourced declaration over sourceless rivals; a tier of only stubs reports each stub as its own rival instead of picking one silently; and affected.resolve_seed learns the same rule so the two commands agree on every shape. Sourced-vs-sourced ties and lone-stub resolution are unchanged. Independent of #2516 (different functions).

3. fix(engine): refuse relative-scope names as PHP scoped-call callees (088ce5c)

The scoped_call_expression handler took the scope text as the callee name, so every parent::__construct() / parent::setUp() in the corpus emitted a raw call named parent — and the cross-file label pass then bound them all to whatever callable happened to be named parent(): 1,698 fabricated calls edges into one model accessor on the measured corpus. parent/self/static are now refused as callee names (resolution would need inheritance context the raw-call facts don't carry — refusal over guessing). Legit Helper::format() scoped calls are unaffected.

Tests

Full suite on this branch (based on v8 @ 9f25a3a): 4,109 passed / 3 skipped / 0 failed. Three new test files (+26 tests); each was re-verified red against clean v8 before the fix commit, not just green after. Zero pre-existing tests modified.

Two sibling fixes deliberately not in this PR

🤖 Generated with Claude Code

filipechagas and others added 3 commits August 7, 2026 11:32
)

The scoped_call_expression handler took the scope text as the callee name,
so parent::setUp() minted a raw call to a callee literally named 'parent'.
Unresolved in-file, that reached the cross-file pass, which matches by
normalized label and bound it to any unrelated ->parent() method in the
corpus (1,698 wrong inbound edges on ServiceCategory::parent() at api scale).

parent/self/static are relative scopes: which class they denote needs the
inheritance context the raw-call facts do not carry, so refuse rather than
guess. Absolute scopes are unaffected.

The fork gates this on `_PHP_NON_CONCRETE_TYPE_NAMES`, a 17-name set it also
applies to written-type reads and to its `(new X())->m()` receiver capture,
neither of which exists here. This introduces instead the three-name
`_PHP_RELATIVE_SCOPE_NAMES` — every name in it is justified at the one call
site that reads it — which is the whole of the set that can fire on a
scoped-call scope anyway: no PHP builtin is a legal `::` scope.

Adapted from #51.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nto one group (#49)

`find_node_ambiguity` grouped the winning match tier by `source_file`. Every
stub the extractor mints for a reference it could not resolve carries
`source_file == ""`, so N unrelated stubs collapsed into a single `""` bucket
and looked like N members of one file: no ambiguity was reported, and `explain`
answered with `matches[0]` — whichever stub `G.nodes()` yielded first. On the
pinned corpus that is the 18 stubs shadowing `BalanceitemRepository`; reorder
the graph and the same query answered with a different stub, equally
confidently, while `affected` refused with "No unique node match" (RC3 of #46).

- `_find_node_tiers` drops sourceless nodes from the exact tier when that tier
  also holds a sourced one. A stub is a broken duplicate of the real
  declaration, never the better answer, and not something the caller could
  disambiguate anyway — it has no path to retry with.
- Sourceless nodes are keyed individually in the ambiguity grouping, so a tier
  made only of stubs reports rivals instead of picking one silently. Neither
  change depends on stub counts staying high.
- `affected`'s `resolve_seed` learns the same sourced-beats-sourceless rule in
  its exact-label and bare-name passes, so `explain` and `affected` now agree:
  both resolve to the sourced declaration when one exists, both refuse when
  every rival is a stub.

Sourced-vs-sourced ties (the monorepo `MetricsPort` case) are untouched, and a
lone stub with no sourced rival still resolves as before.

Adapted from #51.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…des (#47)

`_PHP_CONFIG.class_types` held only `class_declaration`, so no node was ever
minted for a PHP interface, trait or enum — 291 declarations in a pinned
46.4k-node Laravel corpus. Every resolution pass that could canonicalize an
edge then had nothing to land on: `implements`/`mixes_in` kept bare sourceless
stubs that shadow the real name in `explain`, `Foo::CONST` fan-in fragmented
across per-file stubs, and `imports`/parameter-type `references` parked on the
*file* node (or, when the filename differs from the type name, on a sourceless
FQN-labeled stub).

Add the three declaration kinds to `class_types`, mirroring Java and Groovy.
An enum's body is an `enum_declaration_list` rather than a `declaration_list`,
so `body_fallback_child_types` learns it. The `_resolve_php_type_references`
raw-scan, which read `class_declaration` bodies only, now scans every
declaration kind and both body shapes: without it `interface Reader extends
Sub\Repo` and `enum Status { use Sub\Describes; }` recorded no raw text and fell
through to the same-namespace guess, resolving to the wrong `Repo`/`Describes`.

The fork's version of this commit also rewords `_php_non_class_types`, its
receiver-binding refusal for these three kinds, whose claim that they mint no
node this change falsifies. That pre-scan does not exist here, so nothing is
carried over: a receiver typed by an interface, trait or enum was never refused
in this tree and can now bind, since the definition the single-definition guard
was looking for finally exists.

The residual dangling `imports` edge for a type used only via `::class` is a
separate root cause and is NOT fixed here — this change supplies the node that
fix needs to land on. It is left pinned as a dangling target by the test, and
selected there by the target id rather than by the `target_fqn` edge metadata
the fork uses, which `_import_php` does not stamp in this tree (that metadata
is Graphify-Labs#2502, still open).

Adapted from #51.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Graphify reviewed this change.

Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.


Graphify review — findings

This PR appears to make several changes to PHP extraction and node resolution in a code-graph tool (graphify): 1. PHP relative-scope call handling: Modifies the scoped_call_expression handler so that parent::, self::, and static:: calls no longer emit the scope keyword as the callee name (previously they minted raw calls to callees literally named e.g. parent). 2. PHP declaration kinds: Adds interface_declaration, trait_declaration, and enum_declaration to PHP's class_types so these mint declaration nodes like class does, plus supporting grammar adjustments (enum_declaration_list body type, extended raw-scan coverage). This shifts interface/trait/enum method ids and labels to a type-scoped form. 3. Sourceless-stub resolution: Introduces _is_sourced/_prefer_sourced_node helpers in affected.py and adjusts tier/ambiguity logic in serve so a sourceless stub no longer shadows a real declaration and so explain and affected resolve/refuse consistently. The surface area spans extract.py, extractors/engine.py, extractors/resolution.py, affected.py, serve.py, the CHANGELOG, and associated tests. The changelog notes these are extraction-side changes requiring PHP corpus re-extraction. Note: the diff was truncated in the prompt, so my summary of serve.py, resolution.py, and the test files is inferred from the changelog and symbol names rather than read directly.

Worth a look

  • Relative-scope PHP calls (parent::/self::/static::) silently droppedgraphify/extractors/engine.py:582 · 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 — 2219 functions depend on the 850 functions this change touches.

Health — this change adds coupling hotspots:

  • worse: extract() — 378 callers, 39 callees
  • worse: resolve_seed() — 16 callers, 5 callees

Verification — 2219 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: 2155 function(s) in the blast radius were not formally verified this run

· 2 grounded finding(s) anchored inline below; 1 more finding(s) on lines outside this diff (see the check run).

Comment on lines 583 to 584
return _read_text(node, source).rsplit("\\", 1)[-1] or None

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relative-scope PHP calls (parent::/self::/static::) silently dropped — agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review

Graphify suggests a fix:

Suggested change
return _read_text(node, source).rsplit("\\", 1)[-1] or None
text = _read_text(node, source).rsplit("\\", 1)[-1]
if not text:
return None
# PHP keywords are case-insensitive; relative scopes (parent::/self::/static::)
# name no concrete class, so report them as "no scope" and let the caller emit
# an unqualified call instead of dropping the fact entirely.
if text.lower() in _PHP_RELATIVE_SCOPE_NAMES:
return None
return text

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked into this properly — measured rather than reasoned about — and I'd like to decline it, for two independent reasons.

The suggested edit is anchored on a function this path never calls. It patches _php_name_text (engine.py:579-586), but the scoped_call_expression branch reads its scope through _read_text(scope_node, source) at engine.py:4386; _php_name_text is called only at 600, 606, 2715, 2720 and 2734. Applied literally, the edit changes _php_collect_type_refs and _php_emit_base and leaves relative-scope calls untouched — while removing the references edges that function make(): static currently mints for static/self/parent, as an unreviewed side effect.

Applied as intended, it reintroduces the defect in a different place. I patched the actual branch so a relative scope emits the method name as an unqualified callee, and re-ran extraction on a PHPUnit-shaped corpus (base class out of corpus, one unrelated in-corpus setUp). Baseline: 0 edges. With the change: one INFERRED calls edge, FooTest::prepare()App\Models\ServiceCategory::setUp(). The raw call has is_member_call=False, so it is not skipped at extract.py:5572 and is not claimed by any typed resolver — it falls to the shared normalized-label loop at extract.py:5564+, whose index keys .setUp() and setUp() identically, making a single same-named method anywhere in the corpus an unconditional single-candidate bind. That is the same fabricated-edge shape as the bug this commit fixes, aimed at a different label. A corpus with two same-named candidates produced 0 edges in both trees, so the damage lands precisely where the bind is most confident.

And it cannot be resolved correctly at that point. The raw-call record carries caller, callee, receiver and receiver type — no enclosing or base class. The inherits edges that would answer "which class does parent denote" are never consulted by that loop, so an unqualified callee can only be name-matched. Refusing is also what the pipeline already does with every PHP member call it cannot type (extract.py:5572), so this keeps the existing policy rather than introducing a new class of drop.

On "silently": fair as a description of runtime behaviour, and the drop has no diagnostic counter. It is not undocumented, though — four tests in tests/test_php_relative_scope_calls.py pin it (including a positive control that absolute scoped calls still resolve), and the changelog entry states it outright. Binding a relative scope to its real target needs a base-class-aware resolution pass; I've opened that as its own issue on our fork rather than land an edge that is wrong in a different direction, and I'm happy to pick it up here if you'd want it.

Comment thread graphify/affected.py
return sourced[0] if len(sourced) == 1 else None


def resolve_seed(graph: nx.Graph, query: str) -> str | None:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Health regressionresolve_seed()

16 callers depend on it (afferent coupling).

Grounded coupling-delta finding (deterministic), not an LLM guess.

@filipechagas

Copy link
Copy Markdown
Author

On the automated coupling thread for resolve_seed() (graphify/affected.py:115, "16 callers depend on it") — I think this one is misfiled, and the numbers argue the opposite way.

Audited against base 9f25a3a, head 325d614:

  • Production fan-in is unchanged at 1. The only production caller is graphify/affected.py::format_affected, at both base and head. An AST scan across every .py in the tree counts 9 distinct calling functions at base and 14 at head; all five additions are test functions in the new tests/test_stub_shadow_ambiguity.py. (The tool reports 16 — I can't reproduce that exactly; it likely counts the from graphify.affected import resolve_seed statements as well.)
  • The callee bump is a decomposition, not accretion. The function goes 45 → 58 lines (+14 / −1), and its call targets 8 → 10. The two new targets are _is_sourced() and _prefer_sourced_node() (affected.py:100-113) — named module-level helpers extracted so the sourced-beats-sourceless rule is stated once and shared with _find_node_tiers, rather than inlined twice into the tier logic. The metric counts that as coupling; I'd call it the readability fix the alternative would have needed anyway.

So there's nothing to act on here that I can see. The one thread on this PR I do think deserves a real answer is the relative-scope one on engine.py:584, which I've replied to separately with measurements.

filipechagas added a commit to lawnstarter/graphify that referenced this pull request Aug 9, 2026
Brings in the six upstream commits since the last sync point 9f25a3a:

- 6ba0868 fix(cli): surface four silent success-exit failures (Graphify-Labs#2534, Graphify-Labs#2522)
- 22c41c1 fix(swift): cross-file extension no longer drops static/singleton
  calls (Graphify-Labs#2538)
- 3c17238 fix(dedup): deterministic node-id collision rank, active over
  archived (Graphify-Labs#2532)
- 3d19463 fix(skill): make the Windows skill variant runnable on PowerShell;
  bump to 0.9.36 (Graphify-Labs#2528)
- cfc6a75 fix(extract): TS member-call gating + Kotlin grammar match (Graphify-Labs#2553,
  Graphify-Labs#2552, Graphify-Labs#2526, Graphify-Labs#2550, Graphify-Labs#2551)
- 09a34ad fix(update,llm): retry failed extractions; surface claude-cli
  envelope errors; bump to 0.9.37 (Graphify-Labs#2543, Graphify-Labs#2554)

MERGE, not rebase, per the fork's established convention (be80845, 725d082).
The fork is 64 commits ahead of the merge base and seven of its branches are
open PRs against upstream (Graphify-Labs#2536, Graphify-Labs#2516, Graphify-Labs#2506, Graphify-Labs#2505, Graphify-Labs#2503,
Graphify-Labs#2502, Graphify-Labs#2492); rebasing v8 would rewrite the history all seven are cut from.
None of the six commits above is one of those PRs, so nothing merged here
duplicates work still in review upstream.

Conflicts resolved (4 files):

* graphify/extractors/engine.py — two hunks, both pure additions at a shared
  insertion point, so BOTH SIDES ARE KEPT. The fork's PHP raw-call locals and
  markers (`php_function_call`, `fcc`, `receiver_type`,
  `receiver_type_qualified`, `receiver_qualified`, `php_inline_new_*`) and
  upstream's Kotlin `kotlin_qualified_prefix` / `qualified_prefix` stamp now
  sit side by side on the same `rc_entry`. The two write disjoint keys and
  neither reads the other's, so no ordering question arises.

* graphify/extract.py — one hunk, again additions at a shared point. The fork's
  `_is_php_function_target` helper (#52) and upstream's early
  `_KOTLIN_IMPORT_TARGET_RESOLVER` run are both kept, with the Kotlin run left
  immediately above the import-evidence index — upstream's comment states it
  must precede that index or the INFERRED -> EXTRACTED promotion reads
  pre-rewrite targets, and the fork's helper is a def with no ordering
  constraint, so it goes first.

* pyproject.toml — 0.9.40, with the matching one-line uv.lock bump (fork
  precedent 457b31e, be80845). A new number rather than either parent's:
  the fork's line is at 0.9.39 and upstream's at 0.9.37, and the fork has
  already numbered a 0.9.36 and a 0.9.37 of its own.

* CHANGELOG.md — a pure insertion collision, no competing prose. Upstream's
  0.9.37 (unreleased) and 0.9.36 sections are absorbed VERBATIM and placed
  below the fork's own sections, above upstream's dated 0.9.35. Both are
  retitled `(upstream, ...)` with a collision note, following the precedent the
  fork-only 0.9.35 section set — the fork's 0.9.36/0.9.37 already occupy those
  numbers, and renumbering either line would cascade. Upstream's 0.9.36 is
  stamped 2026-08-07 from its own released section; upstream's 0.9.37 is still
  unreleased on their side (tag v0.9.37 is at 09a34ad, `git tag --contains`).
  A new `## 0.9.40 (unreleased)` on top describes this sync.

Semantic overlap, the one place a clean text merge was not enough:
upstream's Graphify-Labs#2553 rewrote `_resolve_typescript_member_calls`, which is also the
function the fork's cross-language isolation work rewrote (#24 index scoping,
#10 raw-call ownership). Git auto-merged it, and the result is correct because
BOTH SIDES ARE REFUSALS and therefore compose: the fork's
`_is_owned_definition` / `_raw_call_is_owned` gates keep the resolver off
definitions written in another language, and upstream's origin gate
additionally requires the matched type to be visible to the calling file (same
file, a named import, or a module the file imports). Upstream's other two
changes to that function are adopted unchanged — the `references`-edge fallback
for a typed receiver whose type lacks the method is dropped, and a
table-typed receiver is tiered INFERRED 0.8 while a source-written
`Type.method()` stays EXTRACTED 1.0.

One fork test adapted, and only because that adopted gate changed its path:
tests/test_mixed_corpus_member_calls.py::
test_typescript_receiver_resolves_despite_a_same_named_python_class asserted
EXTRACTED on a fixture whose `runner.ts` named `Lead` without importing it —
invalid TypeScript that resolved only because the pre-gate resolver matched on
name alone. The fixture gains the `import { Lead } from './lead';` real
TypeScript requires, and now asserts INFERRED, matching the Swift sibling test
that has always asserted it. What the test is FOR is unchanged and still holds:
the same-named Python decoy must not suppress the real TS edge (#24) — verified
directly through extract(), the TS edge resolves and `svc.py`'s `Lead.search`
gets nothing. The negative sibling
(test_typescript_receiver_type_does_not_match_a_python_class) deliberately
keeps the import-less `_TS_CALLER`, so it goes on pinning the fork's
definition-index scoping on its own evidence instead of passing for upstream's
reason. The `_call_context_pairs` docstring is corrected: the TS `references`
fallback it described no longer exists.

Verified byte-for-byte against both parents: all 51 upstream-only files are
blob-identical to upstream/v8 and 24 of 26 fork-only files blob-identical to
v8 — the two exceptions are exactly the deliberate edits above (the test file
and uv.lock's version line). For the four both-touched CODE files, every added
line from BOTH parents is present in the result, zero missing (cli.py fork
14 / upstream 193; extract.py 675 / 430; engine.py 899 / 146; watch.py 14 / 47).
cli.py and watch.py auto-merged with no overlapping hunks, and both sides'
features are live: the fork's `_php_class_fqns` / `_php_non_class_types`
resolution-context replay and upstream's `failed_sources` manifest handling.
build.py, dedup.py, detect.py, llm.py, tree_html.py, the skill artifacts and
tools/skillgen were untouched by the fork, so upstream's versions are taken
wholesale.

Suite: 4404 passed, 37 skipped, 0 failed. Pre-merge baseline on v8 was
4343 passed, 36 skipped, 0 failed; upstream adds 61 tests and 1 skip, so the
delta is fully accounted for and there are no pre-existing failures to discount.
`ruff check graphify/ tests/ tools/` clean; `python -m tools.skillgen --check`
reports 134 artifacts matching committed output.

NOT verified in this commit, and deliberately deferred to the reviewer: no
real-corpus spot-check was run. Four of the six upstream commits are
extraction-side (Swift extensions, Kotlin, the JS/TS callback walk, the TS
origin gate), so a Swift/Kotlin/JS/TS corpus needs `graphify update .` to pick
them up; the fork's own PHP corpus numbers are unaffected by anything here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant