diff --git a/CHANGELOG.md b/CHANGELOG.md index 1815c4150..2f50f9f06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -201,6 +201,8 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). #### Symbols, tests and the viewer +- **A definition its language makes file-local no longer captures calls from other files.** A C `static` function, a Kotlin/Java/C#/Swift `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: on a betaflight tree 4,448 calls landed on a `static` in some other file, an Android `editor.apply()` on an unrelated class's `private fun apply`, a JavaScript `fail(...)` on a Go program's `func fail`. Such a target is now declined outright — the reference stays unresolved rather than being handed to another file's namesake. Same-file definitions and Rust trait-impl methods are unaffected. Re-index after upgrading. (#1730) + - **Files under an `e2e/` directory count as tests.** Their calls no longer appear as production callers in Steps, dead-code and test badges. - **Production code under a `samples` or `examples` package path is no longer treated as test code.** A Kotlin or Java project whose package path runs through `com/google/samples/…` (Now in Android, for one) had nearly every file counted as a fixture, so the Map opened on `build-logic`, the entry points hid the app, and dead-code and test badges were wrong. Only the project layout above a `src/` folder decides now; the package path below it never does. diff --git a/__tests__/cross-file-visibility.test.ts b/__tests__/cross-file-visibility.test.ts new file mode 100644 index 000000000..1cb0007c9 --- /dev/null +++ b/__tests__/cross-file-visibility.test.ts @@ -0,0 +1,150 @@ +/** + * A definition the language makes file-local is not a candidate for a + * cross-file name match: a C `static`, a Kotlin `private fun`, a Go unexported + * identifier in another package, a Rust non-`pub` item outside its module + * subtree. Each case pairs the invisible shape with the visible one of + * identical form, so the assertion discriminates on visibility alone. + */ + +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 tempDir: string; +let cg: CodeGraph | null = null; + +function project(files: Record): void { + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'codegraph-visibility-')); + for (const [rel, content] of Object.entries(files)) { + const abs = path.join(tempDir, rel); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, content); + } +} + +/** `calls` targets of the function named `caller`, as `file:name` strings. */ +async function calleesOf(caller: string): Promise { + cg = await CodeGraph.init(tempDir, { index: true }); + cg.resolveReferences(); + const from = cg.getNodesByKind('function').concat(cg.getNodesByKind('method')).find((n) => n.name === caller)!; + expect(from).toBeDefined(); + return cg + .getOutgoingEdges(from.id) + .filter((e) => e.kind === 'calls') + .map((e) => cg!.getNode(e.target)) + .filter((n): n is NonNullable => !!n) + .map((n) => `${n.filePath}:${n.name}`); +} + +afterEach(() => { + cg?.close(); + cg = null; + fs.rmSync(tempDir, { recursive: true, force: true }); +}); + +describe('C: a static function is local to its translation unit', () => { + it('does not resolve a call onto a static in another file', async () => { + project({ + 'core.c': 'void coreRun(void)\n{\n usbGetDescriptor();\n}\n', + 'usb_audio.c': 'static void usbGetDescriptor(void)\n{\n}\n', + }); + expect(await calleesOf('coreRun')).not.toContain('usb_audio.c:usbGetDescriptor'); + }); + + it('still resolves onto a non-static function in another file', async () => { + project({ + 'core.c': 'void coreRun(void)\n{\n usbGetDescriptor();\n}\n', + 'usb_audio.c': 'void usbGetDescriptor(void)\n{\n}\n', + }); + expect(await calleesOf('coreRun')).toContain('usb_audio.c:usbGetDescriptor'); + }); + + it('keeps a static inline defined in a header: it lives in every unit that includes it', async () => { + project({ + 'protocol.h': 'static inline void mav_put_char(char *buf, char c)\n{\n buf[0] = c;\n}\n', + 'core.c': '#include "protocol.h"\n\nvoid coreRun(char *b)\n{\n mav_put_char(b, 0);\n}\n', + }); + expect(await calleesOf('coreRun')).toContain('protocol.h:mav_put_char'); + }); + + it('keeps a same-file static, whichever line the keyword is on', async () => { + project({ + 'core.c': 'static void\nhelper(void)\n{\n}\n\nvoid coreRun(void)\n{\n helper();\n}\n', + 'other.c': 'static void helper(void)\n{\n}\n', + }); + expect(await calleesOf('coreRun')).toEqual(['core.c:helper']); + }); +}); + +describe('Kotlin: a private function is class- or file-local', () => { + it('does not resolve an SDK-style call onto another file\'s private fun', async () => { + project({ + 'Budget.kt': 'class Budget {\n private fun apply(bps: Long): Long = bps\n}\n', + 'Main.kt': 'class Main {\n fun onCreate(editor: Editor) {\n editor.apply()\n }\n}\n', + }); + expect(await calleesOf('onCreate')).not.toContain('Budget.kt:apply'); + }); + + it('still resolves onto a public fun in another file', async () => { + project({ + 'Budget.kt': 'class Budget {\n fun apply(bps: Long): Long = bps\n}\n', + 'Main.kt': 'class Main {\n fun onCreate(budget: Budget) {\n budget.apply(1L)\n }\n}\n', + }); + expect(await calleesOf('onCreate')).toContain('Budget.kt:apply'); + }); +}); + +describe('Go: an unexported identifier is package-local', () => { + it('does not resolve a call onto an unexported func in another package', async () => { + project({ + 'cmd/probe/main.go': 'package main\n\nfunc fail(msg string) {}\n', + 'server/turn.go': 'package server\n\nfunc Run() {\n\tfail("x")\n}\n', + }); + expect(await calleesOf('Run')).not.toContain('cmd/probe/main.go:fail'); + }); + + it('still resolves within the package and onto an exported func elsewhere', async () => { + project({ + 'server/util.go': 'package server\n\nfunc fail(msg string) {}\n', + 'server/turn.go': 'package server\n\nfunc Run() {\n\tfail("x")\n\tReport()\n}\n', + 'report/report.go': 'package report\n\nfunc Report() {}\n', + }); + const callees = await calleesOf('Run'); + expect(callees).toContain('server/util.go:fail'); + expect(callees).toContain('report/report.go:Report'); + }); +}); + +describe('Rust: a non-pub item is visible to its module subtree only', () => { + it('does not resolve a sibling module\'s private fn, nor another crate\'s', async () => { + project({ + 'src/main.rs': 'mod util;\nmod net;\nfn main() {}\n', + 'src/util.rs': 'fn count() -> usize { 0 }\n', + 'src/net.rs': 'pub fn run() -> usize {\n count()\n}\n', + }); + expect(await calleesOf('run')).not.toContain('src/util.rs:count'); + }); + + it('keeps a trait-impl method, which has the trait\'s visibility', async () => { + project({ + 'src/main.rs': 'mod shape;\nmod draw;\nfn main() {}\n', + 'src/shape.rs': 'pub struct Circle;\npub trait Area { fn area(&self) -> f64; }\nimpl Area for Circle {\n fn area(&self) -> f64 { 1.0 }\n}\n', + 'src/draw.rs': 'use crate::shape::{Area, Circle};\npub fn render(c: &Circle) -> f64 {\n c.area()\n}\n', + }); + expect(await calleesOf('render')).toContain('src/shape.rs:area'); + }); + + it('still resolves a parent module\'s private fn from a child, and any pub fn', async () => { + project({ + 'src/main.rs': 'mod net;\nmod util;\nfn main() {}\n', + 'src/net.rs': 'pub mod tcp;\nfn shared() {}\n', + 'src/net/tcp.rs': 'use super::shared;\nuse crate::util::exported;\npub fn open() {\n shared();\n exported();\n}\n', + 'src/util.rs': 'pub fn exported() {}\n', + }); + const callees = await calleesOf('open'); + expect(callees).toContain('src/net.rs:shared'); + expect(callees).toContain('src/util.rs:exported'); + }); +}); diff --git a/src/resolution/index.ts b/src/resolution/index.ts index 7988c1bcf..2a995b985 100644 --- a/src/resolution/index.ts +++ b/src/resolution/index.ts @@ -16,7 +16,7 @@ import { FrameworkResolver, ImportMapping, } from './types'; -import { matchReference, matchFunctionRef, matchDottedCallChain, matchScopedCallChain, matchMethodCall, sameLanguageFamily, crossesKnownFamily, dumpNameMatcherProfile, clearNameMatcherMemos } from './name-matcher'; +import { isVisibleAcrossFiles, matchReference, matchFunctionRef, matchDottedCallChain, matchScopedCallChain, matchMethodCall, sameLanguageFamily, crossesKnownFamily, dumpNameMatcherProfile, clearNameMatcherMemos } from './name-matcher'; import { resolveViaImport, resolveJvmImport, extractImportMappings, extractReExports, loadCppIncludeDirs, isPhpIncludePathRef, isCobolCopybookRef, isNixPathImportRef, clearImportResolverMemos } from './import-resolver'; import { ResolverPool, minRefsForPool } from './resolver-pool'; import { detectFrameworks } from './frameworks'; @@ -1012,7 +1012,13 @@ export class ReferenceResolver { // binding it happened to pick. Same-file matches only. if (nameResult) { const target = this.queries.getNodeById(nameResult.targetNodeId); - if (ref.language === 'nix') { + // A definition its language makes file-local — a C `static`, a Kotlin + // `private fun`, a Go unexported name in another package, a Rust + // non-`pub` item outside its module subtree — cannot be what a name in + // another file means, whichever strategy chose it (#1730). + if (target && !isVisibleAcrossFiles(target, ref, this.context)) { + nameResult = null; + } else if (ref.language === 'nix') { if (!target || target.filePath !== ref.filePath) { nameResult = null; } diff --git a/src/resolution/name-matcher.ts b/src/resolution/name-matcher.ts index c74d8f272..6a1863bff 100644 --- a/src/resolution/name-matcher.ts +++ b/src/resolution/name-matcher.ts @@ -4,6 +4,7 @@ * Handles symbol name matching for reference resolution. */ +import * as path from 'path'; import { Language, Node } from '../types'; import { UnresolvedRef, ResolvedRef, ResolutionContext } from './types'; @@ -388,6 +389,149 @@ function isLexicallyReachable( ); } +/** + * Languages in which `visibility: 'private'` on a definition means no other + * FILE can name it: a Kotlin `private fun` is file- or class-local, and the + * same holds for Java, C#, Swift, Scala, Dart and PHP members. + */ +const PRIVATE_IS_FILE_LOCAL = new Set(['kotlin', 'java', 'csharp', 'swift', 'scala', 'dart', 'php']); + +/** Per-context memo: node id → "this C/C++ function is declared `static`". */ +const C_STATIC_MEMO = new WeakMap>(); + +/** + * A C/C++ file that IS a translation unit. A `static` defined here is local + * to it. A `static` (typically `static inline`) in a header is a different + * thing: the header is textually included, so the function exists in every + * unit that includes it and is callable from each — MAVLink's generated + * `mavlink_msg_*.h` are nothing but such functions, 4,306 real calls on one + * betaflight tree. + */ +const C_SOURCE_EXT = /\.(c|cc|cpp|cxx|c\+\+|m|mm)$/i; + +/** + * Whether a C/C++ function definition carries the `static` storage class — + * read from its first source line(s), since the extractor records no storage + * class and the kernel arm would need the same field. `static` on the line + * above the name (`static void\nfoo(void)`) is the common alternative layout. + */ +function isStaticCFunction(candidate: Node, context: ResolutionContext): boolean { + let memo = C_STATIC_MEMO.get(context); + if (!memo) { + memo = new Map(); + C_STATIC_MEMO.set(context, memo); + } + const hit = memo.get(candidate.id); + if (hit !== undefined) return hit; + const lines = context.getFileLines?.(candidate.filePath) ?? context.readFile(candidate.filePath)?.split('\n') ?? []; + const head = [lines[candidate.startLine - 2] ?? '', lines[candidate.startLine - 1] ?? ''].join('\n'); + const isStatic = /(^|[\s;}])static\s/.test(head); + memo.set(candidate.id, isStatic); + return isStatic; +} + +/** Per-context memo: node id → "this Rust method implements a trait". */ +const RUST_TRAIT_IMPL_MEMO = new WeakMap>(); + +/** + * Whether a Rust method sits in an `impl Trait for Type` block. Such a method + * carries no `pub` — the trait decides its visibility — so the extractor + * records it as private; it is reachable wherever the trait is. Read from the + * nearest enclosing `impl` header above the method, memoised per node. + */ +function isRustTraitImplMethod(candidate: Node, context: ResolutionContext): boolean { + if (candidate.kind !== 'method') return false; + let memo = RUST_TRAIT_IMPL_MEMO.get(context); + if (!memo) { + memo = new Map(); + RUST_TRAIT_IMPL_MEMO.set(context, memo); + } + const hit = memo.get(candidate.id); + if (hit !== undefined) return hit; + const lines = context.getFileLines?.(candidate.filePath) ?? context.readFile(candidate.filePath)?.split('\n') ?? []; + let isTrait = false; + for (let i = candidate.startLine - 2; i >= 0; i--) { + const line = lines[i] ?? ''; + if (/^\s*(pub(\([^)]*\))?\s+)?(unsafe\s+)?impl\b/.test(line)) { + isTrait = /\sfor\s/.test(line.replace(/\/\/.*$/, '')); + break; + } + // A top-level item above the method means it was not inside an impl. + if (/^(pub(\([^)]*\))?\s+)?(fn|struct|enum|mod|trait|const|static|type)\b/.test(line)) break; + } + memo.set(candidate.id, isTrait); + return isTrait; +} + +/** + * The directory a Rust file's private items are visible from: the file's own + * module subtree. `src/net.rs` and `src/net/mod.rs` own `src/net/`; a crate + * root (`lib.rs` / `main.rs`) owns its directory. A child module reaches its + * ancestors' private items (`super::`), a sibling or another crate never does. + */ +function rustModuleDir(filePath: string): string { + const base = path.posix.basename(filePath); + const dir = path.posix.dirname(filePath); + if (base === 'mod.rs' || base === 'lib.rs' || base === 'main.rs') return dir; + return path.posix.join(dir, base.replace(/\.rs$/, '')); +} + +/** + * Whether `candidate` can be NAMED from a reference in `ref`'s file at all, + * given what its language says about the definition's visibility. A + * definition the language makes file-local is not a candidate for a + * cross-file name match, however well the names agree: + * + * - **C / C++**: a `static` function defined in a SOURCE file is local to + * that translation unit; one in a header is part of every unit that + * includes it and stays visible. On a 2,109-file betaflight tree 145 + * cross-file calls resolved onto a `static` in another `.c` (#1730) — + * `usbd_get_descriptor` onto the `static get_device_descriptor` of + * whichever USB class file ranked first. + * - **Kotlin, Java, C#, Swift, Scala, Dart, PHP**: `private` is class- or + * file-local. An Android `editor.apply()` resolved onto an unrelated class's + * `private fun apply`. + * - **Go**: an unexported (lowercase) identifier is package-local, and a + * package is a directory. Judged by the name's case: the extractor's + * `isExported` is unset for every Go method. + * - **Rust**: a non-`pub` item is visible to its module and that module's + * descendants, never to a sibling module or another crate — `.count()` on + * an iterator resolved onto a `fn count` in a different crate. A method in + * an `impl Trait for Type` block has the trait's visibility, not `private`. + * + * Same-file candidates are always visible. Applied by ReferenceResolver to + * the target the whole name-matching pipeline settled on, so a rejection ends + * the reference unresolved: declining inside matchByExactName instead let the + * ref fall through to matchFuzzy, which then committed to a same-language + * namesake the ranking had passed over — eight such edges on one tree, all + * onto a local `const fail = …` arrow the graph does not hold. matchFuzzy + * checks its own survivor as well, since nothing runs after it. + */ +export function isVisibleAcrossFiles(candidate: Node, ref: UnresolvedRef, context: ResolutionContext): boolean { + if (candidate.filePath === ref.filePath) return true; + const lang = candidate.language as string; + if (lang === 'c' || lang === 'cpp') { + return ( + candidate.kind !== 'function' || + !C_SOURCE_EXT.test(candidate.filePath) || + !isStaticCFunction(candidate, context) + ); + } + if (lang === 'go') { + // By the name's first letter, not the extractor's flag: the flag is unset + // for every Go method, exported or not. + return /^[A-Z]/.test(candidate.name) || path.posix.dirname(candidate.filePath) === path.posix.dirname(ref.filePath); + } + if (lang === 'rust') { + if (candidate.visibility !== 'private') return true; + if (isRustTraitImplMethod(candidate, context)) return true; + const owner = rustModuleDir(candidate.filePath); + return ref.filePath.startsWith(owner + '/'); + } + if (PRIVATE_IS_FILE_LOCAL.has(lang)) return candidate.visibility !== 'private'; + return true; +} + /** * Try to resolve a reference by exact name match */ @@ -1299,6 +1443,8 @@ function getInferScanStates(context: ResolutionContext): Map RegExp[]): RegExp[] { @@ -2418,7 +2564,7 @@ export function matchFuzzy( const sameLanguageCandidates = callableCandidates.filter(n => n.language === ref.language); const finalCandidates = sameLanguageCandidates.length > 0 ? sameLanguageCandidates : callableCandidates; - if (finalCandidates.length === 1) { + if (finalCandidates.length === 1 && isVisibleAcrossFiles(finalCandidates[0]!, ref, context)) { const isCrossLanguage = finalCandidates[0]!.language !== ref.language; return { original: ref,