mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-09 03:17:54 +00:00
test(scope-resolution): pin the closure-binding caller-attribution limit (#2693)
Review of this PR found the new callable nodes are call TARGETS but never call SOURCES: a call made INSIDE a closure binding is attributed to the enclosing scope, so `impact(handler, direction:"downstream")` reports nothing even though the closure calls out. Consistent across Kotlin, Dart, Ruby and PHP; TS/JS free bindings are the exception because their arrow carries a @scope.function whose range matches. Not fixed here — pinned, so the boundary is visible instead of surprising, and so a change in EITHER direction fails a test. The cause is precise: `pickCallerCallableDef` (graph-bridge/ids.ts) finds the caller by walking CHILD scopes whose range contains the call site, gated on `child.kind === 'Function'`. A closure literal is a BLOCK scope in these languages (Kotlin deliberately, #1757 smart casts), AND the binding's def is owned by the enclosing scope rather than by the closure's scope — so neither half of the link exists. Fixing it needs "callable boundary" decoupled from scope `kind` plus an association between the closure scope and its binding. That is a change to the caller anchor used by every call in the repo, which is not something to land at the tail of this PR. Also adds a unit suite for `buildGraphTargetIndex` itself, covering what the integration tier cannot isolate: a binding is admitted only on POSITIONAL evidence, a name-only match is rejected, a non-callable node at that position is rejected, an ambiguous position claimed by two callables is rejected, and the PHP dollar sigil normalises across the join while still not matching a same-named function on another line. That last one closes the review's LOW — the node/declaration name asymmetry now has an executable contract rather than resting on a comment.
This commit is contained in:
parent
aeb559b25e
commit
15cd66853c
2 changed files with 167 additions and 0 deletions
|
|
@ -476,6 +476,56 @@ describeIfWorkerBuilt('closure bindings resolve in the remaining languages (#269
|
|||
});
|
||||
});
|
||||
|
||||
describeIfWorkerBuilt('a closure binding is a call TARGET, not yet a call SOURCE', () => {
|
||||
// Known limit, pinned deliberately so it is visible rather than surprising.
|
||||
//
|
||||
// A call made INSIDE a closure binding is attributed to the ENCLOSING scope,
|
||||
// not to the binding's own node — so `impact(handler, direction:"downstream")`
|
||||
// reports nothing even though the closure calls `target`.
|
||||
//
|
||||
// Cause: `pickCallerCallableDef` (graph-bridge/ids.ts) finds the caller by
|
||||
// walking CHILD scopes whose range contains the call site, gated on
|
||||
// `child.kind === 'Function'`. A closure literal is a BLOCK scope in these
|
||||
// languages (Kotlin deliberately, #1757 smart casts), and the binding's def is
|
||||
// owned by the enclosing scope rather than by the closure's scope — so neither
|
||||
// half of the link exists. Fixing it means decoupling "callable boundary" from
|
||||
// scope `kind` AND associating the closure scope with its binding; that is the
|
||||
// orthogonal-scope-attribute work, not a query change.
|
||||
//
|
||||
// TS/JS free bindings are the exception: their arrow has a `@scope.function`
|
||||
// with a matching range, so the closure IS the anchor there. These tests exist
|
||||
// to catch that asymmetry changing in EITHER direction.
|
||||
|
||||
it('Kotlin: a call inside the closure is attributed to the file, not the binding', async () => {
|
||||
const targets = await callEdgeIdsFor(
|
||||
'A.kt',
|
||||
'fun target(x: Int): Int = x\n\nval handler = { x: Int -> target(x) }\n',
|
||||
);
|
||||
|
||||
expect(targets).toEqual(['rel:CALLS:File:A.kt->Function:A.kt:target']);
|
||||
});
|
||||
|
||||
it('PHP: a call inside the closure is attributed to the file, not the binding', async () => {
|
||||
const targets = await callEdgeIdsFor(
|
||||
'a.php',
|
||||
'<?php\nfunction target($x) { return $x; }\n' +
|
||||
'$handler = function ($x) { return target($x); };\n',
|
||||
);
|
||||
|
||||
expect(targets).toEqual(['rel:CALLS:File:a.php->Function:a.php:target']);
|
||||
});
|
||||
|
||||
it('JavaScript: a free arrow binding IS the caller anchor', async () => {
|
||||
// The counter-case: an aligned @scope.function makes the closure the anchor.
|
||||
const targets = await callEdgeIdsFor(
|
||||
'c.js',
|
||||
'export function target(x) { return x; }\nvar handler = (x) => target(x);\n',
|
||||
);
|
||||
|
||||
expect(targets).toEqual(['rel:CALLS:Function:c.js:handler->Function:c.js:target']);
|
||||
});
|
||||
});
|
||||
|
||||
describeIfWorkerBuilt('a value binding is never aliased onto a same-named callable', () => {
|
||||
// These are the regression tests for the defect the first cut of #2693
|
||||
// shipped. Admitting a value binding on a same-file NAME match let
|
||||
|
|
|
|||
|
|
@ -0,0 +1,117 @@
|
|||
/**
|
||||
* #2693 — `buildGraphTargetIndex` admits a VALUE binding as a call target only
|
||||
* on POSITIONAL evidence: the callable graph node at the binding's own file,
|
||||
* line and name.
|
||||
*
|
||||
* The first cut of #2693 admitted a value binding whose *resolved* node was
|
||||
* callable, which let `resolveDefGraphId` fall through to its label-agnostic,
|
||||
* first-write-wins `simpleKey(filePath, simpleName)` and alias the binding onto
|
||||
* ANY same-named callable in the file — a fabricated caller, chosen by
|
||||
* declaration order. These tests pin the property that replaced it, at the unit
|
||||
* level where the integration suite cannot isolate it.
|
||||
*/
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import type { KnowledgeGraph } from '../../../src/core/graph/types.js';
|
||||
import type { ScopeResolutionIndexes } from '../../../src/core/ingestion/model/scope-resolution-indexes.js';
|
||||
import type { GraphNodeLookup } from '../../../src/core/ingestion/scope-resolution/graph-bridge/node-lookup.js';
|
||||
import { buildGraphTargetIndex } from '../../../src/core/ingestion/scope-resolution/passes/callable-value-flow.js';
|
||||
|
||||
interface StubNode {
|
||||
readonly id: string;
|
||||
readonly label: string;
|
||||
readonly properties: { filePath: string; name: string; startLine: number };
|
||||
}
|
||||
|
||||
/** Minimal graph — `buildGraphTargetIndex` reads only `iterNodes`/`getNode`. */
|
||||
const graphOf = (nodes: readonly StubNode[]): KnowledgeGraph => {
|
||||
const byId = new Map(nodes.map((node) => [node.id, node]));
|
||||
return {
|
||||
iterNodes: () => nodes[Symbol.iterator](),
|
||||
getNode: (id: string) => byId.get(id),
|
||||
} as unknown as KnowledgeGraph;
|
||||
};
|
||||
|
||||
/** `line` is 1-based, matching the definition-id convention. */
|
||||
const def = (type: string, filePath: string, qualifiedName: string, line: number) => ({
|
||||
nodeId: `${filePath}#${line}:0:${qualifiedName}`,
|
||||
type,
|
||||
filePath,
|
||||
qualifiedName,
|
||||
});
|
||||
|
||||
const scopesOf = (defs: readonly ReturnType<typeof def>[]): ScopeResolutionIndexes =>
|
||||
({
|
||||
defs: { byId: new Map(defs.map((d) => [d.nodeId, d])) },
|
||||
}) as unknown as ScopeResolutionIndexes;
|
||||
|
||||
/** Graph nodes store a 0-BASED startLine; defs are 1-based. */
|
||||
const node = (label: string, filePath: string, name: string, line: number): StubNode => ({
|
||||
id: `${label}:${filePath}:${name}`,
|
||||
label,
|
||||
properties: { filePath, name, startLine: line - 1 },
|
||||
});
|
||||
|
||||
const targetsFor = (nodes: readonly StubNode[], defs: readonly ReturnType<typeof def>[]) =>
|
||||
[
|
||||
...buildGraphTargetIndex(
|
||||
scopesOf(defs),
|
||||
new Map() as GraphNodeLookup,
|
||||
undefined,
|
||||
graphOf(nodes),
|
||||
),
|
||||
]
|
||||
.map(([defId, target]) => `${defId} => ${target.id}`)
|
||||
.sort();
|
||||
|
||||
describe('buildGraphTargetIndex — value bindings join by position', () => {
|
||||
it('admits a value binding whose callable node sits at its own line', () => {
|
||||
// The #2687 closure-binding shape: the value def and the Function node are
|
||||
// the same construct, so they share file, line and name.
|
||||
expect(
|
||||
targetsFor([node('Function', 'a.kt', 'handler', 3)], [def('Property', 'a.kt', 'handler', 3)]),
|
||||
).toEqual(['a.kt#3:0:handler => Function:a.kt:handler']);
|
||||
});
|
||||
|
||||
it('REJECTS a value binding that merely shares a name with a callable elsewhere', () => {
|
||||
// `const save = …` on line 7 beside an unrelated `save` callable on line 2.
|
||||
// A name-only match admitted this and minted a fabricated caller.
|
||||
expect(
|
||||
targetsFor([node('Function', 'a.ts', 'save', 2)], [def('Variable', 'a.ts', 'save', 7)]),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it('REJECTS a value binding whose node at that position is NOT callable', () => {
|
||||
expect(
|
||||
targetsFor([node('Const', 'a.ts', 'CONFIG', 4)], [def('Const', 'a.ts', 'CONFIG', 4)]),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it('REJECTS an ambiguous position claimed by two callables', () => {
|
||||
// Admitting either would be an arbitrary, order-dependent choice.
|
||||
expect(
|
||||
targetsFor(
|
||||
[node('Function', 'a.ts', 'dup', 5), node('Method', 'a.ts', 'dup', 5)],
|
||||
[def('Variable', 'a.ts', 'dup', 5)],
|
||||
),
|
||||
).toEqual([]);
|
||||
});
|
||||
|
||||
it('normalises the PHP dollar sigil across the join', () => {
|
||||
// The PHP node keeps the sigil so `$save` cannot collide with the function
|
||||
// `save()` — PHP holds the two in separate namespaces — while the scope
|
||||
// declaration drops it. The join must still match them, and must NOT match
|
||||
// the same-named function on another line.
|
||||
expect(
|
||||
targetsFor(
|
||||
[node('Function', 'a.php', '$save', 5), node('Function', 'a.php', 'save', 2)],
|
||||
[def('Variable', 'a.php', 'save', 5)],
|
||||
),
|
||||
).toEqual(['a.php#5:0:save => Function:a.php:$save']);
|
||||
});
|
||||
|
||||
it('keeps ordinary callable defs, which never take the positional path', () => {
|
||||
expect(
|
||||
targetsFor([node('Function', 'a.ts', 'fn', 1)], [def('Function', 'a.ts', 'fn', 1)]),
|
||||
).toEqual(['a.ts#1:0:fn => Function:a.ts:fn']);
|
||||
});
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue