Skip to content

fix: include PHP type usages in reference edges - #339

Open
cookerpapa wants to merge 1 commit into
trailhq:mainfrom
cookerpapa:fix/php-type-references
Open

fix: include PHP type usages in reference edges#339
cookerpapa wants to merge 1 commit into
trailhq:mainfrom
cookerpapa:fix/php-type-references

Conversation

@cookerpapa

Copy link
Copy Markdown

Fixes #324.

Emit references for PHP type annotations, construction, instanceof and static class accesses. Reuse existing PHP import resolution for aliases and qualified names. Exclude dynamic expressions, declarations and primitive types.

Add 13 regressions, including the reported enum case through buildGraph and callersOf.

Validation: 12 new cases fail before the fix. Afterwards, the graph suite has 342 passes, 4 skips and zero failures. Typecheck and git diff --check pass.

@github-actions

github-actions Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

🌱 graft blast radius

1 area changed → 3 areas can be affected. 4 dependent symbols, depth 2.
Tests: Graph Resolution has tests the diff did not touch.
Tag: @Frankie-Xu — Graph Resolution, Graph Engine · @anirudhkumar-nanonets — Pull Request Review · 贾旭卿 — Graph Building, Graph Engine

flowchart TB
  A0(("Graph Building<br/>2 symbols"))
  A1(("Graph Engine<br/>1 symbol"))
  A2(("Pull Request Review<br/>1 symbol"))
  classDef reached fill:#D9EDF3,stroke:#3AA7C9,stroke-width:1.5px,color:#0E313C;
  class A0,A1,A2 reached;
Loading
Can be affected Symbols Nearest hop Reached from
Graph Building 2 src/graph/build.ts:L151-L410 buildGraph — calls, depth 1 Graph Resolution
Graph Engine 1 src/engine.ts:L91-L101 graph — calls, depth 2 Graph Resolution
Pull Request Review 1 src/app/review.ts:L45-L99 reviewPullRequest — calls, depth 2 Graph Resolution
Who knows this code — 6 people across 4 areas
Area Who knows it
Graph Resolution · changed Matt Strayer — 1 commit, last 14d ago · @Frankie-Xu — 1 commit, last 14d ago
Graph Building · affected @tpoignonec — 1 commit, last 14d ago · 贾旭卿 — 1 commit, last 14d ago
Graph Engine · affected @Frankie-Xu — 1 commit, last 10d ago · 贾旭卿 — 1 commit, last 14d ago
Pull Request Review · affected @anirudhkumar-nanonets — 2 commits, last 9d ago · @qoole — 1 commit, last 14d ago

Ownership is git history over each area's own files, weighted towards recent work (120-day half-life). Merge commits and bots are dropped, and you are dropped from your own PR. A name with no @ has no GitHub handle in its commit email — tag them by hand, or add a .mailmap entry. A suggestion from history, not a CODEOWNERS rule.

All 4 dependent symbols, grouped by area

Graph Building — 2 symbols in 2 files

  • src/graph/build.ts:L151-L410 — buildGraph (calls, depth 1)
  • src/graph/refresh.ts:L150-L227 — ensureFreshGraph (calls, depth 2)

Graph Engine — 1 symbol in 1 file

  • src/engine.ts:L91-L101 — graph (calls, depth 2)

Pull Request Review — 1 symbol in 1 file

  • src/app/review.ts:L45-L99 — reviewPullRequest (calls, depth 2)
Test signal per changed area — 1 ⚠

Reached = a node under a test path has a resolved edge into the changed symbol. It undercounts anything called indirectly — through a CLI, a spawned process or a dynamic import — so read a low ratio as “look here”, never as a coverage gate.

  • Graph Resolution — 1 of 5 reached · 3 test files reach it, none changed here
    • not reached: walk, phpAttributeClassRef, phpClassRef, phpTypeReference
35 test suites also reference this code

38 symbols, kept out of the diagram and the table so they cannot crowd out the areas a reviewer has to look at.

  • test/ask-index.test.ts
  • test/ask.test.ts
  • test/container-extract.test.ts
  • test/context-only-dir.test.ts
  • test/context.test.ts
  • test/covers.test.ts
  • test/generic-extract.test.ts
  • test/graph-cross-language.test.ts
  • test/graph-go.test.ts
  • test/graph-incremental.test.ts
  • test/graph-invariants.test.ts
  • test/graph-java.test.ts
  • test/graph-languages.test.ts
  • test/graph-php.test.ts
  • test/graph-posix-paths.test.ts
  • test/graph-python.test.ts
  • test/graph-r-classes.test.ts
  • test/graph-r-phase3.test.ts
  • test/graph-r-phase4.test.ts
  • test/graph-r-phase5.test.ts
  • …15 more

graft blast · refs/graft/base...HEAD · depth 2 · 3 changed files

Open the interactive graph → — click an area to see the code that changed, and the line that reaches it.

github-actions Bot added a commit that referenced this pull request Sep 10, 2026

@Frankie-Xu Frankie-Xu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewing as someone who landed the PHP attribute (#155) and anonymous-class (#175) edges this is continuing.

What looks right

  • Type positions are the ones that matter for #324:
    amed_type,
    ew, instanceof, scoped access (::class / static method / static property). The dedicated extract test excludes declarations, primitives, self/parent/static, and dynamic expressions (
    ew $Shade(), $value->Shade(), $Shade::RED).
  • phpAttributeClassRef now delegates to phpClassRef, so use aliases (including grouped use App\Types{Color as Shade, …}) stay on the existing PHP import map. Attribute name nodes are
    ame / qualified_name, which phpTypeReference's switch does not match, so this should not double-emit #155 edges.
  • Body is Fixes #324 only. It should not close #144.
    esolve.ts still does if (!byId.has(targetFile)) continue for a specifier whose module is not in the graph — that is exactly why vendor #[Route] / #[Deprecated] still vanish. The comment tweak on the no-specifier PHP branch is fine; it does not keep those inferred edges.

Ask

  • statusCheckRollup on this PR only shows Blast viewer publish (green). The Node test matrix (ubuntu/windows + WASM regression) has not appeared. If that is waiting on first-time-contributor workflow approval, the extract/resolve change is still reviewable; I would not merge until est/graph-php.test.ts (existing in-repo attribute fixture) and the new graph-php-references.test.ts have actually run on CI.

Not requesting changes on the type-reference work itself. Vendor attributes remain a separate slice on #144.

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.

PHP emits no type-reference edges, so callers reports nothing for any class or enum

2 participants