Просмотр исходного кода

Merge pull request #1718 from danusha2345/fix/1708-fuzzy-reachability-on-survivor

fix(resolution): fuzzy reachability rejects a unique guess, never manufactures one
Colby Mchenry 8 часов назад
Родитель
Сommit
90dcdbc827
3 измененных файлов с 176 добавлено и 1 удалено
  1. 1 0
      CHANGELOG.md
  2. 153 0
      __tests__/fuzzy-lexical-reach.test.ts
  3. 22 1
      src/resolution/name-matcher.ts

+ 1 - 0
CHANGELOG.md

@@ -219,6 +219,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 - **A definition its language makes file-local no longer captures calls from other files.** A C `static` in another source file (`.c`/`.cc`/… — not a header's `static inline`, which is textually included), a Kotlin/Java/C#/Swift/Scala/Dart/PHP `private` member, a Go unexported name in another package, and a Rust non-`pub` item outside its module subtree cannot be what a name in another file means, but name matching accepted them whenever the names agreed: an Android `editor.apply()` onto an unrelated class's `private fun apply`, a JavaScript `fail(...)` onto a Go `func fail`, a Rust `.count()` onto a private `fn count` in another crate, and C USB helpers onto a `static` in a `.c` they never link. Such a target is now declined after the whole name-matching pipeline settles — the reference stays unresolved rather than falling through to a fuzzy namesake. Same-file definitions, a child Rust module reaching its ancestors' private items, and Rust `impl Trait for Type` methods stay resolvable. Re-index after upgrading. (#1730, #1731)
 - **A binding in a module that exports nothing is no longer a cross-file target.** On vite, every `import { defineConfig } from 'vite'` across the playground resolved onto a `const vite = await createServer(…)` sitting at module scope in `playground/ssr-html/test-stacktrace.js` — a file with an import and no export, so that binding is reachable from nowhere but itself. Name matching commits as soon as one candidate survives, and nothing asked whether an import could reach the survivor; that one binding took 157 edges. A JS/TS file holding an `import` and no export of any kind now offers its locals to no other file. Classic scripts, CommonJS (including `exports["x"] = …`), a later `export { … }`, and names contributed through `declare global` are all unaffected. Across vite this removed 320 wrong edges and added 18, each addition a reference that was previously ambiguous rather than newly invented. Re-index after upgrading. (#1719)
 - **A bare call inside a JavaScript or TypeScript method no longer resolves to the method itself.** When a method and a module-scope function share a name, `serialize(this.raw)` written inside `Record.serialize` means the function, but the nearest same-named definition won the tie and the graph recorded the method calling itself. A call written without a receiver can never reach a method in JS/TS, so methods are no longer candidates for it; `this.serialize()` and `other.serialize()` resolve as before. (#1714)
+- **Fuzzy matching no longer lands on a closure it cannot reach.** A function nested inside another function is only callable from inside its container, and exact-name matching already declined such candidates; the fuzzy fallback did not, so a builtin method call (`res.text()`, `items.push()`) whose only same-named project symbol was some file's closure resolved onto that closure. The fallback now checks that the one candidate it would commit to is reachable, and declines otherwise — it does not filter the candidate list first, which would turn a crowd of same-named definitions into a single "unique" survivor and hand it every call of that name. On vite that removes the 12 edges onto nested functions and adds none. Re-index after upgrading. Thanks @bompus. (#1708, #1709)
 
 - **Files under an `e2e/` directory count as tests.** Their calls no longer appear as production callers in Steps, dead-code and test badges.
 

+ 153 - 0
__tests__/fuzzy-lexical-reach.test.ts

@@ -0,0 +1,153 @@
+/**
+ * A function nested inside another function is only callable from inside its
+ * container. matchByExactName already filters candidates that way; matchFuzzy
+ * must too, or a call to a builtin method (`res.text()`) whose only same-named
+ * project symbol is some file's closure resolves onto that closure.
+ */
+
+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';
+import { matchFuzzy } from '../src/resolution/name-matcher';
+import type { Node } from '../src/types';
+import type { ResolutionContext, UnresolvedRef } from '../src/resolution/types';
+
+describe('fuzzy matching respects lexical reachability of nested functions', () => {
+  let tempDir: string;
+  let cg: CodeGraph | null = null;
+
+  beforeEach(() => {
+    tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-fuzzy-reach-'));
+  });
+
+  afterEach(() => {
+    cg?.destroy();
+    cg = null;
+    try {
+      fs.rmSync(tempDir, { recursive: true, force: true });
+    } catch {
+      // Windows can still hold the SQLite handle for a moment; the OS temp dir is swept anyway.
+    }
+  });
+
+  it('does not resolve a builtin method call onto another file\'s closure of the same name', async () => {
+    fs.writeFileSync(
+      path.join(tempDir, 'seed.ts'),
+      [
+        'export function readSeedState(raw: string): string {',
+        '  function text(): string {',
+        '    return raw.trim();',
+        '  }',
+        '  return text();',
+        '}',
+        '',
+      ].join('\n')
+    );
+    fs.writeFileSync(
+      path.join(tempDir, 'fetch.ts'),
+      [
+        'export async function readOkText(settled: { value: Response }): Promise<string> {',
+        '  // A chained receiver reaches the resolver as the bare method name.',
+        '  return settled.value.text();',
+        '}',
+        '',
+      ].join('\n')
+    );
+    cg = await CodeGraph.init(tempDir, { index: true });
+    cg.resolveReferences();
+
+    const closure = cg
+      .getNodesByKind('function')
+      .find((n) => n.name === 'text' && n.filePath === 'seed.ts');
+    const caller = cg.getNodesByKind('function').find((n) => n.name === 'readOkText');
+    expect(closure).toBeDefined();
+    expect(caller).toBeDefined();
+
+    const fromCaller = cg.getOutgoingEdges(caller!.id).filter((e) => e.kind === 'calls');
+    expect(fromCaller.map((e) => e.target)).not.toContain(closure!.id);
+
+    // The in-container call still resolves.
+    const container = cg.getNodesByKind('function').find((n) => n.name === 'readSeedState');
+    const inside = cg.getOutgoingEdges(container!.id).filter((e) => e.kind === 'calls');
+    expect(inside.map((e) => e.target)).toContain(closure!.id);
+  });
+});
+
+/**
+ * The reachability check must sit on the one candidate matchFuzzy would
+ * commit to, never on the candidate set. Filtering a crowd of same-named
+ * definitions down to the reachable ones leaves a single survivor, and the
+ * strategy then hands it every call of that name: vite has a dozen `resolve`
+ * definitions, most nested, and one reachable `resolve` method inherited 59
+ * `import { resolve } from 'node:path'` calls that way (#1709). Driven
+ * directly, so the shape is pinned regardless of what the earlier strategies
+ * make of a given fixture.
+ */
+describe('fuzzy reachability rejects a unique guess but never manufactures one', () => {
+  const node = (partial: Partial<Node> & Pick<Node, 'id' | 'kind' | 'name' | 'filePath'>): Node => ({
+    qualifiedName: partial.name,
+    language: 'typescript',
+    startLine: 1,
+    endLine: 1,
+    startColumn: 0,
+    endColumn: 0,
+    updatedAt: 0,
+    ...partial,
+  });
+  // build.ts:  function build() { const resolve = …; function resolve() {} }
+  const container = node({ id: 'f:build', kind: 'function', name: 'build', filePath: 'build.ts', startLine: 1, endLine: 40 });
+  const closure = node({ id: 'f:build.resolve', kind: 'function', name: 'resolve', qualifiedName: 'build::resolve', filePath: 'build.ts', startLine: 10, endLine: 12 });
+  // pluginContainer.ts:  class PluginContainer { resolve() {} }
+  const method = node({ id: 'm:resolve', kind: 'method', name: 'resolve', qualifiedName: 'PluginContainer::resolve', filePath: 'pluginContainer.ts', startLine: 5, endLine: 9 });
+  const contextWith = (nodes: Node[]): ResolutionContext =>
+    ({
+      getNodesInFile: () => [],
+      getNodesByName: (name: string) => nodes.filter((n) => n.name === name),
+      getNodesByLowerName: (name: string) => nodes.filter((n) => n.name.toLowerCase() === name),
+      getNodesByQualifiedName: (qn: string) => [container].filter((n) => n.qualifiedName === qn),
+      getNodesByKind: () => [],
+      fileExists: () => false,
+      readFile: () => null,
+      getFileLines: () => [],
+      getProjectRoot: () => '',
+      getAllFiles: () => [],
+      getImportMappings: () => [],
+    }) as unknown as ResolutionContext;
+  const callFrom = (filePath: string, line: number): UnresolvedRef => ({
+    fromNodeId: 'f:caller',
+    referenceName: 'resolve',
+    referenceKind: 'calls',
+    line,
+    column: 2,
+    filePath,
+    language: 'typescript',
+  });
+
+  it('declines the sole candidate when it is a closure the call cannot reach', () => {
+    expect(matchFuzzy(callFrom('vite.config.js', 3), contextWith([closure]))).toBeNull();
+  });
+
+  it('still resolves the sole candidate from inside its container', () => {
+    expect(matchFuzzy(callFrom('build.ts', 20), contextWith([closure]))?.targetNodeId).toBe('f:build.resolve');
+  });
+
+  it('does not let the unreachable closure drop out and leave the method as a "unique" match', () => {
+    // Two same-named callables: ambiguous, exactly as before the check existed.
+    expect(matchFuzzy(callFrom('vite.config.js', 3), contextWith([closure, method]))).toBeNull();
+  });
+
+  it('trusts no nesting in C, where a nested function is an extraction artifact', () => {
+    // betaflight: tree-sitter-c's recovery from `RESET_CONFIG(…, .pid = {…})`
+    // runs resetPidProfile to the end of pid.c, so every function after it is
+    // "nested" in the graph. C has no nested named functions; the call reaches it.
+    const cClosure = node({ ...closure, id: 'f:c', language: 'c' as Node['language'], filePath: 'pid.c' });
+    const cRef = { ...callFrom('core.c', 3), language: 'c' as UnresolvedRef['language'] };
+    expect(matchFuzzy(cRef, contextWith([cClosure]))?.targetNodeId).toBe('f:c');
+  });
+
+  it('resolves a lone reachable method as before', () => {
+    expect(matchFuzzy(callFrom('vite.config.js', 3), contextWith([method]))?.targetNodeId).toBe('m:resolve');
+  });
+});

+ 22 - 1
src/resolution/name-matcher.ts

@@ -354,6 +354,9 @@ export function matchFunctionRef(
   return null;
 }
 
+/** Languages with no nested named functions: nesting in the graph is never a scope. */
+const NO_NESTED_FUNCTIONS = new Set<string>(['c', 'cpp']);
+
 /**
  * A function nested inside another FUNCTION is only callable from within its
  * container — Python, JS/TS, and every closure language scope it lexically.
@@ -371,6 +374,14 @@ function isLexicallyReachable(
   context: ResolutionContext
 ): boolean {
   if (candidate.kind !== 'function') return true;
+  // C and C++ have no nested named functions, so a function the graph shows
+  // inside another is an extraction artifact, not a scope: tree-sitter-c
+  // cannot parse a macro call whose arguments are designated initializers
+  // (betaflight's `RESET_CONFIG(pidProfile_t, pidProfile, .pid = {…})`), and
+  // its error recovery runs the enclosing function_definition to the end of
+  // the file, nesting every function after it. Trusting that nesting rejected
+  // 117 real calls into pid.c on that tree; the functions are reachable.
+  if (NO_NESTED_FUNCTIONS.has(candidate.language)) return true;
   const qn = candidate.qualifiedName;
   if (!qn || !qn.includes('::')) return true;
   const parentQn = qn.slice(0, qn.lastIndexOf('::'));
@@ -2799,13 +2810,23 @@ export function matchFuzzy(
   // a lone one and manufacture a 0.5 guess out of an ambiguity fuzzy declines.
   // Also decline a bare JS/TS call whose only survivor is a method or a
   // cross-file name the file already binds locally (#1714).
+  // A function nested inside another function is only callable from inside
+  // its container (#1230), so a builtin method call (`res.text()`) whose only
+  // same-named project symbol is some file's closure must decline (#1708).
+  // The check sits on the ONE candidate this strategy would commit to, not on
+  // the candidate set: filtering the unreachable ones out of a crowd would
+  // leave a single survivor and hand it every call of that name — on vite,
+  // `import { resolve } from 'node:path'` in a dozen playground configs onto
+  // the one reachable `resolve` method (#1709). Reachability may reject a
+  // unique guess; it must never manufacture one.
   if (
     finalCandidates.length === 1 &&
     isVisibleAcrossFiles(finalCandidates[0]!, ref, context) &&
     isCrossFileReachable(finalCandidates[0]!, ref, context) &&
     !(isBareJsCall(ref, context) &&
       (finalCandidates[0]!.kind === 'method' ||
-        (finalCandidates[0]!.filePath !== ref.filePath && isLocallyBoundJsName(ref.referenceName, ref.filePath, context))))
+        (finalCandidates[0]!.filePath !== ref.filePath && isLocallyBoundJsName(ref.referenceName, ref.filePath, context)))) &&
+    isLexicallyReachable(finalCandidates[0]!, ref, context)
   ) {
     const isCrossLanguage = finalCandidates[0]!.language !== ref.language;
     return {