diff --git a/CHANGELOG.md b/CHANGELOG.md index 1815c4150..6793367c8 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 name imported from a package no longer fuzzy-matches a project symbol.** `import type { EvaluatedModules } from 'vite/module-runner'` names a symbol that is not in the graph at all, but the fuzzy fallback matched it by name anyway — and the match is case-insensitive, so on vitest it landed on the unrelated method `VitestMocker::evaluatedModules`. Fuzzy matching now declines when the call site's binding is a bare specifier (a builtin or an npm package); relative, alias, workspace and `link:`/`file:` imports still fall through, and only JS/TS is affected, since elsewhere a project's own modules are imported by absolute name too. Across vitest, svelte, vite and rollup this removed 46 wrong edges and added none. Re-index after upgrading. + - **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__/fuzzy-bare-import-binding.test.ts b/__tests__/fuzzy-bare-import-binding.test.ts new file mode 100644 index 000000000..ff9ea17ae --- /dev/null +++ b/__tests__/fuzzy-bare-import-binding.test.ts @@ -0,0 +1,131 @@ +/** + * Filtering candidates until one survives does not make that survivor the + * target. When the call site's own name comes from a bare import — `resolve` + * from `node:path` — the real target is external and absent from the graph, so + * the last project symbol standing must not inherit the call. + * + * matchFuzzy is driven directly. Which strategy reaches a given ref depends on + * how many same-named symbols the repo holds and on what the earlier stages of + * matchReference make of them, so a source fixture pins the pipeline rather + * than this guard. On real trees the shape routes through fuzzy as a + * case-insensitive match — `EvaluatedModules` from `vite/module-runner` onto + * vitest's `VitestMocker::evaluatedModules`, `Bundle` from `magic-string` onto + * a svelte build script's `bundle`. + */ + +import { describe, it, expect } from 'vitest'; +import { matchFuzzy } from '../src/resolution/name-matcher'; +import type { Node } from '../src/types'; +import type { ImportMapping, ResolutionContext, UnresolvedRef } from '../src/resolution/types'; + +/** vite's `pluginContainer.ts:resolve` — the sole survivor of the filters. */ +const SURVIVOR: Node = { + id: 'm:resolve', + kind: 'method', + name: 'resolve', + qualifiedName: 'PluginContainer::resolve', + filePath: 'packages/vite/src/node/server/pluginContainer.ts', + language: 'typescript', + startLine: 10, + endLine: 20, + startColumn: 0, + endColumn: 0, + updatedAt: 0, +}; + +function contextWith( + imports: ImportMapping[], + candidate = SURVIVOR, + localLinkNames?: Set +): ResolutionContext { + return { + getWorkspacePackages: () => (localLinkNames ? { byName: new Map(), localLinkNames } : null), + getNodesInFile: () => [], + getNodesByName: () => [candidate], + getNodesByLowerName: () => [candidate], + getNodesByQualifiedName: () => [], + getNodesByKind: () => [], + fileExists: () => false, + readFile: () => null, + getFileLines: () => [], + getProjectRoot: () => '', + getAllFiles: () => [], + getImportMappings: () => imports, + } as unknown as ResolutionContext; +} + +const imported = (source: string): ImportMapping[] => [ + { localName: 'resolve', exportedName: 'resolve', source, isDefault: false, isNamespace: false }, +]; + +const callTo = (language: UnresolvedRef['language']): UnresolvedRef => ({ + fromNodeId: 'f:outDir', + referenceName: 'resolve', + referenceKind: 'calls', + line: 4, + column: 2, + filePath: 'playground/css/vite.config.js', + language, +}); + +describe('matchFuzzy declines a lone survivor bound to a bare import', () => { + it('declines a node: builtin', () => { + expect(matchFuzzy(callTo('javascript'), contextWith(imported('node:path')))).toBeNull(); + }); + + it('declines a bare npm specifier', () => { + expect(matchFuzzy(callTo('javascript'), contextWith(imported('rollup')))).toBeNull(); + }); + + it('still matches when the binding is a relative import the resolver could not follow', () => { + const res = matchFuzzy(callTo('javascript'), contextWith(imported('./generated/chunks'))); + expect(res?.targetNodeId).toBe('m:resolve'); + expect(res?.resolvedBy).toBe('fuzzy'); + }); + + // vite's playground/tsconfig.json declares `"paths": { "~utils": [...] }` — + // a nested tsconfig the alias loader never reads — so shape is the only + // signal that these are local. npm names cannot start with any of them. + it.each(['~utils', '~/utils', '#types/hmrPayload', '$lib/stores'])( + 'still matches a local specifier with no slash after its prefix: %s', + (source) => { + const res = matchFuzzy(callTo('javascript'), contextWith(imported(source))); + expect(res?.targetNodeId).toBe('m:resolve'); + expect(res?.resolvedBy).toBe('fuzzy'); + }, + ); + + // vitest's `test/browser/package.json` declares `"@vitest/bundled-lib": + // "link:./bundled-lib"`, a directory its `test/*` workspace globs do not + // reach, so the workspace map cannot vouch for the name and only the + // dependency protocol shows it is local. + it('still matches a link: dependency, which is local despite its package spelling', () => { + const linked = new Set(['@vitest/bundled-lib']); + const res = matchFuzzy( + callTo('javascript'), + contextWith(imported('@vitest/bundled-lib'), SURVIVOR, linked) + ); + expect(res?.targetNodeId).toBe('m:resolve'); + }); + + it('declines a scoped package that is not linked into the project', () => { + const linked = new Set(['@vitest/bundled-lib']); + expect( + matchFuzzy(callTo('javascript'), contextWith(imported('@vitest/mocker'), SURVIVOR, linked)) + ).toBeNull(); + }); + + it('still matches when the name is bound by no import at all', () => { + const res = matchFuzzy(callTo('javascript'), contextWith([])); + expect(res?.targetNodeId).toBe('m:resolve'); + }); + + it('leaves languages whose own modules are imported by absolute name alone', () => { + // `from os import path` and `from myapp.util import path` are the same + // shape, so the bare test cannot tell external from internal here. + const pythonRef = { ...callTo('python'), filePath: 'app/main.py' }; + const pythonNode = { ...SURVIVOR, language: 'python' as const }; + const res = matchFuzzy(pythonRef, contextWith(imported('os'), pythonNode)); + expect(res?.targetNodeId).toBe('m:resolve'); + }); +}); diff --git a/src/resolution/name-matcher.ts b/src/resolution/name-matcher.ts index c74d8f272..9a396afc0 100644 --- a/src/resolution/name-matcher.ts +++ b/src/resolution/name-matcher.ts @@ -6,6 +6,7 @@ import { Language, Node } from '../types'; import { UnresolvedRef, ResolvedRef, ResolutionContext } from './types'; +import { resolveWorkspaceImport } from './workspace-packages'; /** * Ceiling on how many same-named definitions a FUZZY name-match strategy will @@ -388,6 +389,72 @@ function isLexicallyReachable( ); } +/** + * Whether the call site's own name is bound by an import of a BARE specifier — + * a Node builtin or an npm package. Such a binding names a symbol that is not + * in the graph at all, so no project node is the right target for it, however + * few candidates are left standing. That is the trap the name-based strategies + * fall into: filtering narrows a crowd of same-named symbols but says nothing + * about whether the true target was ever in the crowd, so when one survives it + * inherits the call. `import { resolve } from 'node:path'` is the case that + * matters — a common name, many project definitions, and the real target + * external. + * + * Relative, alias, and workspace imports are deliberately not treated this way: + * those point at project files, so a name match is a reasonable recovery when + * the import resolver could not follow the path. + * + * Only the JS/TS family is checked. There, a project-internal import is + * distinguishable by shape — it is relative, aliased, or a workspace member — + * so "bare" really does mean external. In Java, Kotlin, Go and Python a + * project's own modules are imported by absolute name too, and the same test + * would reject the internal case along with the external one. + */ +function isBoundToBareImport(ref: UnresolvedRef, context: ResolutionContext): boolean { + if ( + ref.language !== 'typescript' && + ref.language !== 'javascript' && + ref.language !== 'tsx' && + ref.language !== 'jsx' && + ref.language !== 'arkts' + ) { + return false; + } + const source = context + .getImportMappings(ref.filePath, ref.language) + .find((i) => i.localName === ref.referenceName)?.source; + if (source === undefined) return false; + if (source.startsWith('.') || source.startsWith('/')) return false; + // `~`, `#` and `$` cannot begin an npm package name, so the prefix alone + // proves a local binding and no resolver lookup is needed: `~utils` (a + // tsconfig `paths` entry, which a nested tsconfig the alias loader never + // reads still declares), `#types/hmrPayload` (a package.json `imports` + // subpath), `$lib/...` (SvelteKit). Matching only `~/` classed the slashless + // spellings as bare and sent real project edges out with the wrong ones. + if (source.startsWith('~') || source.startsWith('#') || source.startsWith('$')) return false; + if (source.startsWith('@/') || source.startsWith('src/')) return false; + const aliases = context.getProjectAliases?.(); + if (aliases?.patterns.some((p) => source.startsWith(p.prefix))) return false; + const workspaces = context.getWorkspacePackages?.(); + if (workspaces && resolveWorkspaceImport(source, workspaces)) return false; + // A `link:` / `file:` dependency is a directory in the project that no + // workspace glob need cover, so the workspace map above cannot see it: + // vitest imports `@vitest/bundled-lib` from `test/browser/bundled-lib`, + // which its `test/*` globs stop short of. The name is local even though it + // is spelled exactly like a scoped registry package. + if (workspaces?.localLinkNames?.has(packageNameOf(source))) return false; + return true; +} + +/** + * The package a specifier names, without its subpath: `@scope/pkg/sub` → + * `@scope/pkg`, `pkg/sub` → `pkg`. Scoped names keep two segments. + */ +function packageNameOf(source: string): string { + const parts = source.split('/'); + return source.startsWith('@') ? parts.slice(0, 2).join('/') : parts[0]!; +} + /** * Try to resolve a reference by exact name match */ @@ -2405,6 +2472,7 @@ export function matchFuzzy( ref: UnresolvedRef, context: ResolutionContext ): ResolvedRef | null { + if (isBoundToBareImport(ref, context)) return null; const lowerName = ref.referenceName.toLowerCase(); // Use pre-built lowercase index for O(1) lookup instead of scanning all nodes diff --git a/src/resolution/workspace-packages.ts b/src/resolution/workspace-packages.ts index 386c3e45e..78031a77f 100644 --- a/src/resolution/workspace-packages.ts +++ b/src/resolution/workspace-packages.ts @@ -41,6 +41,15 @@ export interface WorkspacePackages { * list). Absent for npm/pnpm members (their index conventions cover it). */ entryByName?: Map; + /** + * Package names declared with a `link:` or `file:` specifier in the root or + * any member manifest (`"@vitest/bundled-lib": "link:./bundled-lib"`). Such + * a package lives in the project but need not sit under a workspace glob, + * so {@link resolveWorkspaceImport} cannot see it — this set exists only so + * a caller can tell that the NAME is project-local, and deliberately carries + * no directory, since resolving these is a separate change. + */ + localLinkNames?: Set; } /** @@ -55,15 +64,26 @@ export interface WorkspacePackages { export function loadWorkspacePackages(projectRoot: string): WorkspacePackages | null { const byName = new Map(); + const memberDirs: string[] = []; const patterns = readWorkspaceGlobs(projectRoot); for (const pattern of patterns) { for (const dir of expandWorkspaceGlob(projectRoot, pattern)) { + memberDirs.push(dir); const pkgName = readPackageName(path.join(projectRoot, dir)); // First declaration wins — workspace patterns are tried in order. if (pkgName && !byName.has(pkgName)) byName.set(pkgName, dir); } } + // A member may depend on a package that is inside the project but outside + // every workspace glob (vitest's `test/browser` declares `"@vitest/ + // bundled-lib": "link:./bundled-lib"`, and the globs stop at `test/*`). + // Reading each manifest we already opened for its name costs nothing more. + const localLinkNames = new Set(); + for (const dir of ['', ...memberDirs]) { + for (const dep of readLinkDepNames(path.join(projectRoot, dir))) localLinkNames.add(dep); + } + // HarmonyOS/OpenHarmony (ArkTS) modular projects: every module's // oh-package.json5 declares its local siblings as `"data": "file:../../ // core/data"` dependencies, and code then imports the bare name @@ -77,10 +97,14 @@ export function loadWorkspacePackages(projectRoot: string): WorkspacePackages | if (entry) entryByName.set(name, entry); } - if (byName.size === 0) return null; + if (byName.size === 0 && localLinkNames.size === 0) return null; - logDebug('workspace packages loaded', { count: byName.size }); - return { byName, entryByName: entryByName.size > 0 ? entryByName : undefined }; + logDebug('workspace packages loaded', { count: byName.size, linked: localLinkNames.size }); + return { + byName, + entryByName: entryByName.size > 0 ? entryByName : undefined, + localLinkNames: localLinkNames.size > 0 ? localLinkNames : undefined, + }; } /** @@ -314,6 +338,32 @@ function expandWorkspaceGlob(projectRoot: string, pattern: string): string[] { } /** Read the `name` field from a member directory's package.json. */ +/** + * Dependency names this manifest declares with a `link:` or `file:` specifier + * — the two protocols npm, yarn, pnpm and bun all read as "this package is a + * directory in the project", so the name is local however much it looks like + * a registry package. A missing or malformed manifest contributes nothing. + */ +function readLinkDepNames(dirAbs: string): string[] { + let pkg: Record; + try { + pkg = JSON.parse(fs.readFileSync(path.join(dirAbs, 'package.json'), 'utf-8')); + } catch { + return []; + } + const names: string[] = []; + for (const field of ['dependencies', 'devDependencies', 'optionalDependencies', 'peerDependencies']) { + const deps = pkg?.[field]; + if (!deps || typeof deps !== 'object') continue; + for (const [name, spec] of Object.entries(deps as Record)) { + if (typeof spec === 'string' && (spec.startsWith('link:') || spec.startsWith('file:'))) { + names.push(name); + } + } + } + return names; +} + function readPackageName(dirAbs: string): string | null { try { const pkg = JSON.parse(fs.readFileSync(path.join(dirAbs, 'package.json'), 'utf-8'));