diff --git a/CHANGELOG.md b/CHANGELOG.md index a8adf784a..0cdfbec7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -182,6 +182,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - `codegraph sync` and the file watcher now link a file to a module it imports under a name of its own when the module is added after the file was indexed, comes back after being deleted, or gets the export in a later edit: a default import like `import tagsController from './tag/tag.controller'`, a namespace import like `import * as api from './api'`, or an aliased one like `import { Component as GroupsPageWrapper } from './groups_page'`, in JavaScript, TypeScript, Vue, Svelte and Astro. Before, the import and every use of the name, such as a call, `new`, a base class, a JSX or template tag or an Express `app.use` mount, stayed unlinked until the importing file changed or the project was re-indexed, so the module looked unused and `codegraph affected` missed the files that use it. Re-index after upgrading to pick up links an earlier version missed. - `codegraph sync` and the file watcher now also link a call or read on a name a file imports, like `NODE_PATH_EXCLUDELIST.includes(path)` after `import { NODE_PATH_EXCLUDELIST } from './constants'`, `store.get(key)` through a default import or `api.get()` through a namespace import, when the module gets the export in a later edit, is added after the importing file was indexed, or comes back after being deleted, in JavaScript, TypeScript, Vue, Svelte and Astro. Before, such a call stayed unlinked until the importing file changed or the project was re-indexed, because sync looked it up only by its method's name, which the module seldom declares, and skipped a name as common as `includes`, `get` or `map` altogether. Re-index after upgrading to pick up links an earlier version missed. - `codegraph sync` and the file watcher now also link a call on a variable or parameter declared with a class a file imports, like `cache.get(key)` after `const cache = new LRUCache(10)` or in a function that takes `cache: LRUCache`, when the class's module gets the class in a later edit, is added after the file was indexed, or comes back after being deleted, in JavaScript and TypeScript. Before, such a call stayed unlinked until its own file changed or the project was re-indexed whenever its method's name, like `get`, was too common for sync to look up. Re-index after upgrading to pick up links an earlier version missed. +- `codegraph sync` and the file watcher now link a call that had two candidates once a sync deletes the file holding one of them or renames it away, like `x.go()` with `go` defined in two classes, as a full index does. Before, the call stayed unlinked until its own file changed or the project was re-indexed. Thanks @inth3shadows for the report and the pull request. (#3006) - `codegraph sync` and the file watcher now also link a name a file imports from a barrel module, like `import { useInvocation } from './context'` where `context/index.ts` re-exports it from the file that declares it, when an edit gives the barrel that re-export after the importing file was indexed, in JavaScript, TypeScript, Vue, Svelte and Astro. Before, the import and every use of the name, such as a call or a JSX or template tag, stayed unlinked until the importing file changed or the project was re-indexed, because a barrel declares none of the names it passes on. Re-index after upgrading to pick up links an earlier version missed. - `codegraph sync` and the file watcher now also update the files that import through a barrel module when an edit points one of its re-exports at another module, like `export { foo } from './a'` changed to `'./b'` or an `export *` moved the same way, takes one away, or drops a name from it, in JavaScript and TypeScript. Before, those files kept the names linked to the old module until they changed or the project was re-indexed. Thanks @inth3shadows for the report and @danusha2345. (#2978) - In TypeScript and JavaScript, a call of an array method, like `items.find(…)` or `items.push(…)`, on a variable or parameter annotated as an array of a class (`items: Item[]`) no longer links to the class's method of the same name. Before, the annotation was read as the class itself: in vscode, `distinctElements.find(…)` on an `ExplorerItem[]` linked to `ExplorerItem.find`, and `ranges.map(…)` on an `OffsetRange[]` to `OffsetRange.map`, so those methods showed callers they don't have. A call on an element that shares the array's name, as in `d.forEach(d => d.dispose())`, still links to the class. Re-index TypeScript and JavaScript projects after upgrading. diff --git a/__tests__/sync-removed-competitor.test.ts b/__tests__/sync-removed-competitor.test.ts new file mode 100644 index 000000000..ffb12889a --- /dev/null +++ b/__tests__/sync-removed-competitor.test.ts @@ -0,0 +1,140 @@ +/** + * A call that only failed because a second candidate shared its name resolves + * once a sync removes that candidate. + * + * `x.go()` on a value of unknown type, with `go` defined in two classes, is + * ambiguous: a full index leaves it unresolved. Take one of the two away and a + * full index links the call to the other. Sync didn't: its failed-reference + * retry looked failed refs up by the names in the files it changed, so a + * deleted file (no names left) or an edit that renames the competitor away + * never brought the call up again, and a sync that only deleted files skipped + * the retry altogether. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import * as fs from 'fs'; +import * as path from 'path'; +import * as os from 'os'; +import CodeGraph from '../src/index'; + +/** Every node and edge by natural key, so two indexes of the same files compare directly. */ +function graphOf(cg: CodeGraph): { nodes: string[]; edges: string[] } { + const keys = new Map( + cg.getFiles() + .flatMap((file) => cg.getNodesInFile(file.path)) + .map((n) => [n.id, `${n.kind} ${n.filePath}:${n.qualifiedName}:${n.startLine}`] as const) + ); + const edges = cg.getOutgoingEdgesFrom([...keys.keys()]) + .map((e) => `${e.kind} ${keys.get(e.source)} -> ${keys.get(e.target) ?? e.target}`); + return { nodes: [...keys.values()].sort(), edges: edges.sort() }; +} + +async function reindexFromScratch(cg: CodeGraph): Promise { + const root = cg.getProjectRoot(); + cg.close(); + const rebuilt = await CodeGraph.recreate(root); + await rebuilt.indexAll(); + return rebuilt; +} + +const REINDEX_TIMEOUT = 60_000; + +describe('sync re-resolves a call whose competing candidate went away', () => { + let dir: string; + let cg: CodeGraph; + const write = (rel: string, content: string) => fs.writeFileSync(path.join(dir, rel), content); + + function callTargets(caller: string): string[] { + const node = cg.searchNodes(caller).map((r) => r.node).find((n) => n.name === caller && n.kind === 'function'); + return node ? cg.getCallees(node.id).map((c) => c.node.qualifiedName).sort() : []; + } + + afterEach(() => { + cg?.destroy(); + fs.rmSync(dir, { recursive: true, force: true }); + }); + + describe('Python', () => { + beforeEach(async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-competitor-py-')); + write('a.py', 'class A:\n def go(self):\n return 1\n'); + write('b.py', 'class B:\n def go(self):\n return 2\n'); + write('t.py', 'from a import A\n\ndef make():\n return A()\n\ndef use():\n x = make()\n x.go()\n'); + cg = CodeGraph.initSync(dir); + await cg.indexAll(); + // Two candidates: the call stays unresolved. + expect(callTargets('use')).toEqual(['make']); + }); + + it('after the file holding the other candidate is deleted', async () => { + fs.unlinkSync(path.join(dir, 'b.py')); + const result = await cg.sync(); + expect(result.filesRemoved).toBe(1); + expect(callTargets('use')).toEqual(['A::go', 'make']); + const synced = graphOf(cg); + cg = await reindexFromScratch(cg); + expect(synced).toEqual(graphOf(cg)); + }, REINDEX_TIMEOUT); + + it('after an edit renames the other candidate away', async () => { + write('b.py', 'class B:\n def stop(self):\n return 2\n'); + await cg.sync(); + expect(callTargets('use')).toEqual(['A::go', 'make']); + const synced = graphOf(cg); + cg = await reindexFromScratch(cg); + expect(synced).toEqual(graphOf(cg)); + }, REINDEX_TIMEOUT); + + it('a deletion that leaves the call ambiguous changes nothing', async () => { + write('c.py', 'class C:\n def go(self):\n return 3\n'); + await cg.sync(); + fs.unlinkSync(path.join(dir, 'b.py')); + await cg.sync(); + expect(callTargets('use')).toEqual(['make']); + const synced = graphOf(cg); + cg = await reindexFromScratch(cg); + expect(synced).toEqual(graphOf(cg)); + }, REINDEX_TIMEOUT); + }); + + describe('TypeScript', () => { + beforeEach(async () => { + dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-competitor-ts-')); + write('a.ts', 'export class A {\n go(): number { return 1; }\n}\n'); + write('b.ts', 'export class B {\n go(): number { return 2; }\n}\n'); + write('t.ts', 'export function use(x: any): void {\n x.go();\n}\n'); + cg = CodeGraph.initSync(dir); + await cg.indexAll(); + expect(callTargets('use')).toEqual([]); + }); + + it('after the file holding the other candidate is deleted', async () => { + fs.unlinkSync(path.join(dir, 'b.ts')); + await cg.sync(); + expect(callTargets('use')).toEqual(['A::go']); + const synced = graphOf(cg); + cg = await reindexFromScratch(cg); + expect(synced).toEqual(graphOf(cg)); + }, REINDEX_TIMEOUT); + + it('after an edit renames the other candidate away', async () => { + write('b.ts', 'export class B {\n stop(): number { return 2; }\n}\n'); + await cg.sync(); + expect(callTargets('use')).toEqual(['A::go']); + const synced = graphOf(cg); + cg = await reindexFromScratch(cg); + expect(synced).toEqual(graphOf(cg)); + }, REINDEX_TIMEOUT); + + it('a deletion that leaves the call ambiguous changes nothing', async () => { + write('c.ts', 'export class C {\n go(): number { return 3; }\n}\n'); + await cg.sync(); + fs.unlinkSync(path.join(dir, 'b.ts')); + await cg.sync(); + expect(callTargets('use')).toEqual([]); + const synced = graphOf(cg); + cg = await reindexFromScratch(cg); + expect(synced).toEqual(graphOf(cg)); + }, REINDEX_TIMEOUT); + }); +}); diff --git a/docs/benchmarks/index-drift-cg33.md b/docs/benchmarks/index-drift-cg33.md index 2778beaf0..2c97af8bb 100644 --- a/docs/benchmarks/index-drift-cg33.md +++ b/docs/benchmarks/index-drift-cg33.md @@ -78,7 +78,10 @@ identical graph still picked a different candidate. edges targeting a symbol of that name whose source is in an *unchanged* file, and re-inserts each as the reference that created it (the `metadata.refName` stamp). The existing orphan sweep then resolves them against the post-sync - graph — the same input a rebuild resolves from. Kill switch: + graph — the same input a rebuild resolves from. +- Failed references parked under a delta name go back to pending for the same + sweep: a call that failed only because a second definition shared its name + links once that definition is deleted or renamed away. Kill switch for both: `CODEGRAPH_NO_REBIND=1`. The delta is compared **per file**, not as one name set over the whole batch: a diff --git a/src/index.ts b/src/index.ts index 6275ffe9d..a181cd620 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1526,7 +1526,8 @@ export class CodeGraph { // as a full index resolves. // // `definitionDelta` is empty for a body-only edit, so the overwhelmingly - // common sync pays one branch. CODEGRAPH_NO_REBIND=1 disables it. + // common sync pays one branch. CODEGRAPH_NO_REBIND=1 disables it, and + // the failed-ref re-open that follows the rebind with it. if (result.definitionDelta && process.env.CODEGRAPH_NO_REBIND !== '1') { const tRebind = Date.now(); const rebound = this.orchestrator.resurrectStaleResolutionEdges( @@ -1534,9 +1535,19 @@ export class CodeGraph { result.changedFilePaths ?? [], edgesBeforeRetry ); + // And the failed refs under those names: a call that failed only + // because two definitions shared its name (`x.go()` with `go` in two + // classes) resolves once one of them is gone. The retry above looks + // failed refs up by the names the changed files carry now, which a + // deleted file or a name renamed away no longer has, and a sync that + // only deletes files skips it. The sweep below resolves what this + // re-opens. + const reopenedFailed = this.queries.reopenFailedReferences( + this.queries.getRetryableFailedReferences(result.definitionDelta).flatMap((ref) => (ref.rowId === undefined ? [] : [ref.rowId])) + ); if (process.env.CODEGRAPH_SYNTH_TIMINGS) { console.error( - `[phase-timing] sync-rebind: ${Date.now() - tRebind}ms (${result.definitionDelta.length} changed names, ${rebound} edges re-opened)` + `[phase-timing] sync-rebind: ${Date.now() - tRebind}ms (${result.definitionDelta.length} changed names, ${rebound} edges and ${reopenedFailed} failed refs re-opened)` ); } }