-
Notifications
You must be signed in to change notification settings - Fork 15
fix: replace fixed-depth directory-proximity check with symmetric distance #1894
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: fix/issue-1768-build-edges-technique-stmt-cache
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -461,6 +461,41 @@ function resolveImportPathJS( | |||||||||||||||||||||||||||||||||||||||
| return normalizePath(path.relative(rootDir, resolved)); | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| /** All ancestor directories of `dir`, starting with `dir` itself, walking up to the root. */ | ||||||||||||||||||||||||||||||||||||||||
| function ancestorChain(dir: string): string[] { | ||||||||||||||||||||||||||||||||||||||||
| const chain = [dir]; | ||||||||||||||||||||||||||||||||||||||||
| let cur = dir; | ||||||||||||||||||||||||||||||||||||||||
| for (;;) { | ||||||||||||||||||||||||||||||||||||||||
| const parent = path.dirname(cur); | ||||||||||||||||||||||||||||||||||||||||
| if (parent === cur) return chain; // reached root ('.', '/', or a drive root) | ||||||||||||||||||||||||||||||||||||||||
| chain.push(parent); | ||||||||||||||||||||||||||||||||||||||||
| cur = parent; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||
| * Directory-tree distance between two directories: hops up from `a` to the | ||||||||||||||||||||||||||||||||||||||||
| * nearest ancestor shared with `b`, plus hops down from there to `b`. | ||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||
| * Symmetric and depth-independent — unlike a fixed-depth equality check | ||||||||||||||||||||||||||||||||||||||||
| * (e.g. comparing `dirname(dirname(a))` to `dirname(dirname(b))`, as this | ||||||||||||||||||||||||||||||||||||||||
| * function used to), this correctly scores both sibling directories (common | ||||||||||||||||||||||||||||||||||||||||
| * parent) and direct ancestor/descendant directories (one nested inside the | ||||||||||||||||||||||||||||||||||||||||
| * other) regardless of how deep either path is. The fixed-depth check only | ||||||||||||||||||||||||||||||||||||||||
| * matched when both files sat at the *same* depth, so e.g. a file in | ||||||||||||||||||||||||||||||||||||||||
| * `graph/algorithms/*.ts` calling a method declared in the shallower | ||||||||||||||||||||||||||||||||||||||||
| * `graph/model.ts` was scored as maximally distant (issue #1769). | ||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||
| function directoryDistance(a: string, b: string): number { | ||||||||||||||||||||||||||||||||||||||||
| const chainA = ancestorChain(a); | ||||||||||||||||||||||||||||||||||||||||
| const chainB = ancestorChain(b); | ||||||||||||||||||||||||||||||||||||||||
| for (let i = 0; i < chainA.length; i++) { | ||||||||||||||||||||||||||||||||||||||||
| const j = chainB.indexOf(chainA[i]!); | ||||||||||||||||||||||||||||||||||||||||
| if (j !== -1) return i + j; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| return Infinity; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+489
to
+497
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| function computeConfidenceJS( | ||||||||||||||||||||||||||||||||||||||||
| callerFile: string, | ||||||||||||||||||||||||||||||||||||||||
| targetFile: string, | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -471,10 +506,10 @@ function computeConfidenceJS( | |||||||||||||||||||||||||||||||||||||||
| if (importedFrom === targetFile) return 1.0; | ||||||||||||||||||||||||||||||||||||||||
| // Workspace-resolved imports get high confidence even across package boundaries | ||||||||||||||||||||||||||||||||||||||||
| if (importedFrom && _workspaceResolvedPaths.has(importedFrom)) return 0.95; | ||||||||||||||||||||||||||||||||||||||||
| if (path.dirname(callerFile) === path.dirname(targetFile)) return 0.7; | ||||||||||||||||||||||||||||||||||||||||
| const callerParent = path.dirname(path.dirname(callerFile)); | ||||||||||||||||||||||||||||||||||||||||
| const targetParent = path.dirname(path.dirname(targetFile)); | ||||||||||||||||||||||||||||||||||||||||
| if (callerParent === targetParent) return 0.5; | ||||||||||||||||||||||||||||||||||||||||
| const dist = directoryDistance(path.dirname(callerFile), path.dirname(targetFile)); | ||||||||||||||||||||||||||||||||||||||||
| if (dist === 0) return 0.7; // same directory | ||||||||||||||||||||||||||||||||||||||||
| if (dist === 1) return 0.6; // direct parent/child directory | ||||||||||||||||||||||||||||||||||||||||
| if (dist === 2) return 0.5; // sibling directories, or a grandparent/grandchild pair | ||||||||||||||||||||||||||||||||||||||||
| return 0.3; | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| /** | ||
| * Regression test for #1769: a function parameter typed with a class name, | ||
| * declared in a *different, less-deeply-nested directory*, must still | ||
| * resolve method calls on that parameter back to the class's declaration. | ||
| * | ||
| * Root cause: `computeConfidenceJS` (and its Rust mirror `compute_confidence`) | ||
| * scored directory proximity using a fixed-depth check — comparing the | ||
| * parent of the caller's directory to the parent of the target's directory. | ||
| * That check only matched when both files sat at the *same* depth, so a | ||
| * subdirectory file (e.g. `graph/algorithms/bfs.ts`) calling a method on a | ||
| * class declared in its direct parent directory (`graph/model.ts`) was | ||
| * scored as maximally distant (0.3) — well below the 0.5 threshold used by | ||
| * the call-edge resolver's typed-method lookup (`resolveViaTypedMethod` in | ||
| * src/domain/graph/resolver/strategy.ts), so the call edge was silently | ||
| * dropped even though the parameter's type annotation correctly resolved | ||
| * `foo: Foo` to `typeMap['foo'] = 'Foo'`. | ||
| * | ||
| * Setup mirrors the real-world shape from src/graph/algorithms/bfs.ts calling | ||
| * src/graph/model.ts: | ||
| * - model.ts (parent directory): `export class Foo { bar(x): number {...} }` | ||
| * - algorithms/consumer.ts (child directory): a function whose PARAMETER is | ||
| * typed `foo: Foo` (via type annotation, not `new Foo()` construction) | ||
| * calls `foo.bar(x)`. | ||
| * | ||
| * Expected: a `calls` edge from `useFoo` to `Foo.bar`, in both engines. | ||
| */ | ||
|
|
||
| import fs from 'node:fs'; | ||
| import os from 'node:os'; | ||
| import path from 'node:path'; | ||
| import Database from 'better-sqlite3'; | ||
| import { afterAll, beforeAll, describe, expect, it } from 'vitest'; | ||
| import { buildGraph } from '../../src/domain/graph/builder.js'; | ||
|
|
||
| const TSCONFIG = JSON.stringify({ | ||
| compilerOptions: { | ||
| target: 'es2022', | ||
| module: 'esnext', | ||
| moduleResolution: 'bundler', | ||
| strict: true, | ||
| }, | ||
| include: ['**/*.ts'], | ||
| }); | ||
|
|
||
| const MODEL_TS = ` | ||
| export class Foo { | ||
| bar(x: number): number { | ||
| return x + 1; | ||
| } | ||
| } | ||
| `; | ||
|
|
||
| // Nested one directory deeper than model.ts — mirrors graph/algorithms/bfs.ts | ||
| // receiving a `graph: CodeGraph` parameter from graph/model.ts. | ||
| const CONSUMER_TS = ` | ||
| import type { Foo } from '../model.js'; | ||
|
|
||
| export function useFoo(foo: Foo, x: number): number { | ||
| return foo.bar(x); | ||
| } | ||
| `; | ||
|
|
||
| let tmpWasm: string; | ||
| let tmpNative: string; | ||
|
|
||
| function writeFixture(dir: string): void { | ||
| fs.writeFileSync(path.join(dir, 'tsconfig.json'), TSCONFIG); | ||
| // Both files live under a shared `graph/` directory — NOT at the fixture | ||
| // root — so their relative-path depths mirror the real src/graph/model.ts | ||
| // vs src/graph/algorithms/bfs.ts shape. Placing model.ts directly at the | ||
| // fixture root would make `path.dirname()` hit its "." fixed point at the | ||
| // same recursion depth for both files, which accidentally masks the #1769 | ||
| // bug (the old fixed-depth check happens to coincide at the filesystem | ||
| // root regardless of the asymmetry it's supposed to detect). | ||
| fs.mkdirSync(path.join(dir, 'graph', 'algorithms'), { recursive: true }); | ||
| fs.writeFileSync(path.join(dir, 'graph', 'model.ts'), MODEL_TS); | ||
| fs.writeFileSync(path.join(dir, 'graph', 'algorithms', 'consumer.ts'), CONSUMER_TS); | ||
| } | ||
|
|
||
| beforeAll(async () => { | ||
| tmpWasm = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1769-wasm-')); | ||
| writeFixture(tmpWasm); | ||
|
|
||
| tmpNative = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-1769-native-')); | ||
| writeFixture(tmpNative); | ||
|
|
||
| await Promise.all([ | ||
| buildGraph(tmpWasm, { incremental: false, skipRegistry: true, engine: 'wasm' }), | ||
| buildGraph(tmpNative, { incremental: false, skipRegistry: true, engine: 'native' }), | ||
| ]); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| fs.rmSync(tmpWasm, { recursive: true, force: true }); | ||
| fs.rmSync(tmpNative, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| function hasCallEdge(dbPath: string, sourceName: string, targetName: string): boolean { | ||
| const db = new Database(dbPath, { readonly: true }); | ||
| try { | ||
| const row = db | ||
| .prepare( | ||
| `SELECT 1 | ||
| FROM edges e | ||
| JOIN nodes n1 ON e.source_id = n1.id | ||
| JOIN nodes n2 ON e.target_id = n2.id | ||
| WHERE e.kind = 'calls' AND n1.name = ? AND n2.name = ?`, | ||
| ) | ||
| .get(sourceName, targetName); | ||
| return row !== undefined; | ||
| } finally { | ||
| db.close(); | ||
| } | ||
| } | ||
|
|
||
| describe('parameter-typed receiver call to a parent-directory class (#1769)', () => { | ||
| it('WASM: resolves foo.bar(x) to Foo.bar across a child->parent directory boundary', () => { | ||
| expect(hasCallEdge(path.join(tmpWasm, '.codegraph', 'graph.db'), 'useFoo', 'Foo.bar')).toBe( | ||
| true, | ||
| ); | ||
| }); | ||
|
|
||
| it('Native: resolves foo.bar(x) to Foo.bar across a child->parent directory boundary', () => { | ||
| expect(hasCallEdge(path.join(tmpNative, '.codegraph', 'graph.db'), 'useFoo', 'Foo.bar')).toBe( | ||
| true, | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chain_b.iter().position(...)does a linear scan for each element ofchain_a. Collectingchain_binto aHashMap<&str, usize>first would bring this to O(n). At typical path depths the cost is negligible, but the Rust engine also runs this on the rayon-parallelised hot path.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!