Skip to content

fix(graph): resolve Python calls made through a module name or import alias - #544

Open
Knightkolla wants to merge 1 commit into
trailhq:mainfrom
Knightkolla:fix-python-module-call-resolution
Open

Knightkolla wants to merge 1 commit into
trailhq:mainfrom
Knightkolla:fix-python-module-call-resolution

Conversation

@Knightkolla

@Knightkolla Knightkolla commented Oct 3, 2026 •

Copy link
Copy Markdown

Fixes #537.

graft callers said "no indexed callers" for Python calls made through a
module. Three forms now resolve:

  • import util then util.resolve_plan()
  • import util as u then u.acquire_lock()
  • from pkg import sub then sub.f(), including the relative form
    from . import sub

Why it was dropped: the extractor was fine, util.f() came through as a
member call with receiver "util". But resolve.ts only resolves member
calls when the receiver has a type, and a module has no type, so the
edge just got thrown away.

What changed:

  • extract.ts now records which local names are bound to modules, so a
    call on one of them carries the module info.
  • resolve.ts maps that to a file and looks the name up inside that one
    file only, functions first then class constructors, needs exactly one
    match. So a same-named function in another file can't accidentally win.
  • from pkg import sub follows Python's own rule: if pkg/init.py
    defines sub itself, that wins. Otherwise it's the submodule file.

Not in scope here, left as known gaps:

  • import a.b with no alias (only a gets bound)
  • rebinding a module name later in the same scope (util = something_else)

Also ran into a separate bug while in here: the existing from-import
case doesn't really use an import table either, it just searches the
whole repo for a uniquely named function. Two files with the same
function name would silently break that too. Filing it separately,
didn't want to widen this PR.

@trailhq-graft

trailhq-graft Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

🌱 graft blast radius

1 area changed → 4 areas can be affected. 8 dependent symbols, depth 2.
Tests: Dependency Resolution has tests the diff did not touch.
Tag: @anirudhkumar-nanonets — 4 of 5 areas · @shhdwi — 3 of 5 areas · @Faeeth — Symbol Overlap Detection

flowchart TB
  A0(("Dependency Graph Construction<br/>5 symbols"))
  A1(("Graph Engine<br/>1 symbol"))
  A2(("Pull Request Review<br/>1 symbol"))
  A3(("Symbol Overlap Detection<br/>1 symbol"))
  classDef reached fill:#D9EDF3,stroke:#3AA7C9,stroke-width:1.5px,color:#0E313C;
  class A0,A1,A2,A3 reached;
Loading
Can be affected Symbols Nearest hop Reached from
Dependency Graph Construction 5 src/graph/build.ts:L214-L483 buildGraph — calls, depth 1 Dependency Resolution
Graph Engine 1 src/engine.ts:L97-L108 graph — calls, depth 2 Dependency Resolution
Pull Request Review 1 src/app/review.ts:L45-L99 reviewPullRequest — calls, depth 2 Dependency Resolution
Symbol Overlap Detection 1 src/notes/edit-overlap.ts:L79-L94 symbolsIn — calls, depth 2 Dependency Resolution
Who knows this code — 3 people across 5 areas
Area Who knows it
Dependency Resolution · changed @shhdwi — 14 commits, last 12d ago · @anirudhkumar-nanonets — 10 commits, last yesterday
Dependency Graph Construction · affected @anirudhkumar-nanonets — 22 commits, last 6d ago · @shhdwi — 9 commits, last 12d ago
Graph Engine · affected @anirudhkumar-nanonets — 17 commits, last 26d ago
Pull Request Review · affected @anirudhkumar-nanonets — 3 commits, last 2mo ago
Symbol Overlap Detection · affected @Faeeth — 1 commit, last today · @shhdwi — 1 commit, last 17d 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 8 dependent symbols, grouped by area

Dependency Graph Construction — 5 symbols in 5 files

  • src/graph/build.ts:L214-L483 — buildGraph (calls, depth 1)
    234: const rawEdges: RawEdge[] = [];
  • src/graph/check.ts:L59-L159 — checkGraph (calls, depth 2)
  • src/graph/container.ts:L153-L214 — extractContainer (calls, depth 2)
    155: const rawEdges: RawEdge[] = [];
  • src/graph/refresh-run.ts:L18-L21 — main (calls, depth 2)
  • src/graph/refresh.ts:L202-L225 — rebuild (calls, depth 2)

