Skip to content

fix(resolution): Python module-qualified calls colliding with builtin methods - #1704

Open
inth3shadows wants to merge 3 commits into
colbymchenry:mainfrom
inth3shadows:fix/python-module-member-builtin-collision
Open

fix(resolution): Python module-qualified calls colliding with builtin methods#1704
inth3shadows wants to merge 3 commits into
colbymchenry:mainfrom
inth3shadows:fix/python-module-member-builtin-collision

Conversation

@inth3shadows

Copy link
Copy Markdown

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) — where ledger is a project module exporting a top-level append — was classified as list.append by isBuiltInOrExternal and discarded. The only escape hatch was a capitalized receiver matching a known class, so a module receiver never qualified. The ref never reached resolveViaImport / 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 bare append ref. That bare ref then exact-matched an unrelated top-level append as 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.

resolution.test.ts   190 passed
extraction.test.ts   622 passed

Provenance

Found by running testgraph's trace-derived ground-truth comparison against its own codebase — the Python graph showed ledger.append with zero callers while the runtime trace showed several, and showed callers it did not have (inth3shadows/testgraph#66).

CHANGELOG entry added under [Unreleased] → Fixes.

… 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).
@danusha2345

Copy link
Copy Markdown
Contributor

Two notes from merging this into a local integration build of current main:

  1. The extractor half is dead once the native kernel is present. Python extraction runs in codegraph-kernel/src/python.rs whenever the .node is staged (every published bundle ships it), so a change made only in src/extraction/tree-sitter.ts never reaches runtime and the two arms diverge — the same trap fix(resolution): resolve direct calls through aliased Python function imports #1518 / fix(go): resolve cross-module calls in multi-module layouts #1521 fell into. Your own rows.setdefault(k, []).append(x) test passes in that build only because fix(extraction): never fabricate an edge from a call-result receiver #1692 already keeps call receivers in both arms (<inner>().<method>, TS/JS/Python, tsjs/extractors.rs + python.rs, parity fixtures). The broader receiver shapes you cover (self.data.append, d[k].append) would need the same mirror in python.rs plus a line in __tests__/fixtures/kernel-parity/torture.py.

  2. The resolver half is the part fix(extraction): never fabricate an edge from a call-result receiver #1692 does not have, and it is good. isBuiltInOrExternal letting a receiver through when it is an imported module of the caller's file (ledger.appendresolveViaImport / resolvePythonModuleMember) closes the false-negative side of Python: a top-level function named like a collection method (append/update/get) gets ZERO real callers and fabricated ones — uncovered shapes left by #715 and #1317, live in 1.6.0 #1681. I took exactly that hunk plus your tests into the integration build (resolution 208 → 262 passed, no regressions); the extractor hunk was dropped as above.

Suggestion so the maintainer does not get two competing PRs for one bug: keep this PR to the resolver half (the ledger.append recall fix, which stands on its own and merges clean), and let #1692 carry the receiver encoding for both arms. Happy to cross-reference either way.

…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.
@inth3shadows

Copy link
Copy Markdown
Author

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 bundle

Installed @colbymchenry/codegraph 1.5.0 (which ships codegraph-linux-x64/lib/kernel/codegraph-kernel.node), three files, codegraph index, then reading the edges table directly:

build_map (unrelated.py) --calls--> append (ledger.py)   <- fabricated (self.data.append)
build_map (unrelated.py) --calls--> append (ledger.py)   <- fabricated (rows["k"].append)
add_outcome --calls--> ledger.append                     <- MISSING

So both halves of #66 are live in the shipped build, which is exactly your point: a fix in src/extraction/tree-sitter.ts alone never runs there.

#1692 covers one of the three shapes

python.rs's extract_call filters the receiver to identifier | simple_identifier | field_identifier and sends everything else to a bare method_name. #1692 adds one branch for receiver.kind() == "call". That fixes d.setdefault(k, []).append(x) — but an attribute chain (self.data.append) and a subscript (d[k].append) still fall through to the bare name, and the bare name is what exact-matches an unrelated project function.

The parity suite already fails without the mirror

__tests__/fixtures/kernel-parity/torture.py line 26 is self.registry.lookup("x") — the attribute-chain shape. With this PR's TS hunk applied and no kernel mirror, kernel-tsjs-parity fails on it:

wasm:   referenceName "self.registry.lookup"
kernel: referenceName "lookup"

That failure is invisible in a from-source checkout: with no .node staged, every kernel-*-parity suite describe.skipIfs itself and the extraction tests silently exercise the wasm arm. I only saw it after installing Rust and running scripts/build-kernel.sh.

What 2c5319c does

  • Mirrors the branch into codegraph-kernel/src/python.rs for all non-identifier receivers, with a collapse_js_whitespace helper rather than char::is_whitespace — the two sets differ (U+0085 in one, U+FEFF in the other) and the parity sweep compares arms byte for byte.
  • Adds the subscript and call-chain shapes to torture.py.
  • Adds __tests__/kernel-python-call-fabrication.test.ts, which forces CODEGRAPH_KERNEL_LANGS=python and asserts the end-to-end property on the arm that ships, skipping when no .node is staged.

Verified by rebuilding the kernel both ways: kernel-tsjs-parity 17/17 with the mirror, 2 failures without it; the new suite passes against a freshly built kernel.

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.
@inth3shadows

Copy link
Copy Markdown
Author

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

extractPythonImports emits an ImportMapping for every import — stdlib and PyPI included — so imp.localName === receiver was 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 shares the name.

A project with class Store: remove(), get() and a file containing only import os / import requests:

this branch @2c5319c   cleanup -> Store.remove   refName "os.remove"
                       cleanup -> Store.get      refName "requests.get"
installed 1.6.0        (0 edges)

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

__tests__/python-import-gate.test.ts pins both directions and fails on the first without the change (['remove@store.py','get@store.py'] vs []). resolution + extraction + kernel parity: 832 tests, all pass.

You were also right that my tests were vacuous for the attribute-chain shape

I checked this properly. ledger.append is a top-level function, and Strategy 3 only considers method kinds — so the negative assertion in my test could never have failed. With a method decoy (class Sink: def append(self, x)) the picture is:

site released 1.6.0 this branch
self.data.append(1) wrong edge · exact-match 0.9 wrong edge · instance-method 0.7
rows[k].append(2) wrong edge · exact-match 0.9 gone
self.inner.get(k) wrong edge · exact-match 0.9 wrong edge · instance-method 0.7

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 refName — but it is not the fix its own comment claims. self.data.append still reaches the bare-name fallback, because Python has no exclusive chained-receiver branch equivalent to matchGoFieldChainCall / matchRustSelfFieldCall. Only receivers containing non-word characters (subscript, call chain) get the silent-miss behaviour.

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 proposed

Still happy to go either way. If you take the resolver half, please take b34d85f with ite799cd0 alone is the regression above.

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.

2 participants