diff --git a/CHANGELOG.md b/CHANGELOG.md index 56e8d3b39..2e0a6fe8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - In Dart, an annotation is no longer lost when a comment sits between it and the declaration, like `@override` followed by `// ignore: must_call_super`, a `///` comment written below the annotations, or `@riverpod` followed by an `// expect_lint: …` line. Before, the comment hid the annotation, so a provider written that way was missing from the annotation's `callers` and impact. Re-index Dart projects after upgrading. - In Dart, a call written through an import prefix, like `http.get(…)` after `import 'package:http/http.dart' as http;`, or `utils.Listener()` and `kit.Box.create()` through a prefix of your own, now links to what the library imported with that prefix exports, following its `export` chains, `show` and `hide`, and to nothing when that library is a package outside your project. Before, the prefix was taken for a variable, so `http.get(…)` could link to an unrelated `Http` class's `get` method, and calls made through a prefix into your own libraries were missing from `callers` and impact. Re-index Dart projects after upgrading. - `codegraph sync` and the file watcher now link an import to the file it names when that file is added after the importing file was indexed, or comes back after being deleted: `import 'package:app/b.dart';` in Dart, `import './side'` in JavaScript or TypeScript, `#include "net/socket.h"` in C and C++, `require 'inc/db.php'` in PHP, `import pkg.mod` in Python, a Lua `require`, or a C# `using` of a namespace declared later. Before, the import stayed unlinked until the importing file itself changed or the project was re-indexed, so `codegraph affected` and the files listed as using the new file missed it. Re-index after upgrading. +- `codegraph sync` and the file watcher now also link the imports of an added or restored JavaScript or TypeScript file whose name hundreds of other imports share, like a project's own `utils.ts` among `jotai/utils` and `@scope/pkg/utils` imports, or `assert.ts` in a project full of `import * as assert from 'assert'`. Before, a name that common was skipped as noise no single file could answer, so in a large project the file's own `./utils` and `~/settings/utils` imports stayed unlinked until the importing files changed or the project was re-indexed. - `codegraph sync` and the file watcher now link a navigation call to its page when the page or route is added after the call was indexed, or comes back after being removed: `history.push('/login')` and `navigate('/login')` with React Router, `router.push('/login')` and `redirect('/login')` in Next.js, Expo Router, Vue Router, TanStack Router and SvelteKit (`goto('/login')`), and `this.router.navigate(['/login'])` and `routerLink="/login"` in Angular. Before, the call stayed unlinked, or linked to a catch-all or `:id` page that answered it in the meantime, until its own file changed or the project was re-indexed, so the step from that call to the page it opens was missing or wrong. A route whose path changes without its own file changing, like one built from a path constant kept in another file, now also lets go of the calls that named its old path. Re-index after upgrading. - JSON files in a `templates` or `sections` folder are now read as Shopify Liquid only inside a Shopify theme, where the folder that contains `templates` or `sections` also has `layout/theme.liquid` or `config/settings_schema.json`. Before, all such JSON was, so a .NET project template's `template.json`, a schematic or a CMS's content showed up in the index as an empty Liquid file, and one that lists `sections` could be linked to an unrelated Liquid file of the same name. Shopify themes, including one kept in a subfolder of a repository, are indexed as before, and the next sync removes JSON that an earlier version indexed this way. - In a repository that holds more than one Shopify theme, a theme's `{% render %}`, `{% include %}` and `{% section %}` tags and the sections its JSON templates list now link only to that theme's own snippets and sections, which is where Shopify looks for them. Before, a section or snippet the theme doesn't have was linked to another theme's file of the same name, and a theme kept in a subfolder could link to the root theme's copy instead of its own. A reference the theme can't satisfy now links to nothing. Projects with a single theme are indexed as before. Re-index projects with several Shopify themes after upgrading. diff --git a/__tests__/sync-import-retry-crowded-key.test.ts b/__tests__/sync-import-retry-crowded-key.test.ts new file mode 100644 index 000000000..d315aa5f1 --- /dev/null +++ b/__tests__/sync-import-retry-crowded-key.test.ts @@ -0,0 +1,129 @@ +/** + * A sync links an import of a file that appears later, even when the key the + * import waits under is one a popular import shares. + * + * Sync looks a failed import up by what an added file can be imported as — its + * name, its stem, its folder — and skips a key more than 500 failed imports + * share, as noise one file cannot answer. But a path import is parked under + * its path's stem, which a package's imports share with every file of that + * name: `utils` is the stem of every `jotai/utils` and `@scope/pkg/utils` as + * much as of a project's own `./utils`, and `assert` the name of every + * binding of `import * as assert from 'assert'`. In a large project those + * crowded the key past the ceiling, so the project's own `utils.ts`, added + * after its importers were indexed or restored after a delete, never linked + * until the importers changed or the project was re-indexed. + */ +import { describe, it, expect, afterEach } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import CodeGraph from '../src/index'; + +interface Case { + name: string; + initial: Record; + added: Record; + /** `imports` edges the sync must make: ` -> `. */ + expected: string[]; +} + +/** One file importing `count` package modules whose paths all end in `/utils`. */ +function packageUtils(count: number): string { + return Array.from({ length: count }, (_, i) => `import 'kit${i}/utils';\n`).join(''); +} + +const CASES: Case[] = [ + { + name: "a project's utils.ts among package subpaths named utils", + initial: { + 'package.json': JSON.stringify({ name: 'app', dependencies: Object.fromEntries(Array.from({ length: 520 }, (_, i) => [`kit${i}`, '1.0.0'])) }), + 'src/vendor.ts': packageUtils(520), + 'src/settings/view.ts': "import { format } from './utils';\nexport const view = () => format();\n", + 'src/settings/panel/list.ts': "import { format } from '../utils';\nexport const list = () => format();\n", + }, + added: { 'src/settings/utils.ts': "export function format() { return ''; }\n" }, + expected: [ + 'src/settings/view.ts -> file src/settings/utils.ts', + 'src/settings/panel/list.ts -> file src/settings/utils.ts', + ], + }, + { + name: "a project's assert.ts among bindings of the assert package", + initial: { + 'package.json': JSON.stringify({ name: 'app', devDependencies: { assert: '2.0.0' } }), + // Each statement is a module reference to the package and a binding, + // both waiting under `assert`: 520 in all. + 'test/all.test.ts': Array.from({ length: 260 }, () => "import * as assert from 'assert';\n").join('') + 'assert.ok(true);\n', + 'src/common/strings.ts': "import { ok } from './assert';\nexport const check = (s: string) => ok(s.length > 0);\n", + }, + added: { 'src/common/assert.ts': 'export function ok(value: boolean) { return value; }\n' }, + expected: ['src/common/strings.ts -> file src/common/assert.ts'], + }, + { + // bitwarden imports its `account.service` from 724 files: not noise at all. + name: 'a file more than 500 imports name', + initial: Object.fromEntries(Array.from({ length: 510 }, (_, i) => [ + `src/features/f${i}.ts`, + `import { format } from '../shared/utils';\nexport const f${i} = () => format();\n`, + ])), + added: { 'src/shared/utils.ts': "export function format() { return ''; }\n" }, + expected: ['src/features/f0.ts -> file src/shared/utils.ts', 'src/features/f509.ts -> file src/shared/utils.ts'], + }, +]; + +const roots: string[] = []; +const graphs: CodeGraph[] = []; + +afterEach(() => { + // Close before removing: a held database makes the unlink EBUSY on Windows. + for (const graph of graphs.splice(0)) graph.destroy(); + for (const dir of roots.splice(0)) fs.rmSync(dir, { recursive: true, force: true }); +}); + +function tempRoot(): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-crowded-key-')); + roots.push(dir); + return dir; +} + +function write(root: string, files: Record): void { + for (const [rel, content] of Object.entries(files)) { + fs.mkdirSync(path.dirname(path.join(root, rel)), { recursive: true }); + fs.writeFileSync(path.join(root, rel), content); + } +} + +/** Every `imports` edge between two files of the project: ` -> `. */ +function fileImports(graph: CodeGraph): string[] { + const edges = new Set(); + for (const file of graph.getFiles()) { + const ids = graph.getNodesInFile(file.path).map((n) => n.id); + for (const edge of graph.getOutgoingEdgesFrom(ids, ['imports'])) { + const target = graph.getNode(edge.target); + if (target && target.filePath !== file.path) edges.add(`${file.path} -> ${target.kind} ${target.filePath}`); + } + } + return [...edges].sort(); +} + +describe('sync links an import under a key a popular import crowds', () => { + it.each(CASES)('$name', async (c) => { + const root = tempRoot(); + write(root, c.initial); + const graph = await CodeGraph.init(root, { index: true }); + graphs.push(graph); + for (const edge of c.expected) expect(fileImports(graph)).not.toContain(edge); + + write(root, c.added); + expect((await graph.sync()).filesAdded).toBe(Object.keys(c.added).length); + const synced = fileImports(graph); + expect(synced).toEqual(expect.arrayContaining(c.expected)); + + // A fresh index of the same tree, in a folder of its own, links the same. + const fresh = tempRoot(); + write(fresh, { ...c.initial, ...c.added }); + const oracle = await CodeGraph.init(fresh, { index: true }); + graphs.push(oracle); + expect(synced).toEqual(fileImports(oracle)); + }, 120_000); +}); diff --git a/__tests__/sync-import-retry.test.ts b/__tests__/sync-import-retry.test.ts index 1d93a9a57..d5e7b47e0 100644 --- a/__tests__/sync-import-retry.test.ts +++ b/__tests__/sync-import-retry.test.ts @@ -362,6 +362,20 @@ describe('getRetryableFailedImports', () => { expect(names(queries.getRetryableFailedImports(['b', 'b.dart'], [], 2))).toEqual(['b.dart']); expect(queries.getRetryableFailedImports(['b'], [], 3)).toHaveLength(3); }); + + it('looks a key past the ceiling through for the imports that could name an added file', () => { + const queries = setup(); + const only = (...accepted: string[]) => (ref: { referenceName: string }) => accepted.includes(ref.referenceName); + // Three imports wait under `b`, one more than the ceiling. + expect(names(queries.getRetryableFailedImports(['b'], [], 2, [], undefined, only('../b.dart')))).toEqual(['../b.dart']); + // What the test accepts is no noise: the module ceiling holds it. + expect(names(queries.getRetryableFailedImports(['b'], [], 1, [], 2, only('../b.dart', 'src/b.dart')))).toEqual(['../b.dart', 'src/b.dart']); + expect(queries.getRetryableFailedImports(['b'], [], 1, [], 1, only('../b.dart', 'src/b.dart'))).toEqual([]); + // A key within the ceiling is retried whole, whatever the test says. + expect(queries.getRetryableFailedImports(['b'], [], 3, [], undefined, only())).toHaveLength(3); + // A namespace waits for its node, not a file: the test is not asked. + expect(names(queries.getRetryableFailedImports([], ['Foo.Bar'], 0, [], undefined, only('Foo.Bar')))).toEqual([]); + }); }); describe('schema v12', () => { @@ -420,6 +434,9 @@ describe('schema v12', () => { const prepare = vi.spyOn(db!.getDb(), 'prepare'); // The fixture's tails are old ones: `dart` finds a row by tail, `c.h` by name. expect(queries.getRetryableFailedImports(['dart', 'c.h'], ['Foo.Bar'])).toHaveLength(2); + // A key past the ceiling is read whole for `couldName` to look through: at + // a ceiling of 0 both keys are, and what it accepts is retried. + expect(queries.getRetryableFailedImports(['dart', 'c.h'], [], 0, [], undefined, () => true)).toHaveLength(2); const lookups = prepare.mock.calls.map(([sql]) => sql as string).filter((sql) => sql.includes("reference_kind = 'imports'")); prepare.mockRestore(); expect(lookups.length).toBeGreaterThan(0); diff --git a/src/db/queries.ts b/src/db/queries.ts index fd8367f17..bfb9ed751 100644 --- a/src/db/queries.ts +++ b/src/db/queries.ts @@ -4126,6 +4126,13 @@ export class QueryBuilder { * The other lookups share the per-name ceiling of * {@link getRetryableFailedReferences}, counted per key; this one has the * module ceiling, since a specifier that resolves to the file is no noise. + * + * A path key past the ceiling is not all noise, though: `utils` is the stem + * of a project's own `./utils` and of every `jotai/utils` and + * `@scope/pkg/utils` alike, and `assert` the name of every binding of + * `import * as assert from 'assert'`. Such a key is retried for the + * imports `couldName` accepts, the ones that could name a file the sync + * added: no noise either, so within the module ceiling. */ getRetryableFailedImports( pathKeys: string[], @@ -4133,21 +4140,34 @@ export class QueryBuilder { perNameCeiling: number = 500, resolvedModules: string[] = [], perModuleCeiling: number = 5000, + couldName?: (ref: UnresolvedReference) => boolean, ): UnresolvedReference[] { const failedImports = "status = 'failed' AND reference_kind = 'imports'"; const lookups = [ - { keys: pathKeys, column: 'name_tail', where: failedImports, ceiling: perNameCeiling }, - { keys: pathKeys, column: 'reference_name', where: failedImports, ceiling: perNameCeiling }, + { keys: pathKeys, column: 'name_tail', where: failedImports, ceiling: perNameCeiling, couldName }, + { keys: pathKeys, column: 'reference_name', where: failedImports, ceiling: perNameCeiling, couldName }, { keys: names, column: 'reference_name', where: `${failedImports} AND name_tail != reference_name`, ceiling: perNameCeiling }, // A module specifier that now resolves to an added file: no noise, so // the module ceiling, as for a module key. { keys: resolvedModules, column: 'reference_name', where: failedImports, ceiling: perModuleCeiling }, ]; + const toReference = (row: UnresolvedRefRow): UnresolvedReference => ({ + fromNodeId: row.from_node_id, + referenceName: row.reference_name, + referenceKind: row.reference_kind as EdgeKind, + line: row.line, + column: row.col, + candidates: row.candidates ? safeJsonParse(row.candidates, undefined) : undefined, + filePath: row.file_path, + language: row.language as Language, + rowId: row.id, + }); const rows = new Map(); - for (const { keys, column, where, ceiling } of lookups) { + for (const { keys, column, where, ceiling, couldName } of lookups) { const unique = [...new Set(keys)].filter((key) => key.length > 0); // Pass 1: per-key counts, chunked under the SQLite parameter limit. const retryKeys: string[] = []; + const crowdedKeys: string[] = []; for (let i = 0; i < unique.length; i += SQLITE_PARAM_CHUNK_SIZE) { const chunk = unique.slice(i, i + SQLITE_PARAM_CHUNK_SIZE); const placeholders = chunk.map(() => '?').join(','); @@ -4156,6 +4176,7 @@ export class QueryBuilder { .all(...chunk) as Array<{ key: string; count: number }>; for (const row of counts) { if (row.count <= ceiling) retryKeys.push(row.key); + else if (couldName) crowdedKeys.push(row.key); } } // Pass 2: load the surviving rows; a row two lookups find is kept once. @@ -4167,19 +4188,18 @@ export class QueryBuilder { .all(...chunk) as UnresolvedRefRow[]; for (const row of chunkRows) rows.set(row.id, row); } + // A key past the ceiling: only the imports that could name an added file. + for (const key of crowdedKeys) { + const keyRows = this.db + .prepare(`SELECT * FROM unresolved_refs WHERE ${where} AND ${column} IN (?)`) + .all(key) as UnresolvedRefRow[]; + const named = keyRows.filter((row) => couldName!(toReference(row))); + if (named.length > perModuleCeiling) continue; + for (const row of named) rows.set(row.id, row); + } } - return [...rows.values()].map((row) => ({ - fromNodeId: row.from_node_id, - referenceName: row.reference_name, - referenceKind: row.reference_kind as EdgeKind, - line: row.line, - column: row.col, - candidates: row.candidates ? safeJsonParse(row.candidates, undefined) : undefined, - filePath: row.file_path, - language: row.language as Language, - rowId: row.id, - })); + return [...rows.values()].map(toReference); } /** diff --git a/src/index.ts b/src/index.ts index 598143518..b27d5847a 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1078,11 +1078,16 @@ export class CodeGraph { .filter((ref) => ref.filePath !== undefined && ref.language !== undefined && resolvesToAdded(ref.referenceName, ref.filePath, ref.language)); const retryRows = new Set(retryable.map((ref) => ref.rowId)); + // A key a popular import shares (`utils`, `react`, `assert`) is + // past the ceiling, and is still looked through for the imports + // that could name an added file. const importRetry = this.queries.getRetryableFailedImports( (result.addedFilePaths ?? []).flatMap(importPathKeys), this.queries.getNodeNamesByFiles(result.changedFilePaths, ['namespace', 'module']), undefined, - resolvedModules + resolvedModules, + undefined, + this.resolver.couldNameAnyOf([...addedFiles]) ); for (const ref of [...importRetry, ...folderImports]) { if (!retryRows.has(ref.rowId)) { diff --git a/src/resolution/import-resolver.ts b/src/resolution/import-resolver.ts index 0bced5dd5..23c6522f7 100644 --- a/src/resolution/import-resolver.ts +++ b/src/resolution/import-resolver.ts @@ -520,7 +520,7 @@ const C_CPP_STDLIB_HEADERS = new Set([ * same syntax, and leaving them out made `isExternalImport` answer "not * external" for every npm specifier in an SFC. */ -const ESM_IMPORT_LANGUAGES = new Set([ +export const ESM_IMPORT_LANGUAGES: ReadonlySet = new Set([ 'typescript', 'tsx', 'javascript', 'jsx', 'arkts', 'svelte', 'vue', 'astro', ]); @@ -1841,6 +1841,19 @@ function isJsBareModuleRef(ref: UnresolvedRef, context: ResolutionContext): bool return imports.some((m) => m.source === ref.referenceName) && !imports.some((m) => m.localName === ref.referenceName); } +/** + * The file a JS/TS module reference names, the way resolveViaImport reads it: + * a path (`../lib/a`, `@/lib/x`), a scoped package a `tsconfig` path or a + * workspace member maps to the project, or a bare module under a `baseUrl`. + * Null for any other reference, and for one that names no file of the project + * but its own. + */ +export function jsModuleFile(ref: UnresolvedRef, context: ResolutionContext): string | null { + if (!isJsPathImportRef(ref) && !isJsBareModuleRef(ref, context)) return null; + const file = resolveImportPath(ref.referenceName, ref.filePath, ref.language, context); + return file && file !== ref.filePath ? file : null; +} + /** PHP reference kinds whose name is a class name, written as in the source. */ const PHP_CLASS_NAME_REFS: ReadonlySet = new Set(['instantiates', 'extends', 'implements', 'references']); @@ -1917,9 +1930,9 @@ export function resolveViaImport( // found the way the runtime finds it: extensions, `index` files, path // aliases. Matching the basename instead missed every extensionless one // (the common spelling) and could land on a same-named file elsewhere. - if (isJsPathImportRef(ref) || isJsBareModuleRef(ref, context)) { - const file = resolveImportPath(ref.referenceName, ref.filePath, ref.language, context); - const fileNode = file && file !== ref.filePath ? context.getNodesInFile(file).find((n) => n.kind === 'file') : undefined; + const moduleFile = jsModuleFile(ref, context); + if (moduleFile) { + const fileNode = context.getNodesInFile(moduleFile).find((n) => n.kind === 'file'); if (fileNode) return { original: ref, targetNodeId: fileNode.id, confidence: 0.9, resolvedBy: 'import' }; } // A Dart `import` / `export` URI names a library file, and a `part` URI one diff --git a/src/resolution/index.ts b/src/resolution/index.ts index d6b4f2348..a6aeb6f2f 100644 --- a/src/resolution/index.ts +++ b/src/resolution/index.ts @@ -34,7 +34,7 @@ import { clearCppTypeAliasMemos } from './cpp-type-aliases'; import { clearCppIncluderMemos } from './cpp-includers'; import { clearCFeatureVariantMemos } from './c-feature-variants'; import { matchShopifyThemeFile } from './shopify-themes'; -import { resolveViaImport, resolvePhpImportedStaticCall, resolvePhpQualifiedClassRef, resolveJvmImport, extractImportMappings, extractReExports, loadCppIncludeDirs, isPhpIncludePathRef, isCobolCopybookRef, isNixPathImportRef, isDartImportRef, isLuaRequireRef, isJsPathImportRef, isBoundToOutOfRepoImport, importBindingTail, clearImportResolverMemos, resolveImportPath, isExternalImport } from './import-resolver'; +import { resolveViaImport, resolvePhpImportedStaticCall, resolvePhpQualifiedClassRef, resolveJvmImport, extractImportMappings, extractReExports, loadCppIncludeDirs, isPhpIncludePathRef, isCobolCopybookRef, isNixPathImportRef, isDartImportRef, isLuaRequireRef, isJsPathImportRef, isBoundToOutOfRepoImport, importBindingTail, clearImportResolverMemos, resolveImportPath, isExternalImport, ESM_IMPORT_LANGUAGES, jsModuleFile } from './import-resolver'; import { ResolverPool, minRefsForPool, shouldEngageAdaptively } from './resolver-pool'; import { resolveAliasBinding } from './alias-binding'; import { detectFrameworks, getResolvingFrameworks } from './frameworks'; @@ -2743,6 +2743,57 @@ export class ReferenceResolver { return this.queries.reopenFailedReferences(rowIds) + refs.length; } + /** + * Whether a failed import could name one of `files`, the files a sync just + * added: the test the import retry puts a crowded key's imports to + * (`QueryBuilder.getRetryableFailedImports`). `utils` is the path stem of a + * project's own `./utils` and of every `jotai/utils` and `@scope/pkg/utils` + * import alike, and `assert` the name of every binding of `import * as + * assert from 'assert'`, so past the retry's ceiling a key holds the few + * imports an added `utils.ts` or `assert.ts` is for among hundreds that no + * file of the project is. + * + * A JavaScript or TypeScript import reaches a file by the module it is + * written as (a path, a tsconfig alias or a workspace package, an `index` + * file and the emitted `.js` of a `.ts` included), by a framework's alias + * (SvelteKit's `$lib/…`) or by the file's own name (`…/utils.js`). One that + * none of those lead to an added file — a package, an import binding, a + * workspace package's subpath only its `exports` map resolves — names none + * of them. Of an import written in another language this knows nothing, so + * a crowded key skips it, as it always has. + */ + couldNameAnyOf(files: readonly string[]): (reference: UnresolvedReference) => boolean { + const added = new Set(files); + const names = new Set(files.map((file) => file.slice(file.lastIndexOf('/') + 1))); + return (reference) => { + const language = reference.language || this.getLanguageFromNodeId(reference.fromNodeId); + if (!ESM_IMPORT_LANGUAGES.has(language)) return false; + const ref: UnresolvedRef = { + fromNodeId: reference.fromNodeId, + referenceName: reference.referenceName, + referenceKind: reference.referenceKind, + line: reference.line, + column: reference.column, + filePath: reference.filePath || this.getFilePathFromNodeId(reference.fromNodeId), + language, + rowId: reference.rowId, + }; + const name = ref.referenceName; + if (names.has(name.slice(name.lastIndexOf('/') + 1))) return true; + // Where the specifier leads first: a package's leads nowhere, and only + // then is it worth asking whether the resolver reads the reference as a + // module, which for a bare name means reading the file's imports. + const target = resolveImportPath(name, ref.filePath, language, this.context); + if (target !== null && added.has(target) && jsModuleFile(ref, this.context) === target) return true; + // A framework's alias is a path (`$lib/x`, `~/x`); a bare name, which + // every binding is, is no alias, and asking reads the file. + return name.includes('/') && this.frameworksFor(language).some((framework) => { + const result = framework.resolve(ref, this.context); + return result !== null && added.has(this.nodeById(result.targetNodeId)?.filePath ?? ''); + }); + }; + } + /** * True when `receiver` is a local name bound by an import that resolves to a * file IN THIS PROJECT — the only case where letting a python