Graph Engine — 1 symbol in 1 file

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

Pull Request Review — 1 symbol in 1 file

  • src/app/review.ts:L45-L99 — reviewPullRequest (calls, depth 2)

Symbol Overlap Detection — 1 symbol in 1 file

  • src/notes/edit-overlap.ts:L79-L94 — symbolsIn (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.

  • ⚠ Dependency Resolution — 1 of 10 reached · 3 test files reach it, none changed here
    • not reached: extractTree, walk, bind, visit, bindTargets, visit, indexPyModule, resolvePyModule, …1 more
49 test suites also reference this code

59 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/claude-hooks.test.ts
  • test/claude-prompt-private.test.ts
  • test/container-extract.test.ts
  • test/context-only-dir.test.ts
  • test/context.test.ts
  • test/covers.test.ts
  • test/depth-grammars.test.ts
  • test/depth-parity.ts
  • test/generic-extract.test.ts
  • test/generic-node24-probe.ts
  • test/graph-bindings.test.ts
  • test/graph-cross-language.test.ts
  • test/graph-extensions.test.ts
  • test/graph-extract-dedup.test.ts
  • test/graph-go.test.ts
  • test/graph-incremental.test.ts
  • test/graph-invariants.test.ts
  • test/graph-java.test.ts
  • …29 more

graft blast · origin/main...HEAD · depth 2 · 3 changed files

Open the interactive graph → — click an area to see its dependent symbols at file:line.

@shhdwi

shhdwi commented Nov 8, 2026

Copy link
Copy Markdown
Collaborator

The bug still reproduces on main, thanks: callers resolve_plan finds no callers for util.resolve_plan(), u.resolve_plan() or sub.f(). Since #707, extract.ts parses through the SyntaxNode interface in src/graph/depth-parser.ts, which has no childrenForFieldName or firstNamedChild, so this branch no longer compiles once rebased. Could you rebase and use children/namedChildren, or add those two methods to the interface for both the native and WASM backends, then re-run the Python module test?

@Knightkolla

Copy link
Copy Markdown
Author

Thanks for checking! I’ll rebase onto main, switch to children/namedChildren so it fits the new SyntaxNode interface, and re-run the Python module test. Will push the update soon.

… alias

`import util` + `util.f()`, `import util as u` + `u.f()` and
`from pkg import sub` + `sub.f()` were dropped, so `graft callers`
reported no indexed callers for them.

The extractor already captured these as member calls, but resolve.ts only
resolves member calls whose receiver has a type, and a module has none.

- src/graph/extract.ts: record which local names are bound to modules, and
  put the module specifier on calls made through them. A parameter or local
  with the same name shadows the module inside that function.
- src/graph/resolve.ts: map the specifier to a repo file (absolute or
  relative) and look the name up inside that file only.
- test/graph-python-module-calls.test.ts (new): the three call forms,
  same-named functions in two files, shadowing, and the relative import.

Not covered: `import a.b` without an alias, and rebinding a module name
later in the same scope.

Assisted-by: Claude Code
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Signed-off-by: Knightkolla <kartikeyadhavala2468@gmail.com>
@Knightkolla
Knightkolla force-pushed the fix-python-module-call-resolution branch from 3c8b31d to eeb5e29 Compare November 8, 2026 19:31
github-actions Bot added a commit that referenced this pull request Nov 8, 2026
@Knightkolla

Copy link
Copy Markdown
Author

@shhdwi rebased onto current main (e02b754) and pushed, so it applies cleanly and compiles again.

I took the children/namedChildren route, so the SyntaxNode interface and both parser backends are untouched. It only uses childForFieldName, which is already on the interface. The imported names after import are read by position, so the module in from pkg import sub isn't mistaken for a name. Wildcards and comments are skipped.

Checked:

Two trail-scenario-hooks tests fail on a second local run because they keep a per-session "said once" marker in the Trail home dir. They fail the same on pristine main and pass with a clean HOME. Unrelated, but you may hit it.

Known gaps are unchanged (import a.b with no alias, rebinding a module name later in the same scope). The from-import case that relies on repo-wide name uniqueness is #545.

Ready for another look when you have time.

This branch has not been deployed

No deployments
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.

graft callers misses Python calls made through a module name or an import alias

2 participants