From 9249e8a1ce664005ae4335abbd07c01be2868ca9 Mon Sep 17 00:00:00 2001 From: VivienP <87722478+VivienP@users.noreply.github.com> Date: Sat, 15 Aug 2026 17:57:36 +0200 Subject: [PATCH] fix(graph): compare syntax nodes by identity, not JS wrapper (#116) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two cold builds of byte-identical source could produce different graphs. The node set was stable, and so were `calls`, `contains`, `imports` and `implements` — only `references` moved, between 201 and 238 edges across repeated builds of graft's own source. `isDirectCallee`, `isDeclarationName` and `withoutShadowedImports` ask whether an identifier IS the callee (or the declared name) of its parent, and answered by comparing `SyntaxNode` objects with `===`. node-tree-sitter materializes those wrappers on demand and caches them weakly, so reaching one node twice can hand back two different JS objects depending on whether a scavenge ran in between. The guards returned different verdicts for identical source, and a direct callee was intermittently booked as a value reference as well. `sameSyntaxNode()` compares the stable node id within the same tree instead. An id is unique per tree but not across trees, so the tree is compared too; a Tree is one object per parse, unlike its nodes, so `===` is right for it. Applied at those three sites — the only SyntaxNode identity comparisons in src/. Measured over 10 independent cold builds of the tracked source: before 3658 / 3687 / 3695 / 3690 edges, 4 distinct canonical edge digests after 3479 edges, one digest, identical on all 10 runs That delta is 216 `references` edges removed and none added, with every calls/imports/contains/implements edge unchanged. Classified against the AST, 214 were direct-callee occurrences already carried by a `calls` edge with the same source→target, and 2 moved from a file node to the enclosing symbol (src/cli.ts → src/cli.ts#wireTarget, test/ask.test.ts → …#rankOf). The remaining one, test/claude-init-workspace.test.ts → test/helpers.ts#runCli, loses connectivity: `runCli` is defined in four files, so the bare call drops as ambiguous. That is a pre-existing call-resolution limit the false edge was masking, and is left alone here. The regression test makes the bug reproducible rather than occasional: its fixture is bulk direct call sites, and the build runs in a child with a 1 MB young generation so the cold cache the defect needs actually occurs. Against the previous extractor it fails 20/20 on Node 20.20.2 and 20/20 on Node 24.8.0; against this one, 0/20 on both. It also pins what a fix must not break — a value use still yields a reference, `calls` and `references` still coexist when one body does both, and a declaration reusing an imported name is not a use. 660 tests (+1). --- src/graph/extract.ts | 26 ++++++++-- test/graph-references.test.ts | 97 ++++++++++++++++++++++++++++++++++- test/reference-scale-probe.ts | 28 ++++++++++ 3 files changed, 147 insertions(+), 4 deletions(-) create mode 100644 test/reference-scale-probe.ts diff --git a/src/graph/extract.ts b/src/graph/extract.ts index e10fa6f0e..4cd7e9880 100644 --- a/src/graph/extract.ts +++ b/src/graph/extract.ts @@ -470,6 +470,23 @@ function collectTsImportBindings( for (const child of node.namedChildren) collectTsImportBindings(child, specifier, out); } +/** + * Do these two wrappers stand for the same syntax node? `===` does not answer that: + * node-tree-sitter materializes `SyntaxNode` objects on demand and caches them + * weakly, so reaching one node twice can return two different JS objects. Comparing + * wrappers makes a purely syntactic question depend on collector timing — two cold + * builds of unchanged source then disagree on `references` edges (#116). + * + * `id` is the stable identity, unique within one tree, so the tree is compared too. + * A `Tree` is one object per parse (unlike its nodes), so `===` is right for it. + */ +function sameSyntaxNode( + a: Parser.SyntaxNode | null | undefined, + b: Parser.SyntaxNode | null | undefined, +): boolean { + return !!a && !!b && a.tree === b.tree && a.id === b.id; +} + /** * A parameter or local declaration wins over an import inside that function. * Drop that imported binding for the whole function rather than create a false @@ -483,7 +500,7 @@ function withoutShadowedImports( const shadowed = new Set(); const definitionValue = definition.childForFieldName("value"); const visit = (node: Parser.SyntaxNode): void => { - if (node !== definition && node !== definitionValue && isFunctionBoundary(node)) { + if (!sameSyntaxNode(node, definition) && !sameSyntaxNode(node, definitionValue) && isFunctionBoundary(node)) { const name = node.childForFieldName("name"); if (name?.type === "identifier") shadowed.add(name.text); return; @@ -521,13 +538,16 @@ function isFunctionBoundary(node: Parser.SyntaxNode): boolean { function isDirectCallee(node: Parser.SyntaxNode, callTypes: ReadonlySet): boolean { const parent = node.parent; if (!parent || !callTypes.has(parent.type)) return false; - return parent.childForFieldName("function") === node || parent.childForFieldName("name") === node; + return ( + sameSyntaxNode(parent.childForFieldName("function"), node) || + sameSyntaxNode(parent.childForFieldName("name"), node) + ); } /** Definition/declaration identifiers name a new binding; they do not use one. */ function isDeclarationName(node: Parser.SyntaxNode): boolean { const parent = node.parent; - return parent?.childForFieldName("name") === node; + return sameSyntaxNode(parent?.childForFieldName("name"), node); } /** Recognize the definition shapes: mapped node types, Go's type/method forms, and diff --git a/test/graph-references.test.ts b/test/graph-references.test.ts index b3873d3d3..cf6642b9c 100644 --- a/test/graph-references.test.ts +++ b/test/graph-references.test.ts @@ -2,11 +2,17 @@ * Regression coverage for issue #34: imported functions used as values must * remain visible to `callers`, but as weaker `references` edges rather than * being mislabeled as direct calls. + * + * …and for issue #116, at the bottom: the same rules have to hold in a file + * big enough that tree-sitter stops handing back the same JS wrapper object + * for a node it already materialized. */ import { test } from "node:test"; import assert from "node:assert/strict"; import { mkdirSync, writeFileSync } from "node:fs"; -import { join } from "node:path"; +import { spawnSync } from "node:child_process"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; import { buildGraph } from "../src/graph/build.js"; import { callersOf } from "../src/graph/traverse.js"; import { readGraph, wiringPath } from "../src/graph/write.js"; @@ -85,6 +91,95 @@ test("aliased named imports resolve references to the exported symbol", async () ); }); +/** + * Regression for #116. node-tree-sitter may rematerialize SyntaxNode wrappers + * after GC, so wrapper identity cannot reliably classify direct callees. + * + * The probe constrains GC pressure and exercises enough call sites to make the + * failure reproducible while preserving the intended calls/references semantics. + */ +const CALL_SITES = 60; + +test("a direct call is not also recorded as a value reference, at scale (#116)", () => { + const root = tmpRepo("graft-reference-scale-"); + mkdirSync(join(root, "src"), { recursive: true }); + writeFileSync( + join(root, "src", "dep.ts"), + [ + "export function target(): number { return 1; }", + "export function register(fn: () => number): unknown { return fn; }", + "", + ].join("\n"), + ); + writeFileSync( + join(root, "src", "consumer.ts"), + [ + 'import { target, register } from "./dep.js";', + "", + // (b) handed over as a value — a `references` edge + "export function usesValue(): unknown { return register(target); }", + // (c) both, in one body — BOTH edges, from two distinct occurrences + "export function usesBoth(): number { register(target); return target(); }", + "", + // (d) a block-scoped declaration that happens to reuse the imported name. + // It DECLARES a binding, so it is not a use of the import. Module-level, so + // `withoutShadowedImports` (which only runs inside a function or method) is + // not what protects it — `isDeclarationName` is. + "{", + " const target = 0;", + "}", + "", + // (a) the case #116 breaks: every one of these invokes `target` directly, so + // each must produce a `calls` edge and no `references` edge. + ...Array.from( + { length: CALL_SITES }, + (_, i) => `export function callsTarget${i}(): number { return target(); }`, + ), + "", + ].join("\n"), + ); + + const probe = join(dirname(fileURLToPath(import.meta.url)), "reference-scale-probe.ts"); + const run = spawnSync( + process.execPath, + ["--max-semi-space-size=1", "--import", "tsx", probe, root], + { encoding: "utf8" }, + ); + assert.equal(run.status, 0, `probe failed:\n${run.stderr}`); + const line = run.stdout.split("\n").find((l) => l.startsWith("__EDGES__")); + assert.ok(line, `probe printed no edges:\n${run.stdout}\n${run.stderr}`); + const edges: Array<{ source: string; relation: string }> = JSON.parse(line.slice("__EDGES__".length)); + const linked = (from: string, relation: string): boolean => + edges.some((e) => e.source === `src/consumer.ts#${from}` && e.relation === relation); + + // (a) a direct invocation is a call, and ONLY a call — at every one of the sites. + const callSites = (relation: string): number => + edges.filter((e) => /^src\/consumer\.ts#callsTarget\d+$/.test(e.source) && e.relation === relation).length; + assert.equal(callSites("calls"), CALL_SITES, "every direct call resolves to a calls edge"); + assert.equal( + callSites("references"), + 0, + "an identifier that is the direct callee must not also be booked as a value reference", + ); + + // (b) the recall #34/#49 bought must survive any fix to (a). + assert.equal(linked("usesValue", "references"), true, "a function passed as a value is still a reference"); + + // (c) `calls` and `references` may legitimately coexist between the same pair — + // here two different occurrences of `target` in one body. Whatever fixes (a) + // must not become a blanket "references and calls are mutually exclusive" rule. + assert.equal(linked("usesBoth", "calls"), true, "the invoked occurrence yields a calls edge"); + assert.equal(linked("usesBoth", "references"), true, "the value occurrence yields a references edge"); + + // (d) the block-scoped redeclaration is attributed to the file node, since it sits + // outside any definition. It names a binding, so it must not reference the import. + assert.equal( + edges.some((e) => e.source === "src/consumer.ts" && e.relation === "references"), + false, + "a declaration that reuses an imported name is not a use of the import", + ); +}); + test("a local binding that shadows an import does not create a false reference", async () => { const root = tmpRepo("graft-reference-shadow-"); mkdirSync(join(root, "src"), { recursive: true }); diff --git a/test/reference-scale-probe.ts b/test/reference-scale-probe.ts new file mode 100644 index 000000000..41551aa4b --- /dev/null +++ b/test/reference-scale-probe.ts @@ -0,0 +1,28 @@ +/** + * Child process for the #116 regression test in `graph-references.test.ts`. + * + * It exists only so that the build can be run under `--max-semi-space-size=1`. + * #116 is a garbage-collection race: `isDirectCallee`/`isDeclarationName` compare + * tree-sitter `SyntaxNode` objects with `===`, which only answers correctly while + * the node is still in node-tree-sitter's tree cache. Whether a scavenge has + * cleared that cache mid-walk is otherwise pure luck — measured over a sweep of + * fixture sizes, the same input flips between correct and misclassified from one + * process to the next. Shrinking the young generation makes scavenges frequent + * enough that the cache is reliably cold, which turns the race into a fixed + * outcome and the regression test into a deterministic one. + * + * Prints one JSON line: every edge pointing at `src/dep.ts#target`. + */ +import { buildGraph } from "../src/graph/build.js"; +import { readGraph, wiringPath } from "../src/graph/write.js"; +import { join } from "node:path"; + +const root = process.argv[2]; + +await buildGraph(root, { reuse: false }); +const graph = readGraph(wiringPath(join(root, "graft"))); +const edges = (graph?.edges ?? []) + .filter((edge) => edge.target === "src/dep.ts#target") + .map(({ source, relation }) => ({ source, relation })); + +console.log(`__EDGES__${JSON.stringify(edges)}`);