mirror of
https://github.com/colbymchenry/codegraph.git
synced 2026-10-24 19:30:10 +08:00
perf(resolution): skip receiver typing for JS/TS calls of a method no node is named (#2840)
A JS/TS call of a method that no node in the index is named, like
`list.push()`, `fs.writeFileSync()` or a test library's matcher, still
went through matchMethodCall's receiver-type inference, the TS binding
and import readers, and the class strategies. Every one of them ends in a
lookup by the method's name, so for such a name they can only answer
null. The one exception is the object-literal reader, which follows a
member to the binding the literal gives it: `const api = { alias:
renamed }` makes `api.alias()` a call of `renamed` (Issue #1932).
Root cause: matchMethodCall reads the calling file for the receiver's type
before anything tests the method's name.
Fix: the resolution context answers isAbsentName(name) from the set of
node names warmCaches already loads for the pre-filter. The answer is
exact, like getNodesByName, and false whenever the set isn't loaded. For a
JS-family call whose member is absent, matchMethodCall asks only the
object-literal reader, exactly as the full path would. When the reader
answers, the full path runs unchanged; otherwise the call returns null at
once. CodeGraph.indexFiles now drops the resolver's caches after it stores,
as indexAll and sync already do before they resolve, so a later resolution
never reads the name set from before the new files.
Validation:
- New __tests__/absent-member-calls.test.ts (6 cases). The skip reads
nothing of the calling file (red on main). The name set answers only
once loaded. Every path that links a member to a symbol of another name
is pinned with the member asserted absent: a literal alias, a shorthand
member bound to a renamed import, an `as const` literal, a holder in the
same file and one imported, and a renamed re-export read through a
namespace import. A method added through indexFiles after indexAll still
links (main links it; the skip without the cache drop did not). A
fixture indexes identically with and without the skip.
- Graphs byte-identical to main (scripts/dump-graph.mjs: nodes, edges with
metadata, refs, files), kernel built per arm, on express,
vue-element-admin, excalidraw and this repository (JS, TS, Vue, Svelte,
Astro) and on the controls flask, gson, gin, ripgrep and slim (Python,
Java, Go, Rust, PHP). Default mode (pooled on the two largest) for all
nine, single-threaded for the JS four, and again after rebasing onto
the current main. Main run twice is identical too. An incremental sync
adding a file that declares formerly absent names (1,088 failed refs
retried) is identical between the arms, with and without git, and
matches a fresh index.
- Survey of main (instrumented build, nine repos, every language): no call
whose member no node is named was resolved by matchMethodCall. The skip
fires on exactly the calls the survey found (12,354 here).
- Profile counters, single-threaded: receiver inference runs 38,626 times
instead of 50,980 on this repository, 6,460 instead of 9,306 on
excalidraw, 2,488 instead of 3,586 on express. Outcome counts are equal.
- Cost, medians of 5 interleaved runs: the resolve loop on this repository
4.54 to 4.41 s pooled and 16.85 to 16.29 s single-threaded, faster in 5
of 5 paired rounds each; resolution phase -2.1% and -2.3%; whole index
-1.3% and -2.2%. excalidraw's loop -1.6%, express's -5.0%; the five
controls within noise (3 runs each).
Fixes #2829
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
6500a3782b
commit
d833827503
@@ -91,6 +91,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
|
||||
- In Go, variables declared in a `var ( … )` block, like gin's `binding.JSON` or prometheus's metric counters, are now indexed, with their own doc comments. Before, every variable in such a block was missing from the graph: the calls and types in its value were not recorded, so a struct used only there looked unused, and a call made through it, like `fPointPool.Get()` or a counter's `.Inc()`, was matched by its method's name alone and could link to an unrelated type's method. Re-index Go projects after upgrading.
|
||||
- Indexing a project that includes large bundled JavaScript files, such as a copy of pdf.js or d3, is fast again: since 1.6.2, resolving the calls in a JavaScript or TypeScript file re-read the file's text above each call, so a single bundled library could add many seconds to an index. The graph it builds is unchanged. Thanks @bompus for the report. (#2334)
|
||||
- Indexing a project that ships a large minified JavaScript file, such as the GraphiQL bundle inside go-ethereum, no longer stalls at "Resolving refs" when CodeGraph runs on Node 22, and linking such a file is faster on any Node version. The graph it builds is unchanged.
|
||||
- Indexing JavaScript and TypeScript projects is a little faster when their code calls methods the project doesn't define, like `list.push(…)`, `fs.writeFileSync(…)` or a test library's matchers: CodeGraph now sees from the names it has indexed that nothing in the project carries such a method, and no longer reads the calling code to work out what the call is made on. The graph it builds is unchanged. Thanks @luojiyin1987 for the report and the analysis. (#2829)
|
||||
- The time `codegraph init` and `codegraph index` print beside the node and edge counts now covers the whole run, resolving references and linking included, and `codegraph sync` reports its whole run the same way. Before, it counted only reading and parsing the files, which can be a small part of an index, so a slow index looked fast. Thanks @bompus for the report. (#2334)
|
||||
- The call paths `codegraph context` prints now say what bridges each dynamic-dispatch hop, in the same words `codegraph_explore` uses: a call through an interface reads `interface → impl`, and a C++ override, a Redux thunk or a Gin middleware chain is named as one. Before, every hop of a kind it didn't know was labeled an "event", so a call through an interface looked like an event being emitted.
|
||||
- In Rust, code that uses an enum only through its variants, like `mode::Mode::A` in an expression, a `Mode::B =>` match arm or `Self::A` inside the enum's own `impl`, now shows up in that enum's callers and impact, linked to the enum that actually declares the variant rather than to a same-named type elsewhere. Standard-library variants like `Ordering::Less` or `Option::Some` and associated items like `Foo::new()` or `Foo::MAX` don't count as using a project enum. Re-index Rust projects after upgrading. Thanks @mg-mg-mg for the report and @danusha2345. (#2328)
|
||||
|
||||
@@ -0,0 +1,269 @@
|
||||
import { afterEach, describe, expect, it } from 'vitest';
|
||||
import * as fs from 'fs';
|
||||
import * as os from 'os';
|
||||
import * as path from 'path';
|
||||
import { CodeGraph } from '../src';
|
||||
import { DatabaseConnection } from '../src/db';
|
||||
import { QueryBuilder } from '../src/db/queries';
|
||||
import { createResolver, type ReferenceResolver } from '../src/resolution';
|
||||
import { matchMethodCall } from '../src/resolution/name-matcher';
|
||||
import type { ResolutionContext, UnresolvedRef } from '../src/resolution/types';
|
||||
import type { Node } from '../src/types';
|
||||
|
||||
/**
|
||||
* #2829: a JS/TS call of a member that no node in the index is named — a
|
||||
* library's `list.push()`, `fs.writeFileSync()`, a test framework's matcher —
|
||||
* can't be linked by any strategy in matchMethodCall that looks the member up
|
||||
* by its own name, so the matcher no longer reads the calling file to work
|
||||
* out the receiver's type for it. The one exception is the object-literal
|
||||
* reader, which follows a member to the binding the literal gives it. These
|
||||
* tests count the work the skip saves, pin every way a call reaches a symbol
|
||||
* of another name, and check a whole index against one made without the skip.
|
||||
*/
|
||||
|
||||
/** A context holding no nodes, which counts every read of the calling file. */
|
||||
function countingContext(reads: { n: number }, isAbsentName?: (name: string) => boolean): ResolutionContext {
|
||||
const source = ['const svc = createService();', 'svc.absent();'];
|
||||
return {
|
||||
getNodesInFile: () => [],
|
||||
getNodesByName: () => [],
|
||||
getNodesByQualifiedName: () => [],
|
||||
getNodesByKind: () => [],
|
||||
getNodesByLowerName: () => [],
|
||||
getImportMappings: () => [],
|
||||
fileExists: () => false,
|
||||
getProjectRoot: () => '/',
|
||||
getAllFiles: () => [],
|
||||
readFile: () => {
|
||||
reads.n++;
|
||||
return source.join('\n');
|
||||
},
|
||||
getFileLines: () => {
|
||||
reads.n++;
|
||||
return source;
|
||||
},
|
||||
...(isAbsentName ? { isAbsentName } : {}),
|
||||
};
|
||||
}
|
||||
|
||||
const absentCall: UnresolvedRef = {
|
||||
fromNodeId: 'function:caller',
|
||||
referenceName: 'svc.absent',
|
||||
referenceKind: 'calls',
|
||||
line: 2,
|
||||
column: 0,
|
||||
filePath: 'caller.ts',
|
||||
language: 'typescript',
|
||||
};
|
||||
|
||||
describe('A call of a member no node is named (#2829)', () => {
|
||||
const dirs: string[] = [];
|
||||
const graphs: CodeGraph[] = [];
|
||||
|
||||
afterEach(() => {
|
||||
for (const cg of graphs.splice(0)) cg.close();
|
||||
for (const dir of dirs.splice(0)) fs.rmSync(dir, { recursive: true, force: true, maxRetries: 5 });
|
||||
});
|
||||
|
||||
function tempDir(): string {
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-absent-member-'));
|
||||
dirs.push(dir);
|
||||
return dir;
|
||||
}
|
||||
|
||||
it('reads nothing of the calling file when no node has the member\'s name', () => {
|
||||
const reads = { n: 0 };
|
||||
expect(matchMethodCall(absentCall, countingContext(reads, () => true))).toBeNull();
|
||||
expect(reads.n).toBe(0);
|
||||
});
|
||||
|
||||
it('still reads it when a node has the name, or nothing says whether one does', () => {
|
||||
// The same call, with the name known: the receiver is read as before, so
|
||||
// the count above is the skip's doing.
|
||||
const known = { n: 0 };
|
||||
expect(matchMethodCall(absentCall, countingContext(known, () => false))).toBeNull();
|
||||
expect(known.n).toBeGreaterThan(0);
|
||||
// A context that can't answer never skips.
|
||||
const unknown = { n: 0 };
|
||||
expect(matchMethodCall(absentCall, countingContext(unknown))).toBeNull();
|
||||
expect(unknown.n).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
it('answers absence only from a loaded set of names', () => {
|
||||
const dir = tempDir();
|
||||
const db = DatabaseConnection.initialize(path.join(dir, 'names.db'));
|
||||
try {
|
||||
const queries = new QueryBuilder(db.getDb());
|
||||
const node: Node = {
|
||||
id: 'method:Service::known', kind: 'method', name: 'known', qualifiedName: 'Service::known',
|
||||
filePath: 'service.ts', language: 'typescript', startLine: 1, endLine: 2, startColumn: 0, endColumn: 0,
|
||||
updatedAt: Date.now(),
|
||||
};
|
||||
queries.insertNodes([node]);
|
||||
const resolver = createResolver(dir, queries);
|
||||
const context = resolver.getResolutionContext();
|
||||
|
||||
// Not loaded yet: no name is absent, so no caller skips on a guess.
|
||||
expect(context.isAbsentName?.('absent')).toBe(false);
|
||||
resolver.warmCaches();
|
||||
expect(context.isAbsentName?.('known')).toBe(false);
|
||||
expect(context.isAbsentName?.('absent')).toBe(true);
|
||||
// The name set is exact: case and a qualified name are other names.
|
||||
expect(context.isAbsentName?.('Known')).toBe(true);
|
||||
expect(context.isAbsentName?.('Service::known')).toBe(true);
|
||||
// Dropped with the other caches, as nodes may change after.
|
||||
resolver.clearCaches();
|
||||
expect(context.isAbsentName?.('absent')).toBe(false);
|
||||
} finally {
|
||||
db.close();
|
||||
}
|
||||
});
|
||||
|
||||
/**
|
||||
* Calls whose member no node is named, beside calls whose member is known:
|
||||
* a literal's member bound to another function (#1932), a shorthand member
|
||||
* bound to a renamed import, an `as const` literal, a literal typed
|
||||
* `Record<…>` (whose answer the type decides before the literal is read),
|
||||
* a literal imported from another file, a renamed re-export read through a
|
||||
* namespace import, and library calls through a parameter, a field and a
|
||||
* global.
|
||||
*/
|
||||
function writeFixture(dir: string): void {
|
||||
const files: Record<string, string> = {
|
||||
'a.ts': [
|
||||
"import { fetchUser as getUser } from './users';",
|
||||
"import * as ns from './barrel';",
|
||||
"import { holder } from './holder';",
|
||||
'function renamed() { return 4; }',
|
||||
'function renamedConst() { return 3; }',
|
||||
'export const api = { alias: renamed, getUser };',
|
||||
'const constApi = { aliasC: renamedConst } as const;',
|
||||
'const typedApi: Record<string, () => number> = { aliasT: renamedConst };',
|
||||
'export function sameFileCaller(list: number[]) {',
|
||||
' list.push(1);',
|
||||
' return [api.alias(), api.getUser(), constApi.aliasC(), typedApi.aliasT(), ns.run(), holder.aliasH()];',
|
||||
'}',
|
||||
'',
|
||||
].join('\n'),
|
||||
'users.ts': 'export function fetchUser() { return 1; }\n',
|
||||
'impl.ts': 'export function implementation() { return 2; }\n',
|
||||
'barrel.ts': "export { implementation as run } from './impl';\n",
|
||||
'holder.ts': 'function renamedHolder() { return 2; }\nexport const holder = { aliasH: renamedHolder };\n',
|
||||
'service.ts': [
|
||||
'export class Store { save(): number { return 1; } }',
|
||||
'export class Service {',
|
||||
' private store = new Store();',
|
||||
' private items: string[] = [];',
|
||||
' work(): number {',
|
||||
" this.items.push('x');",
|
||||
' return this.store.save();',
|
||||
' }',
|
||||
'}',
|
||||
'export function useService(s: Service, store: Store) {',
|
||||
' store.save();',
|
||||
' s.missing();',
|
||||
" console.log('x');",
|
||||
' return s.work();',
|
||||
'}',
|
||||
'',
|
||||
].join('\n'),
|
||||
'local.js': [
|
||||
'function impl() { return 5; }',
|
||||
'const local = { start: impl };',
|
||||
'function useLocal() { return [local.start(), local.stop(), Math.max(1, 2)]; }',
|
||||
'module.exports = { useLocal };',
|
||||
'',
|
||||
].join('\n'),
|
||||
};
|
||||
for (const [file, content] of Object.entries(files)) fs.writeFileSync(path.join(dir, file), content);
|
||||
}
|
||||
|
||||
/** Index the fixture; `isAbsentName` replaces the resolver's own answer when given. */
|
||||
async function indexFixture(isAbsentName?: (original: ResolutionContext['isAbsentName']) => ResolutionContext['isAbsentName']) {
|
||||
const dir = tempDir();
|
||||
writeFixture(dir);
|
||||
const cg = CodeGraph.initSync(dir);
|
||||
graphs.push(cg);
|
||||
const context = (cg as unknown as { resolver: ReferenceResolver }).resolver.getResolutionContext();
|
||||
if (isAbsentName) context.isAbsentName = isAbsentName(context.isAbsentName);
|
||||
await cg.indexAll();
|
||||
return { cg, dir };
|
||||
}
|
||||
|
||||
const callers = (cg: CodeGraph, name: string): string[] => {
|
||||
const target = cg.getNodesByName(name).find((n) => n.kind === 'function');
|
||||
expect(target, name).toBeDefined();
|
||||
return cg.getIncomingEdges(target!.id)
|
||||
.filter((e) => e.kind === 'calls')
|
||||
.map((e) => cg.getNode(e.source)!.name)
|
||||
.sort();
|
||||
};
|
||||
|
||||
it('keeps every way a call reaches a symbol of another name', async () => {
|
||||
const { cg } = await indexFixture();
|
||||
// Each call below names a member no node is named, so the skip sees it.
|
||||
for (const member of ['alias', 'getUser', 'aliasC', 'aliasT', 'run', 'aliasH', 'start', 'stop', 'push', 'missing']) {
|
||||
expect(cg.getNodesByName(member), member).toEqual([]);
|
||||
}
|
||||
// The literal reader: `{ alias: renamed }`, `{ getUser }` bound to a
|
||||
// renamed import, an `as const` literal, a holder in its own file.
|
||||
expect(callers(cg, 'renamed')).toEqual(['sameFileCaller']);
|
||||
expect(callers(cg, 'fetchUser')).toEqual(['sameFileCaller']);
|
||||
expect(callers(cg, 'renamedConst')).toEqual(['sameFileCaller']);
|
||||
expect(callers(cg, 'impl')).toEqual(['useLocal']);
|
||||
// The import resolver: an imported literal's member, and a re-export
|
||||
// renamed in a barrel read through a namespace import.
|
||||
expect(callers(cg, 'renamedHolder')).toEqual(['sameFileCaller']);
|
||||
expect(callers(cg, 'implementation')).toEqual(['sameFileCaller']);
|
||||
}, 60_000);
|
||||
|
||||
it('sees a method that a file indexed after a resolution pass declares', async () => {
|
||||
// indexAll leaves the resolver's name set loaded; indexFiles then adds a
|
||||
// method a new call names. The call is resolved against the new names,
|
||||
// as it is without the skip.
|
||||
const dir = tempDir();
|
||||
fs.writeFileSync(path.join(dir, 'store.ts'), 'export class Store { save(): number { return 1; } }\n');
|
||||
fs.writeFileSync(path.join(dir, 'main.ts'), "import { Store } from './store';\nexport function useStore(store: Store) { return store.save(); }\n");
|
||||
const cg = CodeGraph.initSync(dir);
|
||||
graphs.push(cg);
|
||||
await cg.indexAll();
|
||||
fs.writeFileSync(path.join(dir, 'store.ts'), 'export class Store {\n save(): number { return 1; }\n restock(): number { return 2; }\n}\n');
|
||||
fs.writeFileSync(path.join(dir, 'caller.ts'), "import { Store } from './store';\nexport function useRestock(store: Store) { return store.restock(); }\n");
|
||||
await cg.indexFiles(['store.ts', 'caller.ts']);
|
||||
cg.resolveReferences();
|
||||
const restock = cg.getNodesByName('restock').find((n) => n.kind === 'method');
|
||||
expect(restock).toBeDefined();
|
||||
expect(cg.getIncomingEdges(restock!.id).filter((e) => e.kind === 'calls').map((e) => cg.getNode(e.source)!.name))
|
||||
.toEqual(['useRestock']);
|
||||
}, 60_000);
|
||||
|
||||
it('indexes the fixture exactly as it does without the skip', async () => {
|
||||
let absent = 0;
|
||||
const withSkip = await indexFixture((original) => (name) => {
|
||||
const answer = original?.(name) ?? false;
|
||||
if (answer) absent++;
|
||||
return answer;
|
||||
});
|
||||
const withoutSkip = await indexFixture(() => undefined);
|
||||
// The skip was asked about the calls above and answered for some of them.
|
||||
expect(absent).toBeGreaterThan(0);
|
||||
expect(dumpGraph(withSkip.dir)).toEqual(dumpGraph(withoutSkip.dir));
|
||||
}, 60_000);
|
||||
});
|
||||
|
||||
/** Edges and parked references by natural keys, sorted, as scripts/dump-graph.mjs dumps them. */
|
||||
function dumpGraph(dir: string): string[] {
|
||||
// eslint-disable-next-line @typescript-eslint/no-require-imports
|
||||
const { DatabaseSync } = require('node:sqlite') as typeof import('node:sqlite');
|
||||
const db = new DatabaseSync(path.join(dir, '.codegraph', 'codegraph.db'), { readOnly: true });
|
||||
try {
|
||||
const rows = [
|
||||
...db.prepare(`SELECT s.qualified_name AS source, t.qualified_name AS target, e.kind, e.line, e.col, e.metadata
|
||||
FROM edges e JOIN nodes s ON s.id = e.source JOIN nodes t ON t.id = e.target`).all(),
|
||||
...db.prepare(`SELECT from_node_id, reference_name, reference_kind, line, col, status FROM unresolved_refs`).all(),
|
||||
];
|
||||
return rows.map((r) => JSON.stringify(r)).sort();
|
||||
} finally {
|
||||
db.close();
|
||||
}
|
||||
}
|
||||
+5
-1
@@ -884,7 +884,11 @@ export class CodeGraph {
|
||||
return { success: false, filesIndexed: 0, filesSkipped: 0, filesErrored: 0, nodesCreated: 0, edgesCreated: 0, errors: [{ message: 'Could not acquire file lock - another process may be indexing', severity: 'error' as const }], durationMs: 0 };
|
||||
}
|
||||
try {
|
||||
return this.orchestrator.indexFiles(filePaths);
|
||||
// The files' nodes are new to the resolver: a resolution after this
|
||||
// reloads its caches, the set of node names included, as indexAll and
|
||||
// sync do before they resolve, instead of reading the graph as it was
|
||||
// before (#2829).
|
||||
return this.orchestrator.indexFiles(filePaths).finally(() => this.resolver.clearCaches());
|
||||
} finally {
|
||||
this.fileLock.release();
|
||||
}
|
||||
|
||||
@@ -774,6 +774,10 @@ export class ReferenceResolver {
|
||||
return result;
|
||||
},
|
||||
|
||||
// The set warmCaches loads (`SELECT DISTINCT name FROM nodes`, the exact
|
||||
// match getNodesByName makes); unloaded, no name is absent.
|
||||
isAbsentName: (name: string) => this.knownNames !== null && !this.knownNames.has(name),
|
||||
|
||||
getMethodMatches: (typeName: string, methodName: string, language: Language) => {
|
||||
const key = `${language} ${typeName}::${methodName}`;
|
||||
const cached = this.methodMatchCache.get(key);
|
||||
|
||||
@@ -19728,6 +19728,22 @@ export function matchMethodCall(
|
||||
}
|
||||
|
||||
const [, objectOrClass, methodName] = match;
|
||||
|
||||
// A JS/TS call of a member no node in the index is named — `list.push()`,
|
||||
// `fs.writeFileSync()`, a test library's matcher — is one only the
|
||||
// object-literal reader below can link (#2829). It follows a member to the
|
||||
// binding the literal gives it: `const api = { alias: renamed }` makes
|
||||
// `api.alias()` a call of `renamed` (#1932). Every other strategy looks the
|
||||
// member up by its own name and finds nothing, so reading the calling file
|
||||
// for the receiver's type is wasted work. When the reader answers, the
|
||||
// whole path below runs and decides, as before.
|
||||
if (ref.referenceKind === 'calls' && JS_FAMILY.has(ref.language) && context.isAbsentName?.(methodName!)) {
|
||||
const literal = nmTimedT('mc-absent', ref, () => dotMatch && !objectOrClass!.includes('.')
|
||||
? resolveObjectPathMember(objectOrClass!, methodName!, ref, context, null)
|
||||
: null);
|
||||
if (!literal) return null;
|
||||
}
|
||||
|
||||
// A simple `receiver.method` / `receiver:method` / `receiver$method` shape whose
|
||||
// receiver type we can try to infer from its local declaration.
|
||||
// A python attribute receiver (`self.conn`, `cfg.client`) is not one: the
|
||||
|
||||
@@ -101,6 +101,15 @@ export interface ResolutionContext {
|
||||
getNodesInFileNamed?(filePath: string, name: string): Node[];
|
||||
/** Get all nodes by name */
|
||||
getNodesByName(name: string): Node[];
|
||||
/**
|
||||
* Whether no node in the index has exactly this name, answered from the
|
||||
* resolver's set of every node name rather than a query (#2829). False
|
||||
* whenever that set isn't loaded (before `warmCaches`, after
|
||||
* `clearCaches`), so a caller only ever acts on a certain absence. A view
|
||||
* that hides nodes keeps its base's answer, which stays certain: hiding
|
||||
* nodes adds no name. Optional: a context without it never answers.
|
||||
*/
|
||||
isAbsentName?(name: string): boolean;
|
||||
/** Get all nodes by qualified name */
|
||||
getNodesByQualifiedName(qualifiedName: string): Node[];
|
||||
/** Get all nodes of a kind */
|
||||
|
||||
Reference in New Issue
Block a user