fix(resolution): Python module-qualified calls colliding with builtin methods - #1704
fix(resolution): Python module-qualified calls colliding with builtin methods#1704inth3shadows wants to merge 23 commits into
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
|
b964128 closes the attribute-chain gap I flagged in my last comment. It is the discipline Go (#1276), Rust (#1585) and PHP's It costs recall, and I measured rather than assumedIndexing the tracked
So 38 fabrications for 3 real edges. I think that trade is right for a graph agents read, but it is a trade, not a free win. The 3 are recoverable, and my first attempt at recovering them was wrongPython names an attribute's type in the class body —
Doing it properly needs the AST rather than a line scan, so it is a separate change. Until it exists this shape is a silent miss. On the testsWorth flagging as a general trap for this area: my first version of these tests passed against the pre-change resolver. With a single project-wide symbol of that name, the old bare-name fallback found the right target by luck, so the test proved nothing. Every negative test here now carries a distractor — a second project symbol with the same method name — and I verified all three fail against the pre-change resolver while the two boundary guards (a single-segment inferable receiver, a module-qualified call) pass on both arms. 860 tests across resolution, extraction, frameworks and kernel parity. |
The gate in c8106bf made `self.data.append(1)` a silent miss instead of a fabricated edge — 38 fabrications dropped on a 249-file python repo, at the cost of 3 genuine `self._capture.{start,stop,chunks}()` edges. This recovers those 3 without bringing any fabrication back. Python declares no field types, so the evidence is spread around the class body, and there are four shapes: a class-level annotation (`conn: Client`, the dataclass / pydantic / attrs style), a typed `__init__` parameter, `self.x: T`, and `self.x = T()`. `memberTypesInTree` in `graph/branch-guards.ts` already reads "the declared types of a class's members" from the tree for TS, Java, Kotlin and C#; this adds the python cases, so the UI's Steps tab gets them too — which is what that function exists for. `memberTypesForSourceSync` is the sync entry the resolver needs, in `guardsForFileSync`'s shape: it serves only a language whose grammar is already loaded and yields nothing otherwise, never a wrong type. **Reading the TREE rather than the class's source lines is the whole point, and it was learned the hard way.** An earlier regex version of this reader was rejected in review for taking a type out of a DOCSTRING and turning a correct edge into a wrong one, reading a nested class's `__init__` as the outer class's, and stripping the package off `requests.Session()` to bind an external object to a project class. The tree answers the first two by construction: a docstring is a `string` node and never an assignment, and a nested `class_definition` is simply not descended into. The third is answered by refusing a dotted type outright, the stricter form of the rule `matchGoFieldChainCall` already applies. A typed parameter binds to the attribute assigned FROM it, not to one that merely shares its name, so `self._conn = conn` is right. Precedence is decided rather than accidental: annotation, then parameter, then constructor call, first declaration winning within a tier — source order would let `self.conn = Stub()` beat a later `self.conn: Client`. Measured on the same four corpora. Three unchanged (278 / 643 / 301 call edges). coriolis-local, against the pre-gate baseline of 2605: gate only 2564 38 fabrications gone, 3 genuine edges lost gate + this 2567 the same 38 gone, all 3 recovered, 0 invented Diffed edge-by-edge: nothing lost against the gate-only arm, and nothing present that was not in the pre-gate baseline. Indexing that corpus: 2983ms -> 3091ms median of 3, inside run-to-run spread. 11 new tests, each negative one carrying a DISTRACTOR — a second project symbol with the same method name — because without one the bare-name fallback finds the right target by luck and the test passes on both arms. 7 of the 11 fail against the pre-change tree; the other 4 assert a silent miss the gate alone also gives, and are guards rather than pins. Full suite 4143 passed, every failure a dist-dependent CLI/MCP suite. Plan: ~/.claude/plans/codegraph-python-attribute-type-inference.md
Four defects in the reader added by 35bd4e9, all found by review. **A decorated method was invisible.** tree-sitter wraps `@inject def __init__` and `@property def x` in `decorated_definition`, and both scans tested `stmt.type === 'function_definition'`. Dependency injection and `@property` are exactly where python puts its types, so the common case read as "this class declares nothing". **An `__init__` parameter's type leaked onto any same-named binding.** The parameter types were collected once and applied to `self.x = <identifier>` in EVERY method, so an unrelated local inherited the constructor's type: def __init__(self, dep: Real): self._dep = dep def load(self): dep = make_decoy() # a Decoy self._cache = dep # was typed Real That is a wrong edge, the fabrication class this path exists to prevent. A parameter's type now applies only inside the `__init__` that declared it. **The class was lost six blocks down.** The climb from the call site stopped after 16 frames; `with` + `for` + `if` + `try` inside a method already exceeds it, and losing the class reads as an empty declaration set. It now climbs to the root. **The read was quadratic in refs per class.** The resolver asks once per REF and python's reader walks every method body — unlike the other languages, which scan only the class body's direct children. Measured on a full index: a 2000-method class went 1.2s -> 41s, a 4000-method one to 196s. The parse tree cache now carries the answer per enclosing class, so each class is read once. Five unit tests, one per defect plus the docstring/nested-class guard, at the reader level: several of these are invisible end-to-end, because they only change what happens where the resolver would have produced nothing anyway.
…ports Two wrong-edge defects from review, both in what the inferred type name was allowed to match. **A bare-imported external class bound to a project class of that name.** `pythonAnnotationType` refused a DOTTED type, so `self.h = requests.Session()` was safe — but `from requests import Session` then `self.h = Session()` is the far more common spelling and nothing refused it. It bound to a project `models.Session`. Same shape for `from pathlib import Path` against a project `class Path`. **With two project classes of a name, the method lookup fell back to index order** and landed on a `Client` declared in a TEST file — the exact thing the CHANGELOG entry this branch adds says no longer happens. `pythonTypeClass` answers both: a name the file IMPORTS must come from the module the import names, and a name it does not import must be declared in the file itself. An external import matches no project file and yields null. The method is then looked up on THAT class in THAT file; the shared `resolveMethodOnType` is used only for the inherited case, and only when the class name is unambiguous, because with two candidates there is nothing here to choose between them. That also settles a third, smaller finding: `self.h = make_client()` recorded the "type" `make_client`, which now matches no class and yields nothing. Three tests, each with a distractor.
…that resolves The python attribute-type reader added by 35bd4e9 did nothing in a real build. It reads member types off the parse tree and, by design, returns nothing when the grammar is not loaded — no type beats a wrong one. Nothing loaded it where the resolver runs. A full index routes parsing to `parse-worker.js`, so `loadGrammarsForLanguages` is called only inside those workers and never on the main thread, where `ReferenceResolver` runs. `sync` loads them on the main thread, so it worked there. The result was worse than the feature being absent: `codegraph index` produced no attribute edges and `codegraph sync` produced them, so a project's graph depended on how each file happened to be indexed last, and `sync` runs from a git hook. `resolver-worker.ts` had the same hole independently, and its own contract makes it sharper — `resolveAndPersistBatched` promises the parallel switch "changes wall-clock, never the graph", but a worker with no grammar resolves the same refs to fewer edges. It now warms before posting `ready`, so no batch can arrive first. `warmResolverGrammars` loads only the languages the resolver reads trees for (python today) and only those the project actually contains. **Every test for this feature ran from source, where `parse-worker.js` does not exist, so they all took the in-process fallback and could not fail.** That is the same trap the kernel/wasm split set two commits earlier on this branch. The new test runs the built binary: one case asserts the edge after a full index, the other asserts index and sync agree. Both fail against the unfixed build — `[]` after index, and index/sync disagreeing — which is exactly the bug.
…eir names Review found three statements this branch had made false and four tests that passed for reasons other than what they claim. **Two false statements.** The gate's comment still said the AST reader did not exist — it is on the next line. And the CHANGELOG said "Nothing to re-index", but these are `calls` rows written while indexing; without a re-index or a `sync` nothing changes for an existing project. **`matchRustSelfFieldCall`'s doc block was orphaned** — the python helpers had been inserted between it and its declaration, so it documented the python builtin set and the Rust function had none. **Four tests, verified by mutation rather than by reading.** Deleting the branch each is named for used to leave the whole suite green: - `unwraps Optional and a | None union` declared both attributes and called only one, leaving the `| None` branch untested. Split in two, so each calls what it declares. - `a plain container stays a silent miss` was carried entirely by the generics check and by `self.h = []` not being a call; the builtin list was unexercised. A new case annotates `self.h: dict` in a project that shadows `dict` — a bare name, so only the builtin refusal stops it. - `a same-named class in another file donates nothing` left the attribute untyped, so the reader produced nothing and the cross-file question was never reached. It now types the attribute, and is renamed for what it tests. - the external-object test covered only the DOTTED spelling; renamed to say so, now that the bare-import case has its own. Both previously-dead branches now fail their test when removed.
…y uses it Correcting 32c49f8. That commit added a per-class memo to stop the python reader re-walking a whole class once per ref, and claimed the measured fix. The memo landed on `memberTypesForFile` — the async twin the UI uses — because the line it replaced appears in both functions and the first match won. `memberTypesForSourceSync`, which is the one the resolver calls, kept re-walking every time, so the cost 32c49f8 said it had fixed was still there in full. I only caught it by re-running the benchmark after the fact rather than trusting the change. Measured, indexing a single class of N methods each calling `self.h.run(x)`: before 32c49f8 now (gate-only baseline) 500 methods 2,944ms 2,593ms 371ms 303ms 2000 methods 39,453ms 39,005ms 1,252ms 939ms Quadratic in both earlier arms; linear now, at ~1.3x the baseline. Pinned by IDENTITY rather than wall-clock: two calls for two different lines in one class must hand back the same Map object, which is only true if the memo is on the path under test. A timing assertion would be flaky and would not have caught this failure mode — the code was present and correct, just wired to the wrong function.
…k inheritance Five wrong-edge mechanisms and one dead code path, all found by review of the previous round. Every one is `pythonTypeClass` trusting something it shouldn't. **A docstring decided the module.** `getImportMappings` is a REGEX over raw text, so `from decoy import Real` written inside a docstring produces a mapping indistinguishable from the real import, and `.find()` took the first. The AST rewrite closed exactly this hole on the TYPE side and I re-opened it one layer down on the MODULE side. Now every binding of a name must agree, or there is no answer. **Hand-rolled path math.** `replace(/^\.+/, '')` discarded the dot COUNT, so `from ..core import X` was looked up inside the importing file's own package — wrong module when one existed there, silent miss otherwise. And matching candidate class paths with `endsWith` accepted `examples/services/client.py` for `from services.client import …`. `pythonModuleFile` resolves the specifier properly — the resolver's own `resolveModulePath` first, then a dot-count-correct path checked AGAINST THE INDEX rather than pattern-matched. **`hit[0]` on ambiguity.** With two survivors the alphabetically-first path won, which is the "picks by index order" failure this function was written to prevent — I applied that rule to the class name and not to the file. Two candidates now means no answer. **An alias never resolved.** `from kinds import Real as R` looks up `R`, which names no class. The lookup uses the EXPORTED name, and the method lookup uses the chosen class's own name. **The inheritance hand-off could never fire.** The comment said an inherited method falls through to `resolveMethodOnType`'s supertype walk. That walk needs `extends` edges, which do not exist in the pass that resolves these refs — which is why PHP's `this->prop.method` is parked for the conformance pass. Python's shape was parked by nothing, so `class Real(Base)` — a service subclass, the most ordinary shape there is — resolved to nothing. It is parked now (`PY_SELF_ATTR_SHAPE`), and the walk is done here against the pinned class rather than by name, since searching every class of a name is the ambiguity this function exists to refuse. Six tests, each with a distractor, each verified to fail against the previous commit.
Within the `constructed` tier `put` is first-wins and the methods were walked in source order, so a `reset()` written ABOVE `__init__` decided the attribute's type: `self.h = Decoy()` there beat `self.h = Real()` in the constructor, and the call resolved to Decoy. That is the same accident the annotation/parameter/constructor tiering exists to avoid — I tiered the categories and then let source order decide inside one of them. `__init__` is the declaration; another method's assignment is a reassignment.
…ames **A third resolution path resolved without grammars.** `sync`'s retry sweep re-resolves refs in files it did NOT change, while the orchestrator loads grammars only for the CHANGED files' languages — so a TypeScript edit that makes parked Python refs retryable re-resolved them with no Python parser and dropped their attribute edges. Warmed there too. The public synchronous `resolveReferences()` cannot warm (loading is async) and now says so rather than degrading silently; only tests call it. **The deep-nesting test passed under the old cap.** Its fixture was 15 frames and the cap it claimed to pin was 16, so reverting the change left it green — the "unmutated-but-dead" pattern the file's own header warns about. Each python block costs two frames; the fixture is now 21, measured rather than assumed, and fails under the old cap. **The worker's grammar warming is left uncovered, deliberately.** A resolver worker takes no batch until one reaches `MIN_PARALLEL_BATCH`, and fixtures up to 150 files x 30 calls still resolve entirely on the main thread — so a test at any size this suite can afford passes with the fix removed. Two attempts did exactly that before I checked. The path was verified by measurement instead (9,000 edges vs 3,332 on a 1,500-file project with the pool forced on), and the gap is written down where the test would have gone.
… and restore the recall the module rewrite overshot Five findings from review of the previous round. **The supertype walk crossed same-named classes — a wrong edge.** `getSupertypes` matches by NAME, so it unions the `extends` targets of every python class called `Real`: a `b/real.py::Real` that inherits nothing was given `a/real.py::Real`'s base and resolved `self.h.zorp(x)` onto it. That is the same "two classes of a name is no evidence" rule this function enforces for the receiver, dropped one level down — the third time in this feature that I applied a rule in one place and not the adjacent one. No unique receiver class, no walk. **The module rewrite overshot and cost real recall.** I removed the `endsWith` match because it TIEBROKE wrongly, and took the suffix search with it: - a package root that is not the repo root — `src/pkg/core.py` imported as `pkg.core`, which is the packaged-project default, plus `backend/` and monorepo subdirectories — stopped resolving entirely - `from pkg import Client` re-exported by `pkg/__init__.py`, the dominant python package idiom, stopped resolving Both are back, and neither reintroduces the tiebreak: the suffix search returns an answer only when EXACTLY ONE file matches, so `services/client.py` and `examples/services/client.py` still cancel each other out. The re-export follows one hop through the package's own import of that name, so the answer is still a module the source names. **A commented-out import cancelled the real one.** The mappings come from a regex with no comment stripping, so `# from legacy import Real` above a real import produced a permanent disagreement and the new agreement rule refused both. Bindings are now filtered against the file's uncommented import lines. **Two adjacent JSDoc blocks** left the original strategy list detached from `resolveReferences`. Four tests, each reproducing the reviewer's case, all four verified to fail against the previous commit. Full suite 4271 passed; the one failure is `sync-rebuild-convergence`'s 5s timeout, which passes in isolation and has been flaky on this box all day. Corpus delta unchanged: coriolis-local 2605 -> 2567, the same 3 genuine edges recovered, 0 invented, 0 lost.
The suffix fallback accepted any indexed path ending in `/<module>.py` as long as exactly one did. Uniqueness is not evidence — the same mistake this file's header already records for `examples/services/client.py`, reintroduced one layer out. The imports that reach the fallback are overwhelmingly NOT project modules: on one real corpus 81 of 172 absolute import sources were stdlib. Each claimed any project file that merely shared its name, and the edge carried `provenance: null`, so nothing downstream could discount it. Reproduced through the built binary: `from logging import Logger` bound to a project's own `app/utils/logging.py`; `from redis.client import Redis` bound to a test double under `tests/fixtures/`; `from config.settings import Real` bound to a deployment tree's `deploy/config/settings.py`. Two gates, both python's actual import rule rather than a heuristic: - `PYTHON_STDLIB_TOP` — a module python itself provides is never the project file of that name. - `isPythonPackageRoot` — the anchor must not itself carry an `__init__.py` (python puts the top-level package directly on `sys.path`, so a directory inside a package is never a root), and every intermediate segment of the module path must be a real package. PEP 420 namespace packages are refused deliberately: a silent miss is the correct outcome under this feature's gate. The `src layout` fixture had no `__init__.py`, so it was pinning a layout python could not import; it also had no distractor, so it would have passed under a bare-name fallback. Both fixed. Each of the three new tests fails against the parent commit.
The comment filter added last round was a hand-written, line-anchored regex run over comment-stripped lines. Three problems, all of them the filter disagreeing with the thing it filters: - **Stricter than the extractor.** `extractPythonImports` matches `from X import Y` anywhere on a line; the filter required column 0. So `import os; from kinds import Real` had its binding dropped and stopped resolving — a straight recall regression against the previous commit. - **Comments only, not strings.** A docstring's usage example still landed in the live set. At the top level the agreement rule absorbed that (by refusing both bindings), which is why it looked closed. - **Unreadable and empty were the same value.** The `size === 0` escape was meant for a file that could not be read, but it also fired for a file whose only import-looking line WAS the comment — leaving that comment as the sole binding, unopposed, deciding an edge. Replaced with `livePythonImportSources`: strip comments and single- and triple-quoted strings, then run the extractor's own two patterns over what is left, and return null (not an empty set) when the file cannot be read. Behavior change worth naming: `an import written in a DOCSTRING does not decide the module` now resolves to the real import instead of to nothing. The guarantee is unchanged — the decoy never decides — but a docstring is now distinguishable from code rather than merely disagreeing with it, so the real binding survives. Three of the four new tests fail against the parent commit; the fourth is labelled in-place as a guard on the un-anchoring, not as a fix.
The `__init__.py` re-export hop read `getImportMappings` raw, forty lines below the text filter the previous commit added for the importing file. This is the defect class this feature keeps reproducing: a rule enforced at one site and not the sibling one level down. Here it was the worse of the two sites. The top level has an agreement rule, so a bogus binding at worst causes a refusal; the hop has none, so a commented-out or docstring import was the SOLE binding and DECIDED an edge. Reproduced through the built binary: a `pkg/__init__.py` carrying `# from decoy import Client` above a real `from pkg.core import *` produced `box.py::Box::go -> decoy.py::Client::run` with `provenance: null`. Three rules brought across, all of them already settled at the top level: - the live-source filter, so comments and docstrings are not bindings; - the ambiguity key is source AND exported name, not source alone, which had called two different re-exports of one name unambiguous; - the hopped-to class is looked up under its EXPORTED name, so `from .core import Legacy as Client` finds `Legacy`. Both new tests fail against the parent commit. The existing re-export test had no distractor and would have passed under a bare-name fallback; it has one now.
The supertype walk keyed on `cls.name`, so `getSupertypes` unioned the bases of every python class sharing that name and a class inheriting nothing appeared to inherit its namesake's. The fix for that refused the walk whenever the name was not unique project-wide — the right question answered with the wrong evidence. `cls` is already ONE node in ONE file; `pythonTypeClass` pinned it. A `Real` in a test tree says nothing about it, and vetoing on that lost every inherited edge for the commonest class names in the codebases this feature exists for — a `Client`, `Config`, `Service` or `Manager` defined twice took its whole subtree with it. Reproduced: `class Real(Base)` with an unrelated `unrelated/real.py` present resolved to nothing, and resolved again as soon as the namesake was renamed. Adds `ResolutionContext.getSupertypesOfNode(nodeId, language)` — the `extends` / `implements` edges out of that exact node — and walks nodes rather than names. The one ambiguity that genuinely remains, resolving a supertype NAME back to a class, is still refused exactly as before, which is what keeps the namesake test passing. The new test fails against the parent commit.
…scanning
`getAllFiles()` is an uncached `SELECT path FROM files`, and the suffix fallback
scanned the whole result — three `endsWith` per path — for every python import
that reached it. `import-resolver.ts` already documents this exact mistake being
removed once, for lua requires, and supplies the fix: a `WeakMap` per-context
basename index.
This case was worse than the lua one. The imports reaching the fallback mostly
match NOTHING (third-party packages), so the `found.length > 1` early exit never
fires and each pays the full scan — twice, because the resolver retries in a
second pass.
Measured on a synthetic 4,641-file python project, 600 consumers importing a
third-party module, 3 runs each, through the built binary:
parent (a3c412f) 32.0 / 34.8 / 32.4 s
with the index 6.0 / 5.7 / 6.3 s
Buckets preserve `getAllFiles()` order, so the candidate list filters to exactly
the array the full scan produced — same matches, same winner, and the existing
suite is the proof of that.
Also memoizes `livePythonImportSources` per (context, file). `getFileLines` is
LRU-cached but the strip-and-scan was not, so it re-ran for every
`self.attr.method()` ref in a file — the O(refs x file length) shape
`resolution/types.ts` records as ~20% of index CPU on a java-heavy repo once
before.
No behavior change, so no new test: the 35 existing cases pin the results and
the numbers above are the claim.
|
This is the AST change I said was needed but missing. My last comment ended with the 3 genuine edges lost to the gate, and: "doing it properly needs the AST rather than a line scan, so it is a separate change. Until it exists this shape is a silent miss." Pushed 15 commits; it exists now, and on the 249-file project the count goes back the other way — the same 3 edges ( It reads the four shapes python actually uses to name an attribute's type — constructor assignment, typed Five review rounds hit this. The last one reproduced two defects through the built binary, both of which invented edges:
That is the same failure twice — a rule applied at one site and not the sibling one level down — which is the trap worth naming for anyone working in this file. Also here: import text is classified properly now (comments and single/triple-quoted strings stripped, then the extractor's own patterns re-run over what's left, so the filter can never be stricter than the thing it filters); the supertype walk asks the pinned receiver node for its bases instead of its name, which was costing every inherited edge in any repo with a Verification:
The split offer stands unchanged — if you'd rather take only the resolver half, take b34d85f with e799cd0. |
|
Heads-up: the false-negative half of #1681 ( |
The maintainer asked on colbymchenry#1704 to drop the bits that were landing separately once colbymchenry#1749 merged. Both named PRs are on main now (colbymchenry#1748 bb1d309, colbymchenry#1749 edcd36e), along with three more python-resolution changes the same morning, so this reconciles against all of them rather than only the two. Six conflicts, resolved by what each side is now the authority on: - src/resolution/index.ts — take main in all three hunks. Its isPythonProjectModule is our own, byte-identical apart from one comment, so ours is deleted rather than merged; git had auto-merged both copies into a duplicate definition. Its isKnownClass is the stricter colbymchenry#1776 version, and the collection-binding filter now short-circuits before the class escape. - __tests__/resolution.test.ts — take main. Our +54 was subsumed by its +347; the delta of this branch against main for that file is now zero, which is exactly what was asked for. - src/extraction/tree-sitter.ts and codegraph-kernel/src/python.rs — keep both arms, main's first. colbymchenry#1748 claims the call-chain receiver (`d.setdefault(k, []).append(v)`) and encodes it as `<inner>().<method>`; ours stays the catch-all beneath it for the shapes that arm does not match, the attribute chain (`self.data.append`) and the subscript (`d[k].append`). The comments on both arms said they covered the call-chain shape and no longer do, so they are corrected in place. - __tests__/fixtures/kernel-parity/torture.py — union. bucket_chains keeps the call receivers; fabrication_shapes keeps a real attribute chain and a subscript, since its former call-chain line is now bucket_chains' job. What this branch still carries that main does not: the python attribute-type inference read from the AST, the chained-receiver gate, the kernel mirror for non-call receivers, the branch-guard changes and the module-suffix index. Verified: tsc --noEmit clean; npm test 4226 passed / 238 files. The three remaining failures (object-literal-methods, two in ui-steps-api) reproduce on a clean upstream/main worktree at ee83636 and are not from this merge; a fourth, ui-server-api's "under 100 ms", is a wall-clock assertion that passed and then failed at 113ms in the same command. NOT verified: the Rust arm. There is no cargo on this machine, so every kernel-*-parity suite describe.skipIf's itself and the python.rs change is unexercised.
…ipper livePythonImportSources hand-rolled its own comment and string scanner. main now carries strip-comments.ts (colbymchenry#1746) with a python arm that does the same job, and this file already composes it one function above for the TS receiver-type read, so the bespoke scanner is the odd one out — and a bespoke one drifts from the extractor it exists to mirror. blankStringContents(stripCommentsForRegex(source, 'python')) blanks comments, triple-quoted docstrings and single-line string contents while preserving offsets, so the line anchoring the `^import` pattern relies on survives. The extractor's own two patterns are unchanged, which is the property that matters: a filter stricter than the thing it filters silently drops real bindings, and an earlier line-anchored version did exactly that. Checked against the cases this path exists for before swapping — a commented-out import, a docstring usage example, a single-line string holding import text, an inline comment after a real import, a triple quote opened and closed on one line, and two shapes that could trip the helper's JS regex- literal heuristic (`(a)/b`, an f-string with a slash). All eight give the same answer as the scanner they replace, including the two the hand-written version got wrong. The null-versus-empty distinction is kept: null means the file could not be read, empty means it was read and every import-looking line was comment or string — the case where every binding must be refused.
|
Done — merged Dropped, because they landed as your extracts
Reconciled rather than dropped
VerificationLinux, Node 22.23.2, with the native kernel built and staged — so the All 8 failures are pre-existing on
Measured what this branch's python work is worth, on repos neither of us chose — and ablated it so the number is attributable to one hunk rather than to "the branch". Method. Three python codebases (318 The trade82 of those 90 are fabricated. I read every call site rather than inferring from the name: Production code bound to a symbol in a test file, and a function that appears to call itself. The 8 that are real, stated plainly. All one shape — Which hunk earns itRebuilt the branch twice, removing one piece at a time.
So the two halves are one mechanism, not two independent changes — and the attribute-type inference is what makes the strict half affordable, since it is the only thing that turns a refused Where it is not betterCross-copy binding. My kb and lodestone corpora each contain two sibling copies of one project, and both arms mis-bind across them — upstream 664 and 113, this branch 668 and 115. It adds 4 and 2. Neither side handles the duplicate-copy ambiguity; I am not claiming this branch fixes it, and the corpus is unusually hostile there. Reproducible end to end if you want the fixtures — the whole thing is |
Three commits landed since the last merge at ee83636, one of them colbymchenry#1785 — the aliased-import fix, which patches BOTH join sites in import-resolver.ts (resolvePythonModuleMember and resolveModuleImportToFile), so the file->file imports edge for `from pkg import mod as alias` is covered too. Only CHANGELOG conflicted; both sides append at the end of [Unreleased], so main's colbymchenry#1626 bullet keeps its place and this branch's "Language and framework accuracy" sub-heading follows it. tsc --noEmit clean. Full suite 255 files / 4416 passed; the same 8 failures as before the merge, all reproduced on a clean upstream/main worktree (4 dart kernel-parity, object-literal-methods, 2 ui-steps-api) or a wall-clock flake (ui-server-api's "under 100 ms"). Python and kernel suites specifically: 284/284 across resolution, the six python suites, kernel-tsjs-parity and kernel-python-call-fabrication.
# Conflicts: # src/resolution/index.ts # src/resolution/name-matcher.ts
3 participants
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.