diff --git a/CHANGELOG.md b/CHANGELOG.md index eb4add53d..98020a127 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,6 +81,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. +- 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. - `codegraph sync` and the file watcher now do the same for a Python import under a name of its own, like `from .models import User as UserModel`, `from . import helpers as h` or `from app.pkg import sub as subpkg`: when the module or package is added after the importing file was indexed, comes back after being deleted, or gets the imported name in a later edit, the import and every use of the alias, such as a call, a base class or the class passed as a value, now link to it. Before, they stayed unlinked until the importing file changed or the project was re-indexed. Re-index Python projects after upgrading to pick up links an earlier version missed. - `codegraph sync` and the file watcher now also link a Python import of a name a package re-exports, like `from fastapi import FastAPI` through `fastapi/__init__.py`, when that `__init__.py` is added after the importing files were indexed or gains the re-export in a later edit. Before, calls through the name stayed unlinked until each importing file changed or the project was re-indexed. Large projects gain the most: sync also used to skip, in any language, a module or package that more than 500 references were waiting for, and now restoring Django's `django/urls/__init__.py` relinks every file that imports `path` from it. Re-index Python projects after upgrading to pick up links an earlier version missed. - `codegraph sync` and the file watcher now update the files that import through a module that re-exports what it imports, like a package's `__init__.py` or an `index.ts` barrel, when that module is deleted or added, and in Python also the files that reach it through another package re-exporting it. Before, deleting `shop/__init__.py` left every `from shop import Order` linked to the class it re-exported, and deleting `components/index.ts` left every `import { Button } from './components'` linked, until each importing file changed or the project was re-indexed. When such a module came back, the imports stayed linked the way they were while it was missing, and in a large Python project an import of a module whose name is as common as `models` or `test`, like `from django.test import TestCase`, stayed unlinked. Python, JavaScript, TypeScript, Vue, Svelte and Astro. diff --git a/__tests__/sync-typed-local-member-retry.test.ts b/__tests__/sync-typed-local-member-retry.test.ts index 2323c0b97..a49a184d8 100644 --- a/__tests__/sync-typed-local-member-retry.test.ts +++ b/__tests__/sync-typed-local-member-retry.test.ts @@ -35,6 +35,10 @@ const CALLERS: Files = { 'src/use-param.ts': "import { LRUCache } from './lru-cache';\nexport function c(cache: LRUCache) {\n return cache.get('k');\n}\n", 'src/use-namespace.ts': "import * as lib from './lru-cache';\nexport function g() {\n const cache = new lib.LRUCache(3);\n return cache.get('k');\n}\n", 'src/use-default.ts': "import Store from './store';\nexport function e() {\n const s = new Store();\n return s.get();\n}\n", + // A call on an element that shares the array's name: the element's class. + 'src/use-each.ts': "import { LRUCache } from './lru-cache';\nexport function h(cs: LRUCache[]) {\n cs.forEach((cs) => cs.get('k'));\n}\n", + // One of the class's properties, which resolution reads as the class. + 'src/use-prop.ts': "import { LRUCache } from './lru-cache';\nexport function i(max: LRUCache['max']) {\n return max.toFixed(0);\n}\n", 'src/noise.ts': NOISE, }; @@ -50,6 +54,7 @@ const LINKS = [ 'cache.get: calls src/use-new.ts -> method src/lru-cache.ts::LRUCache::get', 'cache.get: calls src/use-param.ts -> method src/lru-cache.ts::LRUCache::get', 'cache.set: calls src/use-new.ts -> method src/lru-cache.ts::LRUCache::set', + 'cs.get: calls src/use-each.ts -> method src/lru-cache.ts::LRUCache::get', 's.get: calls src/use-default.ts -> method src/store.ts::Store::get', ]; @@ -171,9 +176,9 @@ describe('a failed member call on a typed local', () => { // A local of a built-in type, of a package's, of a type the file // declares itself and one a factory returns: none waits for a module of // the project. Nor does a local or a parameter declared as an array of - // an imported class, whose calls are the array's, or a method of `this` - // passed as a value, which resolution looks up in the enclosing class, - // whatever `this` is declared as. + // an imported class, whose array methods are the array's, or a method of + // `this` passed as a value, which resolution looks up in the enclosing + // class, whatever `this` is declared as. 'src/other.ts': "import { createCache, LRUCache } from './factory';\n" + "import QuickLRU from 'quick-lru';\n" + @@ -212,6 +217,9 @@ describe('a failed member call on a typed local', () => { expect(tails('src/use-param.ts', 'cache.get')).toEqual(['module:lru-cache']); expect(tails('src/use-namespace.ts', 'cache.get')).toEqual(['module:lru-cache']); expect(tails('src/use-default.ts', 's.get')).toEqual(['module:store']); + expect(tails('src/use-each.ts', 'cs.get')).toEqual(['module:lru-cache']); + expect(tails('src/use-each.ts', 'cs.forEach')).toEqual(['forEach']); + expect(tails('src/use-prop.ts', 'max.toFixed')).toEqual(['module:lru-cache']); expect(tails('src/other.ts', 'seen.get')).toEqual(['get']); expect(tails('src/other.ts', 'pkg.get')).toEqual(['get']); diff --git a/__tests__/ts-array-annotated-receivers.test.ts b/__tests__/ts-array-annotated-receivers.test.ts new file mode 100644 index 000000000..70982b968 --- /dev/null +++ b/__tests__/ts-array-annotated-receivers.test.ts @@ -0,0 +1,135 @@ +/** + * A call on a TypeScript local or parameter annotated as an array of a class + * is the array's method, not the class's. + * + * `items: Item[]` was read as an `Item`, because the annotation's capture + * stopped at `[`, so `items.find(…)` linked to `Item.find` whenever the class + * declares a method of that name: on vscode, `distinctElements.find(…)` on + * an `ExplorerItem[]` reached `ExplorerItem.find`, and `ranges.map(…)` on an + * `OffsetRange[]` reached `OffsetRange.map`. A method arrays don't have can + * only be called on an element that shares the array's name, as in + * `d.forEach(d => d.dispose())`, and that one keeps the element's type. + */ +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'; + +let roots: string[] = []; +let graphs: CodeGraph[] = []; + +afterEach(() => { + for (const graph of graphs) graph.close(); + graphs = []; + for (const root of roots) fs.rmSync(root, { recursive: true, force: true }); + roots = []; +}); + +/** Index `files` and return each call edge as ` -> `. */ +async function callsOf(files: Record): Promise { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-ts-array-recv-')); + roots.push(root); + for (const [file, text] of Object.entries(files)) { + fs.mkdirSync(path.dirname(path.join(root, file)), { recursive: true }); + fs.writeFileSync(path.join(root, file), text); + } + const graph = await CodeGraph.init(root, { index: true }); + graphs.push(graph); + const nodes = graph.getFiles().flatMap((f) => graph.getNodesInFile(f.path)); + const byId = new Map(nodes.map((n) => [n.id, n])); + return graph.getOutgoingEdgesFrom(nodes.map((n) => n.id), ['calls']) + .map((e) => `${e.metadata?.refName} -> ${byId.get(e.target)?.qualifiedName}`) + .sort(); +} + +const CLASSES = { + 'package.json': JSON.stringify({ name: 'app' }), + 'src/item.ts': + 'export class Item {\n' + + ' find(key: string) { return key; }\n' + + ' push(value: number) { return value; }\n' + + ' flush() { return 0; }\n' + + '}\n' + + 'export class Repository {\n' + + ' save(value: T) { return value; }\n' + + ' find(id: string) { return id; }\n' + + '}\n', + // The one project method named `findLast`, on an unrelated class. + 'src/stack.ts': 'export class Stack {\n findLast() { return 1; }\n}\n', +}; + +describe('a call on a TypeScript local annotated as an array of a class', () => { + it('links to no method of the element class', async () => { + const calls = await callsOf({ + ...CLASSES, + 'src/use.ts': + "import { Item } from './item';\n" + + 'export function first(items: Item[]) {\n' + + " return items.find((i) => i !== undefined);\n" + + '}\n' + + 'export function fill() {\n' + + ' const list: Item[] = [];\n' + + ' list.push(new Item());\n' + + ' return list;\n' + + '}\n' + + 'export function spaced(all: Item [], grid: Item[][]) {\n' + + " all.find((i) => i !== undefined);\n" + + ' return grid.push([]);\n' + + '}\n', + }); + expect(calls.filter((c) => /^(items|list|all|grid)\./.test(c))).toEqual([]); + }, 60_000); + + it("is not matched to another class's method by the call's name instead", async () => { + const calls = await callsOf({ + ...CLASSES, + 'src/use.ts': + "import { Item } from './item';\n" + + 'export function last(all: Item[]) {\n' + + ' return all.findLast((i) => i !== undefined);\n' + + '}\n', + }); + expect(calls.filter((c) => c.startsWith('all.'))).toEqual([]); + }, 60_000); + + it('does the same for an array of a generic class', async () => { + const calls = await callsOf({ + ...CLASSES, + 'src/use.ts': + "import { Repository } from './item';\n" + + 'export function saveAll(repos: Repository[], value: string) {\n' + + ' repos.forEach((r) => r.save(value));\n' + + " return repos.find((r) => r !== undefined);\n" + + '}\n', + }); + expect(calls.filter((c) => c.startsWith('repos.'))).toEqual([]); + }, 60_000); + + it("still links a method arrays don't have to the element's class: the call is on an element of the same name", async () => { + // vscode's `d.forEach(d => d.dispose())` on an `IDisposable[]`, and + // `extensions.map(extensions => extensions.syncInstalledExtensionsWithGallery(…))`. + const calls = await callsOf({ + ...CLASSES, + 'src/use.ts': + "import { Item } from './item';\n" + + 'export function flushAll(d: Item[]) {\n' + + ' d.forEach((d) => d.flush());\n' + + '}\n', + }); + expect(calls.filter((c) => c.startsWith('d.'))).toEqual(['d.flush -> Item::flush']); + }, 60_000); + + it('still links a call on a local annotated as the class, generic or not', async () => { + const calls = await callsOf({ + ...CLASSES, + 'src/use.ts': + "import { Item, Repository } from './item';\n" + + 'export function one(item: Item, repo: Repository) {\n' + + " repo.find('x');\n" + + " return item.find('k');\n" + + '}\n', + }); + expect(calls).toEqual(['item.find -> Item::find', 'repo.find -> Repository::find']); + }, 60_000); +}); diff --git a/src/resolution/import-resolver.ts b/src/resolution/import-resolver.ts index a5e2a1074..9fa572206 100644 --- a/src/resolution/import-resolver.ts +++ b/src/resolution/import-resolver.ts @@ -23,7 +23,9 @@ import { normalizeInferredTypeName, goRefQualifier, inferLocalReceiverType, + arrayElementType, } from './name-matcher'; +import { JS_ARRAY_METHODS } from './js-builtins'; /** * Extension resolution order by language @@ -3270,34 +3272,22 @@ export function pythonModulePath(source: string, fromFile: string): string | nul * way resolution reads it for the call (inferLocalReceiverType, memoized per * file, scope and receiver, so a call the matcher just tried costs a lookup): * the binding the type is written through, `LRUCache` of `new LRUCache(…)` or `lib` of `new lib.LRUCache()`. Undefined for a type the file - * doesn't import, in a file that imports nothing, and for a local declared as - * an array of the type (`results: URI[]`). Resolution reads the element type - * there too, but the call is the array's (`results.push(…)`), which no edit - * of the type's module links: such calls were most of what this parked on - * vscode, and retrying them nearly doubled what an edit of `uri.ts` retried. + * V>(…)` or `lib` of `new lib.LRUCache()`, of the element type of an array + * (`results: URI[]`), or of the type whose property it is (`Emitter['event']`). + * Undefined for a type the file doesn't import, in a file that imports + * nothing, and for a method arrays have called on an array (`results.push(…)`): + * resolution links that to no project class, so no edit of the type's module + * does. Such calls were most of what this parked on vscode, and retrying them + * nearly doubled what an edit of `uri.ts` retried. */ function localTypeBinding(receiver: string, ref: UnresolvedRef, context: ResolutionContext, mappings: ImportMapping[]): ImportMapping | undefined { if (mappings.length === 0) return undefined; - const decl: { raw?: string; line?: number } = {}; - if (!inferLocalReceiverType(receiver, ref, context, decl) || !decl.raw) return undefined; - const head = decl.raw.split('.')[0]; - const binding = mappings.find((m) => m.localName === head); - if (!binding || decl.line === undefined) return binding; - const lines = context.getFileLines?.(ref.filePath) ?? context.readFile(ref.filePath)?.split(/\r?\n/); - const declaration = lines?.[decl.line]; - return declaration !== undefined && declaresArrayOf(declaration, receiver, decl.raw) ? undefined : binding; -} - -/** A `name: Type` annotation, the type's arguments, and the `[` of an array of it: `items: Item[]`. */ -const ANNOTATION = /([A-Za-z_$][\w$]*)\s*:\s*([A-Za-z_$][\w.$]*)\s*(?:<[^;=(){}<>]*(?:<[^;=(){}<>]*>[^;=(){}<>]*)*>)?\s*(\[)?/g; - -/** Whether `line` annotates `name` as an array of `type`: `items: Item[]`, `items: Item[] = []`. */ -function declaresArrayOf(line: string, name: string, type: string): boolean { - for (const m of line.matchAll(ANNOTATION)) { - if (m[1] === name && m[2] === type) return m[3] !== undefined; - } - return false; + const decl: { raw?: string } = {}; + const type = inferLocalReceiverType(receiver, ref, context, decl); + if (!type || !decl.raw) return undefined; + if (arrayElementType(type) !== undefined && JS_ARRAY_METHODS.has(ref.referenceName.slice(receiver.length + 1))) return undefined; + const head = decl.raw.split(/[.<[\s]/)[0]; + return mappings.find((m) => m.localName === head); } const IDENTIFIER = /^[A-Za-z_$][\w$]*$/; diff --git a/src/resolution/js-builtins.ts b/src/resolution/js-builtins.ts index 3d877759e..435dbd609 100644 --- a/src/resolution/js-builtins.ts +++ b/src/resolution/js-builtins.ts @@ -33,6 +33,21 @@ export const JS_BUILTIN_METHODS = new Set([ 'text', 'json', 'arrayBuffer', 'blob', 'formData', ]); +/** + * The methods an array has: `Array.prototype`'s and those every object + * inherits. On a local annotated as an array (`items: Item[]`) a call of one + * is the array's, never a method of the element's class. + */ +export const JS_ARRAY_METHODS = new Set([ + 'at', 'concat', 'copyWithin', 'entries', 'every', 'fill', 'filter', 'find', + 'findIndex', 'findLast', 'findLastIndex', 'flat', 'flatMap', 'forEach', + 'includes', 'indexOf', 'join', 'keys', 'lastIndexOf', 'map', 'pop', 'push', + 'reduce', 'reduceRight', 'reverse', 'shift', 'slice', 'some', 'sort', 'splice', + 'toReversed', 'toSorted', 'toSpliced', 'unshift', 'values', 'with', + 'toString', 'toLocaleString', 'valueOf', 'hasOwnProperty', 'isPrototypeOf', + 'propertyIsEnumerable', +]); + /** * TypeScript primitive type names. Distinct from JS_BUILT_INS on purpose: those * are runtime globals a receiver can be constructed from, these only ever come diff --git a/src/resolution/name-matcher.ts b/src/resolution/name-matcher.ts index 55162fdd9..32d9b4041 100644 --- a/src/resolution/name-matcher.ts +++ b/src/resolution/name-matcher.ts @@ -9,7 +9,7 @@ import * as path from 'path'; import { Language, Node } from '../types'; import { UnresolvedRef, ResolvedRef, ResolutionContext, ImportMapping, isSupertypeTarget, CPP_DEFINE_SIGNATURE, isInheritanceRef, isImportableKind } from './types'; import { blankStringContents, stripCommentsForRegex } from './strip-comments'; -import { JS_BUILT_INS, JS_BUILTIN_METHODS, TS_PRIMITIVE_TYPES } from './js-builtins'; +import { JS_ARRAY_METHODS, JS_BUILT_INS, JS_BUILTIN_METHODS, TS_PRIMITIVE_TYPES } from './js-builtins'; import { SWIFT_TYPE_PATH_CALL, resolveSwiftTypePathCall } from './swift-type-visibility'; import { dartImportPrefixes, dartLibrarySees, dartPrefixSees, inSameDartLibrary } from './dart-libraries'; import { isDartLocallyBound } from './dart-local-scope'; @@ -12847,6 +12847,11 @@ export function normalizeInferredTypeName(raw: string): string | null { return seg; } +/** The element type of an array type a TS annotation gave (`Item[]`, `Item []` → `Item`), or undefined. */ +export function arrayElementType(type: string): string | undefined { + return /^(.+?)\s*\[\s*\]$/.exec(type)?.[1]; +} + /** * A Java / C# type's optional type arguments (one level of nesting) and array * ranks, as a regex source: ``, `>`, `[]`, `[,]`. @@ -13061,7 +13066,11 @@ function buildLocalReceiverTypePatterns(language: Language, r: string): RegExp[] // at `<` so a generic-typed param (`repo: Repository`) still yields // `Repository`. resolveMethodOnType validates the type actually declares // the method, so the looser match produces no edge on a mis-inference. - new RegExp(`\\b${r}\\b\\s*:\\s*([A-Z][\\w.$]*)`), // lg: Logger (annotation or typed param) + // A type followed by brackets is captured whole, so matchMethodCall + // can tell an array of it (`items: Item[]`, `items: Item[]`), whose + // own methods aren't an element's (arrayElementType), from one of its + // properties (`groupIds: Element["groupIds"]`). + new RegExp(`\\b${r}\\b\\s*:\\s*([A-Z][\\w.$]*(?:(?:\\s*<[^;=(){}<>]*(?:<[^;=(){}<>]*>[^;=(){}<>]*)*>)?\\s*\\[[^\\]]*\\])?)`), // lg: Logger (annotation or typed param) ]; case 'python': return [ @@ -13713,10 +13722,7 @@ export function inferLocalReceiverType( const type = matchLine(i); if (type) { states.set(key, { hi: callIdx, ansIdx: i, ansType: type, ansRaw: lastRaw }); - if (decl) { - decl.raw = lastRaw; - decl.line = i; - } + if (decl) decl.raw = lastRaw; return type; } } @@ -13735,19 +13741,13 @@ export function inferLocalReceiverType( } state.hi = callIdx; if (state.ansIdx < startIdx) return null; - if (decl) { - decl.raw = state.ansRaw; - decl.line = state.ansIdx; - } + if (decl) decl.raw = state.ansRaw; return state.ansType; } for (let i = callIdx; i >= startIdx; i--) { const type = matchLine(i); if (type) { - if (decl) { - decl.raw = lastRaw; - decl.line = i; - } + if (decl) decl.raw = lastRaw; return type; } } @@ -14211,6 +14211,18 @@ export function matchMethodCall( if (!awaited.name || TS_PRIMITIVE_TYPES.has(awaited.name)) return null; inferredType = awaited.name; } + // A TS annotation with brackets after the type. An array of it (`items: + // Item[]`): a method arrays have is the array's, which no project class + // declares, where `distinctElements.find(…)` on vscode's `ExplorerItem[]` + // reached `ExplorerItem.find`; any other is called on one of its elements + // under the same name, as in `d.forEach(d => d.dispose())`, so it takes + // the element's type. A type's property (`Emitter['event']`, + // `Parameters[0]`) is still read as the type. + if (inferredType && ESM_FAMILY.has(ref.language) && inferredType.endsWith(']')) { + const element = arrayElementType(inferredType); + if (element !== undefined && JS_ARRAY_METHODS.has(methodName!)) return null; + inferredType = element ?? inferredType.replace(/\s*\[[^\]]*\]$/, ''); + } if (inferredType) { // A Go local bound from a type assertion calls a method of the type Go // finds for it, or nothing the project declares: `flusher.Flush()` after @@ -14965,16 +14977,13 @@ interface GoProjectType { /** * What inferLocalReceiverType tells its caller besides the type: the type as - * the declaration spells it (`raw`), the 0-based index of the line it was - * read from (`line`, set with `raw` by the line scan), and for a Go local - * bound from a type assertion, the type goWrittenType finds for it - * (`asserted`; null for one with no method the project declares). `asserted` - * is unset when the local is declared some other way, or goWrittenType can't - * tell its type. + * the declaration spells it (`raw`), and for a Go local bound from a type + * assertion, the type goWrittenType finds for it (`asserted`; null for one + * with no method the project declares). `asserted` is unset when the local + * is declared some other way, or goWrittenType can't tell its type. */ interface GoReceiverDecl { raw?: string; - line?: number; asserted?: GoProjectType | null; }