diff --git a/__tests__/graph.test.ts b/__tests__/graph.test.ts index 5379c97c37..c4ac708a45 100644 --- a/__tests__/graph.test.ts +++ b/__tests__/graph.test.ts @@ -612,4 +612,42 @@ describe('Traversal edge-completeness & limits (#1086–#1090)', () => { // The regression: this direct dependency edge used to vanish. expect(sub.edges.some((e) => e.source === 'Q' && e.target === 'P' && e.kind === 'calls')).toBe(true); }); + + // The issue's graph: a→t, b→a, b→t, c→b. Edge order sends the walk to b + // through a first, at the depth limit, before the direct b→t edge (#1974). + const depthNodes = ['t', 'a', 'b', 'c'].map((n) => tNode(n)); + const depthEdges: Edge[] = [ + { source: 'a', target: 't', kind: 'calls', line: 1 }, + { source: 'b', target: 'a', kind: 'calls', line: 2 }, + { source: 'b', target: 't', kind: 'calls', line: 3 }, + { source: 'c', target: 'b', kind: 'calls', line: 4 }, + ]; + + it('getImpactRadius finds a dependent within the depth even when a longer path reaches its parent first (#1974)', () => { + const sub = tGraph(depthNodes, depthEdges).getImpactRadius('t', 2); + // c → b → t is two hops. Pre-fix: b was first reached via a at depth 2 and + // never expanded again, so c was missing. + expect([...sub.nodes.keys()].sort()).toEqual(['a', 'b', 'c', 't']); + expect(sub.edges.some((e) => e.source === 'c' && e.target === 'b')).toBe(true); + // Re-expanding b does not record its incoming edges twice. + const keys = sub.edges.map((e) => `${e.source}>${e.target}:${e.line}`); + expect(new Set(keys).size).toBe(keys.length); + }); + + it('getCallers at depth N finds every caller within N hops, each once (#1974)', () => { + const callers = tGraph(depthNodes, depthEdges).getCallers('t', 2); + expect(callers.map((c) => c.node.id).sort()).toEqual(['a', 'b', 'c']); + }); + + it('getCallees at depth N finds every callee within N hops, each once (#1974)', () => { + // Mirror image: t→a, a→b, t→b, b→c. From t, b is 1 hop and c is 2. + const edges: Edge[] = [ + { source: 't', target: 'a', kind: 'calls', line: 1 }, + { source: 'a', target: 'b', kind: 'calls', line: 2 }, + { source: 't', target: 'b', kind: 'calls', line: 3 }, + { source: 'b', target: 'c', kind: 'calls', line: 4 }, + ]; + const callees = tGraph(depthNodes, edges).getCallees('t', 2); + expect(callees.map((c) => c.node.id).sort()).toEqual(['a', 'b', 'c']); + }); }); diff --git a/src/graph/traversal.ts b/src/graph/traversal.ts index 8d3e7762c9..481d30d7b8 100644 --- a/src/graph/traversal.ts +++ b/src/graph/traversal.ts @@ -251,6 +251,24 @@ export class GraphTraverser { } } + /** + * Depth-limited walks record the SHALLOWEST depth each node was expanded at. + * A node first reached through a longer path at the depth limit is expanded + * again when a shorter path reaches it, so its own dependents within the + * limit are not lost to edge order (#1974). Returns false when the node was + * already expanded at this depth or nearer. + */ + private enterAtDepth(visited: Map, nodeId: string, depth: number): boolean { + if (!this.nearerThanBefore(visited, nodeId, depth)) return false; + visited.set(nodeId, depth); + return true; + } + + private nearerThanBefore(visited: Map, nodeId: string, depth: number): boolean { + const seen = visited.get(nodeId); + return seen === undefined || depth < seen; + } + /** * Find all callers of a function/method * @@ -260,9 +278,9 @@ export class GraphTraverser { */ getCallers(nodeId: string, maxDepth: number = 1): Array<{ node: Node; edge: Edge }> { const result: Array<{ node: Node; edge: Edge }> = []; - const visited = new Set(); + const visited = new Map(); - this.getCallersRecursive(nodeId, maxDepth, 0, result, visited); + this.getCallersRecursive(nodeId, maxDepth, 0, result, visited, new Set([nodeId])); return result; } @@ -272,17 +290,15 @@ export class GraphTraverser { maxDepth: number, currentDepth: number, result: Array<{ node: Node; edge: Edge }>, - visited: Set + visited: Map, + reported: Set ): void { // Mark visited BEFORE the depth check, not after. Folding both into one // guard meant that when `currentDepth >= maxDepth` fired we returned without // marking the node — so a caller reachable from the same parent via two // edges (two call sites, or calls + references) was pushed once per edge, // duplicating it in `result` at the default `maxDepth=1` (#1086). - if (visited.has(nodeId)) { - return; - } - visited.add(nodeId); + if (!this.enterAtDepth(visited, nodeId, currentDepth)) return; if (currentDepth >= maxDepth) { return; } @@ -302,10 +318,12 @@ export class GraphTraverser { for (const edge of incomingEdges) { const callerNode = callerNodes.get(edge.source); - if (callerNode && !visited.has(callerNode.id)) { + if (!callerNode || !this.nearerThanBefore(visited, callerNode.id, currentDepth + 1)) continue; + if (!reported.has(callerNode.id)) { + reported.add(callerNode.id); result.push({ node: callerNode, edge }); - this.getCallersRecursive(callerNode.id, maxDepth, currentDepth + 1, result, visited); } + this.getCallersRecursive(callerNode.id, maxDepth, currentDepth + 1, result, visited, reported); } } @@ -318,9 +336,9 @@ export class GraphTraverser { */ getCallees(nodeId: string, maxDepth: number = 1): Array<{ node: Node; edge: Edge }> { const result: Array<{ node: Node; edge: Edge }> = []; - const visited = new Set(); + const visited = new Map(); - this.getCalleesRecursive(nodeId, maxDepth, 0, result, visited); + this.getCalleesRecursive(nodeId, maxDepth, 0, result, visited, new Set([nodeId])); return result; } @@ -330,15 +348,13 @@ export class GraphTraverser { maxDepth: number, currentDepth: number, result: Array<{ node: Node; edge: Edge }>, - visited: Set + visited: Map, + reported: Set ): void { // Mark visited before the depth check — see getCallersRecursive: the merged // guard dropped the `visited.add` at the depth boundary, duplicating a // callee reached from the same node via two edges at `maxDepth=1` (#1086). - if (visited.has(nodeId)) { - return; - } - visited.add(nodeId); + if (!this.enterAtDepth(visited, nodeId, currentDepth)) return; if (currentDepth >= maxDepth) { return; } @@ -356,10 +372,12 @@ export class GraphTraverser { for (const edge of outgoingEdges) { const calleeNode = calleeNodes.get(edge.target); - if (calleeNode && !visited.has(calleeNode.id)) { + if (!calleeNode || !this.nearerThanBefore(visited, calleeNode.id, currentDepth + 1)) continue; + if (!reported.has(calleeNode.id)) { + reported.add(calleeNode.id); result.push({ node: calleeNode, edge }); - this.getCalleesRecursive(calleeNode.id, maxDepth, currentDepth + 1, result, visited); } + this.getCalleesRecursive(calleeNode.id, maxDepth, currentDepth + 1, result, visited, reported); } } @@ -525,13 +543,13 @@ export class GraphTraverser { const nodes = new Map(); const edges: Edge[] = []; - const visited = new Set(); + const visited = new Map(); // Add focal node nodes.set(focalNode.id, focalNode); // Traverse incoming edges to find all dependents - this.getImpactRecursive(nodeId, maxDepth, 0, nodes, edges, visited); + this.getImpactRecursive(nodeId, maxDepth, 0, nodes, edges, visited, new Set()); return { nodes, @@ -546,19 +564,21 @@ export class GraphTraverser { currentDepth: number, nodes: Map, edges: Edge[], - visited: Set + visited: Map, + expanded: Set ): void { // Mark visited before the depth check so a node collected at the depth // boundary still lands in `visited`. Otherwise it could sit in `nodes` but // not `visited`, and the two loops below — which used different sets to // gate re-processing — would disagree about it (#1089). - if (visited.has(nodeId)) { - return; - } - visited.add(nodeId); + if (!this.enterAtDepth(visited, nodeId, currentDepth)) return; if (currentDepth >= maxDepth) { return; } + // A node re-expanded from a nearer depth re-reads the same edges; record + // them on its first expansion only. + const firstExpansion = !expanded.has(nodeId); + expanded.add(nodeId); // For container nodes (classes, interfaces, structs, etc.), also traverse // into their children so that callers of contained methods appear in impact @@ -571,11 +591,13 @@ export class GraphTraverser { const children = this.queries.getNodesByIds(containsEdges.map((e) => e.target)); for (const edge of containsEdges) { const childNode = children.get(edge.target); - if (childNode && !visited.has(childNode.id)) { - nodes.set(childNode.id, childNode); - edges.push(edge); + if (childNode && this.nearerThanBefore(visited, childNode.id, currentDepth)) { + if (!nodes.has(childNode.id)) { + nodes.set(childNode.id, childNode); + edges.push(edge); + } // Recurse into children at the same depth (they're part of the same symbol) - this.getImpactRecursive(childNode.id, maxDepth, currentDepth, nodes, edges, visited); + this.getImpactRecursive(childNode.id, maxDepth, currentDepth, nodes, edges, visited, expanded); } } } @@ -597,11 +619,11 @@ export class GraphTraverser { // edge collection (`!nodes.has(...)`), so a second incoming edge into a // node already collected via another path was silently dropped from // `edges` even though it's a real dependency (#1089). Each node's incoming - // edges are fetched once (nodes are expanded once), so no edge repeats. - edges.push(edge); - if (!visited.has(sourceNode.id)) { + // edges are recorded on its first expansion only, so no edge repeats. + if (firstExpansion) edges.push(edge); + if (this.nearerThanBefore(visited, sourceNode.id, currentDepth + 1)) { nodes.set(sourceNode.id, sourceNode); - this.getImpactRecursive(sourceNode.id, maxDepth, currentDepth + 1, nodes, edges, visited); + this.getImpactRecursive(sourceNode.id, maxDepth, currentDepth + 1, nodes, edges, visited, expanded); } } }