From 056df2b02716f8ff9adcbbb816a865a0f5d43b3e Mon Sep 17 00:00:00 2001 From: Divyam Talwar Date: Thu, 24 Sep 2026 00:53:14 +0530 Subject: [PATCH] fix(graph): preserve Python qualified names without false edges --- src/graph/extract/python.ts | 35 +++++++--- tests/shared/graph/python.test.ts | 108 +++++++++++++++++++++++++++++- 2 files changed, 131 insertions(+), 12 deletions(-) diff --git a/src/graph/extract/python.ts b/src/graph/extract/python.ts index 9c65fada7..ad319d3f3 100644 --- a/src/graph/extract/python.ts +++ b/src/graph/extract/python.ts @@ -160,16 +160,14 @@ function handleClass(node: PyNode, relativePath: string, result: FileExtraction, /* c8 ignore next */ if (base === null) continue; // Only real base expressions are inheritance: a bare `identifier` (Base) - // or a dotted `attribute` (module.Base → use the final name). Skip - // `keyword_argument` (metaclass=Meta), *args/**kwargs, comments, etc. - // (codex review). + // or a dotted `attribute` (module.Base). Skip `keyword_argument` + // (metaclass=Meta), *args/**kwargs, comments, etc. (codex review). + // A dotted base keeps its FULL qualification (`requests.Session`, not + // `Session`) so the shared heritage resolver can't mistake it for a + // same-named local class (e.g. `class Session(requests.Session)` → self). let baseName: string | null = null; if (base.type === "identifier") baseName = base.text; - else if (base.type === "attribute") { - const attr = base.childForFieldName("attribute"); - /* c8 ignore next */ - baseName = attr !== null ? attr.text : null; - } + else if (base.type === "attribute") baseName = dottedAttributeName(base); /* c8 ignore next */ if (baseName === null || baseName.length === 0) continue; result.edges.push({ @@ -214,8 +212,12 @@ function extractImports(node: PyNode, relativePath: string, result: FileExtracti if (child === null) continue; let modText: string | null = null; let local: string | null = null; + // Unaliased `import a.b` binds `a` (the top package), NOT `b` — binding + // `b` → a.b would fabricate `b.f()` calls. Only a single-segment + // `import a` binds a namespace; dotted unaliased imports keep their + // `imports` edge but get no binding (conservative, never guessed). /* c8 ignore next */ - if (child.type === "dotted_name") { modText = child.text; local = lastDottedSegment(child.text); } + if (child.type === "dotted_name") { modText = child.text; local = child.text.includes(".") ? null : child.text; } else if (child.type === "aliased_import") { const name = child.childForFieldName("name"); const alias = child.childForFieldName("alias"); @@ -442,6 +444,21 @@ function firstOfType(node: PyNode, type: string): PyNode | null { return null; } +/** + * `a.b.C` attribute chain → "a.b.C" (whitespace/comments dropped). Returns null + * unless every link is a plain identifier (e.g. `f().C`, `m[0].C`), so no + * placeholder name is ever guessed from an arbitrary expression. + */ +function dottedAttributeName(node: PyNode): string | null { + if (node.type === "identifier") return node.text; + if (node.type !== "attribute") return null; + const obj = node.childForFieldName("object"); + const attr = node.childForFieldName("attribute"); + if (obj === null || attr === null) return null; + const head = dottedAttributeName(obj); + return head !== null ? `${head}.${attr.text}` : null; +} + function lastDottedSegment(dotted: string): string { const parts = dotted.split("."); /* c8 ignore next */ diff --git a/tests/shared/graph/python.test.ts b/tests/shared/graph/python.test.ts index 874309cf7..85f59a6d8 100644 --- a/tests/shared/graph/python.test.ts +++ b/tests/shared/graph/python.test.ts @@ -46,11 +46,11 @@ describe("extractPython (B6)", () => { expect(ex.edges.some((e) => e.relation === "extends" && e.source === "a.py:Sub:class")).toBe(true); }); - it("ignores keyword args in the base list and uses the final name of a dotted base (codex)", () => { + it("ignores keyword args in the base list and keeps a dotted base fully qualified (codex)", () => { const ex = extractPython("import abc\n\nclass C(abc.ABC, metaclass=Meta):\n pass\n", "a.py"); const extendsEdges = ex.edges.filter((e) => e.relation === "extends" && e.source === "a.py:C:class"); - // dotted base abc.ABC → "ABC"; the metaclass=Meta keyword arg is NOT a base. - expect(extendsEdges.some((e) => e.target.endsWith(":ABC:class"))).toBe(true); + // dotted base abc.ABC → "abc.ABC"; the metaclass=Meta keyword arg is NOT a base. + expect(extendsEdges.map((e) => e.target)).toEqual(["unresolved:a.py:abc.ABC:class"]); expect(extendsEdges.some((e) => e.target.includes("metaclass"))).toBe(false); expect(extendsEdges).toHaveLength(1); }); @@ -130,3 +130,105 @@ describe("extractPython (B6)", () => { expect(ext?.target).toBe("a.py:Base:class"); }); }); + +describe("extractPython — qualified names survive into the snapshot", () => { + function extendsTargets(src: string, file: string, source: string): string[] { + const snap = buildSnapshot([extractPython(src, file)], meta(), obs()); + return snap.links.filter((e) => e.relation === "extends" && e.source === source).map((e) => e.target); + } + + it("does NOT self-link `class Session(requests.Session)` to the local Session", () => { + const targets = extendsTargets( + "import requests\n\nclass Session(requests.Session):\n pass\n", + "client.py", + "client.py:Session:class", + ); + expect(targets).toEqual(["unresolved:client.py:requests.Session:class"]); + }); + + it("does NOT link a dotted base to a DIFFERENT same-named local class", () => { + const targets = extendsTargets( + "import base\n\nclass Model:\n pass\n\nclass User(base.Model):\n pass\n", + "models.py", + "models.py:User:class", + ); + expect(targets).toEqual(["unresolved:models.py:base.Model:class"]); + }); + + it("keeps a multi-segment dotted base fully qualified and unresolved", () => { + const targets = extendsTargets( + "import a.b\n\nclass C:\n pass\n\nclass D(a.b.C):\n pass\n", + "m.py", + "m.py:D:class", + ); + expect(targets).toEqual(["unresolved:m.py:a.b.C:class"]); + }); + + it("skips a base whose attribute chain is not plain identifiers (no guessed name)", () => { + const targets = extendsTargets( + "class Base:\n pass\n\nclass D(make().Base):\n pass\n", + "m.py", + "m.py:D:class", + ); + expect(targets).toEqual([]); + }); + + it("control: a bare local base still resolves alongside a dotted one", () => { + const targets = extendsTargets( + "import requests\n\nclass Mixin:\n pass\n\nclass Session(Mixin, requests.Session):\n pass\n", + "client.py", + "client.py:Session:class", + ); + expect(targets.sort()).toEqual(["client.py:Mixin:class", "unresolved:client.py:requests.Session:class"]); + }); + + it("control: a named-imported bare base still resolves cross-file", () => { + const base = extractPython("class Base:\n pass\n", "pkg/base.py"); + const sub = extractPython("from pkg.base import Base\n\nclass Sub(Base):\n pass\n", "app/sub.py"); + const snap = buildSnapshot([base, sub], meta(), obs()); + const ext = snap.links.find((e) => e.relation === "extends" && e.source === "app/sub.py:Sub:class"); + expect(ext?.target).toBe("pkg/base.py:Base:class"); + }); +}); + +describe("extractPython — `import a.b` binds `a`, not `b`", () => { + const util = () => extractPython("def helper():\n return 1\n", "pkg/util.py"); + function callEdges(caller: ReturnType): string[] { + const snap = buildSnapshot([caller, util()], meta(), obs()); + return snap.links + .filter((e) => e.relation === "calls" && e.source === "app/main.py:run:function") + .map((e) => e.target); + } + + it("emits no namespace binding for an unaliased dotted import, but keeps the import edge", () => { + const ex = extractPython("import pkg.util\n", "app/main.py"); + expect(ex.import_bindings).toEqual([]); + expect(ex.edges.some((e) => e.relation === "imports" && e.target === "external:pkg.util")).toBe(true); + }); + + it("does NOT emit a false call edge to the dotted-import leaf (`import pkg.util; util.helper()`)", () => { + const caller = extractPython("import pkg.util\n\ndef run():\n return util.helper()\n", "app/main.py"); + expect(callEdges(caller)).toEqual([]); + }); + + it("still repoints the dotted import edge to the real module", () => { + const caller = extractPython("import pkg.util\n", "app/main.py"); + const snap = buildSnapshot([caller, util()], meta(), obs()); + expect(snap.links.some((e) => e.relation === "imports" && e.source === "app/main.py::module" && e.target === "pkg/util.py::module")).toBe(true); + }); + + it("control: an explicit alias (`import pkg.util as util`) still binds and resolves", () => { + const caller = extractPython("import pkg.util as util\n\ndef run():\n return util.helper()\n", "app/main.py"); + expect(caller.import_bindings).toEqual([{ local_name: "util", imported_name: "*", kind: "namespace", specifier: "pkg.util" }]); + expect(callEdges(caller)).toEqual(["pkg/util.py:helper:function"]); + }); + + it("control: a simple `import util` still binds `util` as a namespace and resolves", () => { + const helpers = extractPython("def helper():\n return 1\n", "util.py"); + const caller = extractPython("import util\n\ndef run():\n return util.helper()\n", "app/main.py"); + expect(caller.import_bindings).toEqual([{ local_name: "util", imported_name: "*", kind: "namespace", specifier: "util" }]); + const snap = buildSnapshot([caller, helpers], meta(), obs()); + const targets = snap.links.filter((e) => e.relation === "calls" && e.source === "app/main.py:run:function").map((e) => e.target); + expect(targets).toEqual(["util.py:helper:function"]); + }); +});