Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions src/lib/analysis/analyzers/analyzers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,38 @@ class Dog(Animal):
const render = graph.nodes.find((n) => n.id === "widget.js::render");
expect(render?.parent).toBe("class::widget.js::Widget");
});

test("python: un metodo compartido por dos clases no se atribuye a la clase equivocada", async () => {
const graph = await run(pythonAnalyzer, [
[
"dup.py",
`class A:
def __init__(self):
pass
def a_only(self):
pass

class B:
def __init__(self):
pass
def b_only(self):
pass
`,
],
]);

// Unique methods attach to their own class.
expect(graph.nodes.find((n) => n.id === "dup.py::a_only")?.parent).toBe(
"class::dup.py::A",
);
expect(graph.nodes.find((n) => n.id === "dup.py::b_only")?.parent).toBe(
"class::dup.py::B",
);
// The shared __init__ is ambiguous: parented to the module, never the wrong class.
expect(graph.nodes.find((n) => n.id === "dup.py::__init__")?.parent).toBe(
"mod::dup.py",
);
});
});

describe("typescript", () => {
Expand Down
26 changes: 23 additions & 3 deletions src/lib/analysis/analyzers/shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import type { Graph, GraphNode, GraphEdge, SourceFile } from "../types";

type Node = Parser.SyntaxNode;

// Cap parsing of a single file so a pathological input can't block the event loop.
const MAX_PARSE_MICROS = 5_000_000; // 5s

export type LangSpec = {
language: string;
wasm: string;
Expand Down Expand Up @@ -77,7 +80,9 @@ function stripQuotes(text: string): string {
type FileFacts = {
file: string;
defs: Set<string>;
methodClass: Map<string, string>; // function name -> owning class name
// function name -> owning class, or null when the name is owned by more than
// one class in the file (ambiguous: don't attribute it to the wrong class).
methodClass: Map<string, string | null>;
classes: Set<string>;
extendsRel: { cls: string; base: string }[];
calls: { caller: string | null; callee: string }[];
Expand All @@ -90,19 +95,34 @@ async function parseFile(
paths: Set<string>,
): Promise<FileFacts> {
const { parser, language } = await getParser(spec.wasm);
parser.setTimeoutMicros(MAX_PARSE_MICROS);
const tree = parser.parse(source.content);
// parse() returns null when the timeout is hit; skip the file instead of crashing.
if (!tree) {
return {
file: source.path,
defs: new Set<string>(),
methodClass: new Map<string, string | null>(),
classes: new Set<string>(),
extendsRel: [],
calls: [],
imports: new Set<string>(),
};
}
const root = tree.rootNode;

const classNodeTypes = spec.classNodeTypes ?? new Set<string>();
const defs = new Set<string>();
const methodClass = new Map<string, string>();
const methodClass = new Map<string, string | null>();
const defQuery = language.query(spec.funcDefQuery);
for (const { node } of defQuery.captures(root)) {
const name = resolveName(node);
if (!name) continue;
defs.add(name);
const cls = enclosingClassName(node, classNodeTypes);
if (cls) methodClass.set(name, cls);
if (!cls) continue;
// Same method name in two classes (e.g. __init__) -> ambiguous, mark null.
methodClass.set(name, methodClass.has(name) && methodClass.get(name) !== cls ? null : cls);
}
Comment on lines 118 to 126

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Class-vs-module same-name defs are still mis-attributed to class parents.

The ambiguity rule currently handles only class-vs-class collisions. If a module-level function and a class method share the same name in one file, the merged node can still be parented to a class (wrong ownership).

💡 Proposed fix
   const defs = new Set<string>();
   const methodClass = new Map<string, string | null>();
+  const moduleLevelDefs = new Set<string>();
   const defQuery = language.query(spec.funcDefQuery);
   for (const { node } of defQuery.captures(root)) {
     const name = resolveName(node);
     if (!name) continue;
     defs.add(name);
     const cls = enclosingClassName(node, classNodeTypes);
-    if (!cls) continue;
+    if (!cls) {
+      moduleLevelDefs.add(name);
+      if (methodClass.has(name)) methodClass.set(name, null);
+      continue;
+    }
+    if (moduleLevelDefs.has(name)) {
+      methodClass.set(name, null);
+      continue;
+    }
     // Same method name in two classes (e.g. __init__) -> ambiguous, mark null.
     methodClass.set(name, methodClass.has(name) && methodClass.get(name) !== cls ? null : cls);
   }

Also applies to: 264-272

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/analysis/analyzers/shared.ts` around lines 118 - 126, The ambiguity
detection in the methodClass.set call only handles class-vs-class collisions but
misses the case where the same name appears both at module-level and in a class.
Update the condition that currently checks if methodClass.has(name) and
methodClass.get(name) !== cls to also account for the scenario where one of the
definitions is at module-level (cls is null/falsy) and another is in a class,
treating this as an ambiguous case by setting methodClass to null whenever a
name is seen in different class contexts or between module-level and class-level
definitions.


const classes = new Set<string>();
Expand Down
Loading