diff --git a/CHANGELOG.md b/CHANGELOG.md index dd7f4154d..869b2c0a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,6 +87,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - `codegraph status` no longer says the index is up to date while indexed files are missing their symbols: it now names files the parser couldn't read and files stored without their symbols (which `codegraph sync` repairs), `status --json` counts both, and `codegraph files --json` lists each file's recorded errors. Thanks @lipchey for the report. (#2336) - Running `codegraph status` while `codegraph index` rebuilds a project no longer makes the rebuild fail with "database is locked": status now only reads the index, and it says when a rebuild is running in another process, in its output and as `rebuildInProgress` in `status --json`. Thanks @sealad886 for the report, and @danusha2345 and @KissFeng for the fixes. (#2913) - Running `codegraph query`, `explore`, `node`, `callers`, `callees`, `impact`, `context`, `files` or `affected` while `codegraph index` rebuilds a project no longer makes the rebuild fail with "database is locked", and neither does the context hook Claude Code runs on its own when you ask about your code: like `codegraph status`, they now only read the index and say when a rebuild is running, and the hook stays quiet until the rebuild is done. They still answer from an index an older CodeGraph version built. Thanks @sealad886 for the report. (#2913) +- A qualified name given to `codegraph callers`, `callees` or `impact`, or to `codegraph_node`, like `Format.Price`, now names only the symbol whose scope is spelled that way, so its answer no longer merges in `LegacyFormat.Price` or any other class whose name only ends with `Format`, nor a class that just sits in a file named `Format.cs`. In C#, `BasketService.AddItemToBasket` no longer also selects the `IBasketService` interface's method; ask for `IBasketService.AddItemToBasket` for that one. A file or folder still names the module in Rust, Python and Go, and a PHP namespace can be written with its own backslashes, like `Billing\Format::price`. No re-index needed. Thanks @Gobakabana for the report and @yang0228. (#2908) - `codegraph init` and `codegraph index` no longer report a file you deleted as one that "could not be read" while git still lists it, which it does until the deletion is staged: such a file is now skipped, and so is a symlink that points at nothing. A file that is there but can't be read is still reported. Thanks @xianzhi-xzw for the report and the first fix. (#2904) - A full index of a project that holds a video named `.ts`, like an MPEG-TS clip among test fixtures, no longer warns that the index is PARTIAL and missing a file, and `codegraph status` no longer reports the index as partial: the video is skipped on purpose, and it now counts as skipped. Thanks @Hetox for the original report. (#1910) - In CFML, a `` or `` block the parser can't read no longer marks its whole file as contributing nothing to the graph: `codegraph init` no longer warns about the file, and `codegraph status` no longer lists it among files whose symbols are missing, since its component, functions and calls are all indexed. The file's recorded errors now name the block and its line instead. Thanks @HarryMuc for the report. (#2654) diff --git a/__tests__/qualified-lookup-boundary.test.ts b/__tests__/qualified-lookup-boundary.test.ts new file mode 100644 index 000000000..fc12e0671 --- /dev/null +++ b/__tests__/qualified-lookup-boundary.test.ts @@ -0,0 +1,147 @@ +/** + * A qualified symbol query names its scope whole (#2908). + * + * `codegraph callers Format.Price` also selected `LegacyFormat::Price`, and + * merged its callers into the answer: the matcher accepted the query anywhere + * inside the qualifiedName, so `Format::Price` matched the tail of + * `LegacyFormat::Price`. A file named `Format.cs` holding `LegacyFormat` let it + * in a second way, since a path qualifier could stand for any scope. The same + * matcher serves the CLI's callers/callees/impact, `codegraph_node` and + * `codegraph_explore`. + */ +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { CodeGraph } from '../src'; +import { ToolHandler } from '../src/mcp/tools'; +import { matchesSymbol } from '../src/graph/symbol-lookup'; +import type { Node } from '../src/types'; + +function node(over: Partial): Node { + return { + id: 'n1', kind: 'method', name: 'Price', qualifiedName: 'Format::Price', + filePath: 'Pricing.cs', language: 'csharp', + startLine: 1, endLine: 2, startColumn: 0, endColumn: 0, updatedAt: 0, + ...over, + } as Node; +} + +describe('matchesSymbol — a qualifier names a whole scope', () => { + const format = node({}); + const legacy = node({ qualifiedName: 'LegacyFormat::Price' }); + + it('does not match a class whose name only ends with the qualifier', () => { + for (const query of ['Format.Price', 'Format::Price']) { + expect(matchesSymbol(format, query)).toBe(true); + expect(matchesSymbol(legacy, query)).toBe(false); + } + expect(matchesSymbol(legacy, 'LegacyFormat.Price')).toBe(true); + // Nor one that holds it in the middle of an outer scope's name. + expect(matchesSymbol(node({ qualifiedName: 'OldFormat::Inner::Price' }), 'Format::Inner::Price')).toBe(false); + expect(matchesSymbol(node({ qualifiedName: 'Outer::Format::Price' }), 'Format.Price')).toBe(true); + }); + + it('does not let a file named for one type stand for another type the file holds', () => { + expect(matchesSymbol(node({ qualifiedName: 'LegacyFormat::Price', filePath: 'src/Format.cs' }), 'Format.Price')).toBe(false); + expect(matchesSymbol(node({ qualifiedName: 'Helpers::Price', filePath: 'src/Format.cs' }), 'Format.Price')).toBe(false); + // The file named for the type itself still reads as that type. + expect(matchesSymbol(node({ qualifiedName: 'Format::Price', filePath: 'src/Format.cs' }), 'Format.Price')).toBe(true); + }); + + it('still lets a file or folder name the module where a language keeps it there', () => { + // Rust, Python and Go put a type's module in its path, never its name. + const reformat = { name: 'price', qualifiedName: 'Reformat::price' }; + expect(matchesSymbol(node({ ...reformat, filePath: 'format/pricing.go', language: 'go' }), 'format.price')).toBe(true); + expect(matchesSymbol(node({ ...reformat, filePath: 'src/format.py', language: 'python' }), 'format.price')).toBe(true); + expect(matchesSymbol(node({ ...reformat, filePath: 'src/format.rs', language: 'rust' }), 'format::price')).toBe(true); + // A function outside any type is the module's in every language. + const fn = node({ kind: 'function', name: 'formatDate', qualifiedName: 'formatDate', filePath: 'src/utils.ts', language: 'typescript' }); + expect(matchesSymbol(fn, 'utils.formatDate')).toBe(true); + }); + + it('reads a PHP namespace written with backslashes', () => { + const price = node({ name: 'price', qualifiedName: 'App\\Models\\Billing::Format::price', filePath: 'q.php', language: 'php' }); + for (const query of ['Format::price', 'Format.price', 'Billing\\Format::price', 'App\\Models\\Billing\\Format::price']) { + expect(matchesSymbol(price, query)).toBe(true); + } + expect(matchesSymbol(price, 'illing\\Format::price')).toBe(false); + const legacyPrice = node({ name: 'price', qualifiedName: 'App\\Models\\Billing::LegacyFormat::price', filePath: 'q.php', language: 'php' }); + expect(matchesSymbol(legacyPrice, 'Format::price')).toBe(false); + }); + + it('reads an Erlang name past its arity', () => { + const start = node({ kind: 'function', name: 'start', qualifiedName: 'my_server::start/0', filePath: 'src/my_server.erl', language: 'erlang' }); + expect(matchesSymbol(start, 'my_server::start')).toBe(true); + expect(matchesSymbol(start, 'my_server::start/0')).toBe(true); + expect(matchesSymbol(start, 'my_server::start/1')).toBe(false); + expect(matchesSymbol(node({ ...start, qualifiedName: 'old_my_server::start/0', filePath: 'src/old_my_server.erl' }), 'my_server::start')).toBe(false); + }); +}); + +describe('the #2908 repro through the CLI and codegraph_node', () => { + const BIN = path.resolve(__dirname, '../dist/bin/codegraph.js'); + let root: string; + let cg: CodeGraph; + let handler: ToolHandler; + + beforeAll(async () => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-qualified-boundary-')); + fs.writeFileSync( + path.join(root, 'Pricing.cs'), + [ + 'public static class Format', + '{', + ' public static string Price(decimal value) => value.ToString("0.00");', + '}', + '', + 'public static class LegacyFormat', + '{', + ' public static string Price(decimal value) => value.ToString();', + '}', + '', + 'public class Invoice', + '{', + ' public string Total(decimal value) => Format.Price(value);', + '', + ' public string OldTotal(decimal value) => LegacyFormat.Price(value);', + '}', + '', + ].join('\n') + ); + cg = await CodeGraph.init(root, { index: true }); + handler = new ToolHandler(cg); + }); + + afterAll(() => { + handler?.closeAll(); + cg?.close(); + fs.rmSync(root, { recursive: true, force: true }); + }); + + it('`callers Format.Price` names one definition and only its caller', () => { + const result = spawnSync(process.execPath, [BIN, 'callers', '-p', root, '--json', '--', 'Format.Price'], { + encoding: 'utf-8', + env: { ...process.env, CODEGRAPH_NO_DAEMON: '1', CODEGRAPH_WASM_RELAUNCHED: '1', NO_COLOR: '1' }, + timeout: 30_000, + }); + expect(result.status, result.stderr).toBe(0); + const out = JSON.parse(result.stdout); + const text = JSON.stringify(out); + expect(text).toContain('Format::Price'); + expect(text).not.toContain('LegacyFormat'); + expect(text).toContain('Total'); + expect(text).not.toContain('OldTotal'); + }); + + it('codegraph_node on `Format.Price` or `Format::Price` returns that method alone', async () => { + // Format::Price is declared on line 3, LegacyFormat::Price on line 8. + for (const symbol of ['Format.Price', 'Format::Price']) { + const text = (await handler.execute('codegraph_node', { symbol })).content[0]?.text ?? ''; + expect(text, symbol).toContain('Pricing.cs:3'); + expect(text, symbol).not.toContain('Pricing.cs:8'); + expect(text, symbol).not.toContain('OldTotal'); + } + }); +}); diff --git a/src/graph/symbol-lookup.ts b/src/graph/symbol-lookup.ts index b8c4f2e37..7944c0b36 100644 --- a/src/graph/symbol-lookup.ts +++ b/src/graph/symbol-lookup.ts @@ -46,12 +46,26 @@ export function lastQualifierPart(symbol: string): string { * Rewrite every scope separator to `.` so a query and a stored qualifiedName * written in different conventions can be compared directly. The extractors * join hierarchy with `::` while users type the language's own spelling - * (`Session.request`, `stage_apply::run`, `pkg/mod.Fn`). + * (`Session.request`, `stage_apply::run`, `pkg/mod.Fn`), and a PHP namespace + * keeps its own `\` (`App\Models\Billing::Format::price`). */ function canonicalScope(text: string): string { - return text.replace(/::/g, '.').replace(/\//g, '.'); + return text.replace(/::/g, '.').replace(/[/\\]/g, '.'); } +/** An Erlang arity (`start/0`) ends a qualifiedName; the scope test ignores it. */ +const ARITY_SUFFIX = /\/\d{1,3}$/; + +/** + * Languages whose modules are files or folders and never appear in a + * qualifiedName, so a path qualifier can stand for the module a method's type + * sits in: `stage_apply::run`, `views.get`, a Go `format.price`. + */ +const PATH_MODULE_LANGUAGES = new Set(['rust', 'python', 'go']); + +/** Kinds that are members of a type: the scope right above them is that type. */ +const TYPE_MEMBER_KINDS = new Set(['method', 'property', 'field', 'enum_member']); + /** * Does `node` satisfy the user's symbol query? * @@ -87,34 +101,43 @@ export function matchesSymbol(node: Node, symbol: string, goPackageOf?: GoPackag const lastPart = parts[parts.length - 1]!; if (node.name !== lastPart) return false; - // Stage 1: qualified-name containment under the extractor's `::` convention. - if (node.qualifiedName.includes(parts.join('::'))) return true; - - // Stage 1b: boundary-aligned suffix under a canonical separator. + // Stage 1: the qualifiedName ends with the query, on a scope boundary, under + // one canonical separator. // // Splitting on EVERY separator assumes no scope component contains one — // false for any language whose module names are themselves dotted (Elixir // `AppWeb.Format`, a Java/C# package, a Python dotted module). There the - // stored qualifiedName is `AppWeb.Format::group`, so the stage-1 spelling - // `AppWeb::Format::group` cannot match and a perfectly precise query - // resolved to nothing. Canonicalising both sides and requiring the match to - // land on a separator boundary handles both conventions with one rule, and - // is strictly tighter than the `includes` above. + // stored qualifiedName is `AppWeb.Format::group`, so the spelling + // `AppWeb::Format::group` cannot match it as written and a perfectly precise + // query resolved to nothing. Canonicalising both sides handles both + // conventions with one rule. The match must start on a boundary: a plain + // substring test also found `Format::Price` inside `LegacyFormat::Price`, so + // `callers Format.Price` merged the callers of every class whose name ends + // in `Format` (#2908). const canonicalQuery = canonicalScope(symbol); - const canonicalNode = canonicalScope(node.qualifiedName); + const canonicalNode = canonicalScope(node.qualifiedName.replace(ARITY_SUFFIX, '')); if (canonicalNode === canonicalQuery || canonicalNode.endsWith(`.${canonicalQuery}`)) { return true; } // Stage 2: file-path containment. Rust modules and Python packages are not in // qualifiedName — they are encoded in the path — so `stage_apply::run` - // matches a `run` in any file with a `stage_apply` path segment. + // matches a `run` in any file with a `stage_apply` path segment. Elsewhere a + // file or folder names no type: `Format.Price` must not reach + // `LegacyFormat::Price` because it sits in a `Format.cs`, so a member of a + // type matches here only when the path qualifier is its type's own name. const containerHints = parts.slice(0, -1).filter((p) => !RUST_PATH_PREFIXES.has(p)); if (containerHints.length === 0) return false; const segments = node.filePath.split('/').filter((s) => s.length > 0); const inPath = (hint: string): boolean => segments.some((seg) => seg === hint || seg.replace(/\.[^.]+$/, '') === hint); - if (containerHints.every(inPath)) return true; + const scope = canonicalNode.split('.').slice(0, -1); + const pathCanName = + !TYPE_MEMBER_KINDS.has(node.kind) || + PATH_MODULE_LANGUAGES.has(node.language) || + scope.length === 0 || + scope[scope.length - 1] === containerHints[containerHints.length - 1]; + if (pathCanName && containerHints.every(inPath)) return true; // Stage 3: a package, then the scope inside it. `refresh.Discovery.refresh` is // method `refresh` of type `Discovery` in Go package `refresh` (a Python @@ -122,7 +145,6 @@ export function matchesSymbol(node: Node, symbol: string, goPackageOf?: GoPackag // qualifiedName and the package in the path, so neither stage above reads the // whole name. The leading qualifiers must name the package as stage 2 does, // the rest must end the node's scope. - const scope = canonicalScope(node.qualifiedName).split('.').slice(0, -1); const splits: number[] = []; for (let k = 1; k < containerHints.length; k++) { const inner = containerHints.slice(k);