fix(resolution): Python module-qualified calls colliding with builtin methods - #1704
Conversation
… methods A call like `ledger.append(row)` was silently dropped: isBuiltInOrExternal treated any `x.method()` as `list.append`/`dict.update`/etc whenever `method` matched a common collection method name, unless the capitalized receiver matched a known CLASS — never checking whether the receiver was a known imported MODULE exporting a same-named top-level function. The real call never reached resolveViaImport, so ledger.py's actual callers went uncounted. Separately, a method call through a non-identifier receiver (an attribute chain like `self.data`, or a call chain like `rows.setdefault(k, []).append`) degraded at extraction time to a bare `append` ref. That bare ref then exact-matched the same unrelated top-level `append` as the sole same-named symbol project-wide, fabricating a call edge from unrelated functions. Same root cause as both bugs: the Python method-call heuristics assumed a common method name always means a builtin, and never accounted for a project module exporting a function of that name. Fix: check import bindings before declaring a qualified call a builtin, and stop collapsing non-identifier receivers to a bare name that can collide (mirrors the colbymchenry#1230/colbymchenry#1276 fix philosophy — an unresolved qualifier is a silent miss, never a wrong edge). Found and reproduced via testgraph's trace-derived ground truth run against itself (inth3shadows/testgraph#66).
|
Two notes from merging this into a local integration build of current
Suggestion so the maintainer does not get two competing PRs for one bug: keep this PR to the resolver half (the |
…on.rs The TS extractor keeps a python call's receiver text as a qualifier when the receiver is not a plain identifier — an attribute chain (`self.data.append`), a subscript (`d[k].append`) or a call chain (`d.setdefault(k, []).append`) — so a bare `append` can never exact-match an unrelated project function of that name (colbymchenry#66). `codegraph-kernel/src/python.rs` still collapsed all three to the bare method name. Python is in the kernel's DEFAULT_ROUTED set and every published bundle ships the .node, so the TS-only fix never ran where it mattered: on the installed 1.5.0 build, `self.data.append(...)` and `rows["k"].append(2)` both fabricated a `calls` edge onto an unrelated module-level `append`, while the real `ledger.append(row)` was missing. A from-source checkout has no .node, so the existing coverage silently exercised the wasm arm and stayed green. Mirrored the branch, with a `collapse_js_whitespace` helper rather than `char::is_whitespace`: the sets differ (U+0085 in one, U+FEFF in the other), and the parity sweep compares the two arms byte for byte. torture.py gains the subscript and call-chain shapes; the attribute-chain shape (`self.registry.lookup`) was already there and is what makes kernel-tsjs-parity fail without this commit. The new test asserts the end-to-end property on the kernel arm specifically, and skips when no .node is staged, like the parity suites. Verified: kernel-tsjs-parity 17/17 with the mirror, 2 failures without it (rebuilt both ways); the new suite passes against a freshly built kernel.
|
Thanks — point 1 is correct, and I verified it rather than taking it on trust. It turned out to be a stronger argument for mirroring the extractor hunk than for dropping it, so I've pushed the mirror (2c5319c) instead of narrowing the PR. The bug reproduces on a published bundleInstalled So both halves of #66 are live in the shipped build, which is exactly your point: a fix in #1692 covers one of the three shapes
The parity suite already fails without the mirror
That failure is invisible in a from-source checkout: with no What 2c5319c does
Verified by rebuilding the kernel both ways: Happy to defer the call-chain shape to #1692 and keep this PR to the attribute-chain/subscript shapes plus the resolver half, if that avoids overlap — the two changes are compatible either way, since #1692's branch is checked before the general one. |
…odule The escape added for colbymchenry#66 asked only whether SOME import bound the receiver's local name. Every import produces a mapping — stdlib and PyPI included — so it was also true for `os`, `requests`, `np`. That opened the built-in-method filter for them; `resolveViaImport` then found no project file, resolution fell through to the bare-name strategy, and the call bound to whatever project method happened to share the name. The escape hatch reintroduced the exact fabrication class the filter exists to prevent. Verified before the fix, on a project with `Store.remove` / `Store.get`: import os; import requests cleanup -> Store.remove refName "os.remove" cleanup -> Store.get refName "requests.get" Two wrong edges where 1.6.0 produced none. The escape now resolves the import specifier and opens only when it names a file in this project — the same question `resolveViaImport` asks next, so a receiver that passes is one the qualified path can actually serve. `from . import mod` and `import pkg.mod as m` are both handled; anything else stays a silent miss rather than a wrong edge. `__tests__/python-import-gate.test.ts` pins both directions, and fails on the first without this change (`['remove@store.py','get@store.py']` vs `[]`). resolution + extraction + frameworks + kernel parity: 855 tests, all pass.
|
Heads-up, and an apology: the resolver hunk you took into your integration build has a regression. Fixed in b34d85f, but please re-check that build. A code-review pass over the em-tagged build of this branch caught it, and I reproduced it before believing it. The regression
A project with Two fabricated edges where the released build produced none — the exact class this PR set out to remove, arriving through its own escape hatch. The fix (b34d85f)The escape now resolves the import specifier and opens only when it names a file in this project — the same question
You were also right that my tests were vacuous for the attribute-chain shapeI checked this properly.
So the extraction hunk is a strict improvement here — three wrong edges become two, and the survivors drop from the top confidence tier to 0.7 with an honest That gap is real and I have not fixed it here — it needs a Python chained-receiver resolution branch, which is a bigger change than this PR should carry. Where that leaves the split you proposedStill happy to go either way. If you take the resolver half, please take b34d85f with it — e799cd0 alone is the regression above. |
Since colbymchenry#66 kept the receiver's text, `self.data.append(1)` reaches the resolver as `self.data.append` — which `matchMethodCall`'s dotMatch splits into receiver `self.data` + method `append`, and the bare-name strategies then bound it to any project method of that name. The comment on colbymchenry#66 claimed the qualifier prevented exactly this. It did not; only receivers with non-word characters (subscript, call chain) got the promised silent miss. This is the discipline Go (colbymchenry#1276), Rust (colbymchenry#1585) and PHP's `this->prop.method` already have in this same function: a dotted python receiver resolves through validated inference or not at all. **It costs recall, and the cost is measured rather than waved at.** Indexing the tracked .py of four real projects: three unchanged (278 / 643 / 301 call edges), and a 249-file one 2605 -> 2564. Of the 41 dropped, 38 were fabrications — 21 x a dict `.update` bound to a service's `update`, 16 x application code bound to a `get` defined in a TEST file, and `self._model.transcribe` on an external Whisper model bound to the file's own `transcribe` — and 3 were genuine `self._capture.stop()` hops onto the class the constructor assigns. Those 3 are recoverable: python names an attribute's type in the class body (`self.x: T`, `self.x = T()`, a typed `__init__` parameter, a class-level annotation, a base class). A first attempt read those with regexes over the class's source lines and review killed it — with only `#` stripped it took a type out of a DOCSTRING and turned a correct edge into a wrong one, read a nested class's `__init__` as the outer class's, and stripped the package off `requests.Session()` to bind an external object to a project class. Doing it right needs the AST, and it is its own change. Until then this shape is a silent miss, which is the trade this file makes everywhere else. The three negative tests each use a DISTRACTOR — a second project symbol with the same method name. Without one the old fallback found the right target by single-candidate luck and the test passed on both arms, proving nothing; that is how the first version of this suite was vacuous. Verified: all three fail against the pre-change resolver, and the two boundary guards (a single-segment inferable receiver, a module-qualified call) pass on both arms. resolution + extraction + frameworks + kernel parity: 860 tests, all pass. Plan: ~/.claude/plans/codegraph-python-attribute-chain-receiver.md
Problem
Two Python call-resolution bugs with one root cause: the method-call heuristics assume a common collection-method name always means a builtin, and never account for a project module exporting a function of that name.
1. Real calls silently dropped.
ledger.append(row)— whereledgeris a project module exporting a top-levelappend— was classified aslist.appendbyisBuiltInOrExternaland discarded. The only escape hatch was a capitalized receiver matching a known class, so a module receiver never qualified. The ref never reachedresolveViaImport/resolvePythonModuleMember, which already resolve it correctly.ledger.append's actual callers went uncounted.2. Wrong edges fabricated. A method call through a non-identifier receiver — an attribute chain (
self.data.append(x)), a subscript (d[k].append(x)), a call chain (rows.setdefault(k, []).append(x)) — degraded at extraction time to a bareappendref. That bare ref then exact-matched an unrelated top-levelappendas the sole same-named symbol project-wide, inventing a call edge between functions with no relationship.Fix
src/resolution/index.ts— before declaring a qualified call a builtin, check whether the receiver is an imported module in that file (getImportMappings). If it is, let it through to import resolution.src/extraction/tree-sitter.ts— for Python, keep the receiver's source text as a qualifier instead of collapsing an unresolvable receiver shape to a bare method name. An unresolved qualifier is then a silent miss, never a wrong edge.Same philosophy as #1230 / #1276: prefer a missing edge over a fabricated one.
Tests
__tests__/resolution.test.ts— one test covering both directions: the module-qualified call resolves to the module's function, and the chained-receiver call does not attach to it.Provenance
Found by running testgraph's trace-derived ground-truth comparison against its own codebase — the Python graph showed
ledger.appendwith zero callers while the runtime trace showed several, and showed callers it did not have (inth3shadows/testgraph#66).CHANGELOG entry added under
[Unreleased]→ Fixes.