diff --git a/CHANGELOG.md b/CHANGELOG.md index 8611ba779c..e8de3df509 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixes +- In JavaScript and TypeScript, an import of a sibling package that a `package.json` depends on by path, like `"acme-shared": "file:../common"` (also `link:` and `portal:`), now resolves to that package's file without a root `workspaces` declaration — for the files of the package that declares the dependency, as npm installs it. Before, the import failed and its calls were matched by name, so when two functions shared a name the nearest one won. (#2456) - In TypeScript, an awaited call with type arguments like `await fetchApi("/me")` or `await api.get(url)` now links to the function it calls. Before, the call was dropped, so typed fetch wrappers looked like they had no callers. (#2586) - In React, a component rendered at module scope, in a top-level constant or inside a test's `it(…)` callback, like `createRoot(el).render()` or `render()`, now counts as rendered there, so an app's entry reaches its root component and a component's tests show among its callers. (#2587) - In Python, `import a.b as X` written inside a function now links its calls like `X.f()`, as the same import at module level already did. Thanks @Egrn for the report. (#2600) diff --git a/__tests__/npm-file-dependency-import.test.ts b/__tests__/npm-file-dependency-import.test.ts new file mode 100644 index 0000000000..2f7742deb2 --- /dev/null +++ b/__tests__/npm-file-dependency-import.test.ts @@ -0,0 +1,118 @@ +/** + * A sibling package declared as `"acme-shared": "file:../common"` is in the + * repository, so `import { formatLabel } from 'acme-shared/utils'` is an + * import of `packages/common/utils.ts` — with no root `workspaces` to say so. + * Before, the import failed and the call was matched by name: right while the + * name was unique, the nearest same-named function otherwise (#2456). + * + * The dependency is the declaring package's: a fixture app's `"left-pad": + * "file:../stub-left-pad"` is the stub for the fixture's own files, and + * `left-pad` stays the npm package for the rest of the project. + */ +import { describe, it, expect, afterAll, beforeAll } from 'vitest'; +import * as fs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { CodeGraph } from '../src'; + +let root = ''; +let cg: CodeGraph; + +beforeAll(async () => { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'cg-npm-file-dep-')); + const files: Record = { + 'packages/common/package.json': JSON.stringify({ name: 'acme-shared', version: '1.0.0' }), + 'packages/common/utils.ts': `export function formatLabel(name: string): string { + return name.trim().toUpperCase(); +} +`, + 'packages/api/package.json': JSON.stringify({ name: 'acme-api', version: '1.0.0', dependencies: { 'acme-shared': 'file:../common' } }), + // A same-named function next to the caller: what name matching would pick. + 'packages/api/src/labels.ts': `export function formatLabel(name: string): string { + return name; +} +`, + 'packages/api/src/handler.ts': `import { formatLabel } from 'acme-shared/utils'; + +export function handleRequest(name: string): string { + return formatLabel(name); +} +`, + 'packages/ui/package.json': JSON.stringify({ name: '@acme/ui', version: '1.0.0' }), + 'packages/ui/button.ts': `export function renderButton(text: string): string { + return text; +} +`, + 'packages/web/package.json': JSON.stringify({ name: 'acme-web', version: '1.0.0', devDependencies: { '@acme/ui': 'link:../ui' } }), + 'packages/web/src/page.ts': `import { renderButton } from '@acme/ui/button'; + +export function renderPage(): string { + return renderButton('ok'); +} +`, + 'package.json': JSON.stringify({ name: 'acme-root', private: true, dependencies: { 'left-pad': '^1.0.0' } }), + '__tests__/fixtures/app/package.json': JSON.stringify({ name: 'fixture-app', dependencies: { 'left-pad': 'file:../stub-left-pad' } }), + '__tests__/fixtures/app/use.ts': `import leftPad from 'left-pad'; + +export function useStub(): string { + return leftPad('x', 1); +} +`, + '__tests__/fixtures/stub-left-pad/package.json': JSON.stringify({ name: 'left-pad', main: 'index.ts' }), + '__tests__/fixtures/stub-left-pad/index.ts': `export default function leftPad(s: string, n: number): string { + return s; +} +`, + '__tests__/fixtures/stub-left-pad/lib/pad.ts': `export function pad(s: string): string { + return s; +} +`, + 'src/main.ts': `import leftPad from 'left-pad'; +import { pad } from 'left-pad/lib/pad'; + +export function run(): string { + return leftPad(pad('x'), 3); +} +`, + }; + for (const [rel, content] of Object.entries(files)) { + fs.mkdirSync(path.dirname(path.join(root, rel)), { recursive: true }); + fs.writeFileSync(path.join(root, rel), content); + } + cg = await CodeGraph.init(root, { index: true }); +}); + +afterAll(() => { + cg?.close(); + if (root) fs.rmSync(root, { recursive: true, force: true }); +}); + +/** `kind -> file (resolvedBy)` of what `name` in `file` calls, and of what `file` imports. */ +const edgesOf = (file: string, name: string) => { + const nodes = cg.getNodesInFile(file).filter((n) => n.name === name || n.kind === 'file'); + return cg.getOutgoingEdgesFrom(nodes.map((n) => n.id)).filter((e) => e.kind === 'calls' || e.kind === 'imports') + .map((e) => `${e.kind} -> ${cg.getNode(e.target)!.filePath} (${String(e.metadata?.resolvedBy)})`); +}; + +describe('an npm dependency on a sibling directory', () => { + it('file: — the package-name import is the sibling package\'s file', () => { + const edges = edgesOf('packages/api/src/handler.ts', 'handleRequest'); + expect(edges).toContain('calls -> packages/common/utils.ts (import)'); + expect(edges).toContain('imports -> packages/common/utils.ts (import)'); + expect(edges.filter((e) => e.includes('packages/api/src/labels.ts'))).toEqual([]); + }); + + it('link: in devDependencies, a scoped name', () => { + expect(edgesOf('packages/web/src/page.ts', 'renderPage')).toContain('calls -> packages/ui/button.ts (import)'); + }); +}); + +describe('a file: dependency a fixture app declares on a stub of an npm package', () => { + it('is the stub for the fixture\'s own files', () => { + expect(edgesOf('__tests__/fixtures/app/use.ts', 'useStub')).toContain('calls -> __tests__/fixtures/stub-left-pad/index.ts (import)'); + }); + + it('leaves the npm package external for the rest of the project: no edge from production code into the stub', () => { + expect(edgesOf('src/main.ts', 'run').filter((e) => e.includes('stub-left-pad'))).toEqual([]); + }); +}); diff --git a/src/resolution/callback-synthesizer.ts b/src/resolution/callback-synthesizer.ts index 0dac3e1e1d..2675d82e86 100644 --- a/src/resolution/callback-synthesizer.ts +++ b/src/resolution/callback-synthesizer.ts @@ -22,6 +22,7 @@ * tagged `provenance:'heuristic'`. See docs/design/callback-edge-synthesis.md. */ import type { Edge, Language, Node, NodeKind } from '../types'; +import { workspaceMemberDirs } from './workspace-packages'; import type { QueryBuilder } from '../db/queries'; import type { ResolutionContext } from './types'; import { isGeneratedFile } from '../extraction/generated-detection'; @@ -584,7 +585,7 @@ async function arkuiEmitterEdges(ctx: ResolutionContext, onYield: MaybeYield): P const moduleDirs = (() => { const ws = ctx.getWorkspacePackages?.(); - return ws ? [...new Set(ws.byName.values())].sort((a, b) => b.length - a.length) : []; + return ws ? workspaceMemberDirs(ws) : []; })(); const moduleScopeOf = (file: string): string => { for (const dir of moduleDirs) { @@ -688,7 +689,7 @@ async function arkuiRouterEdges(ctx: ResolutionContext, onYield: MaybeYield): Pr const allFiles = ctx.getAllFiles(); const moduleDirs = (() => { const ws = ctx.getWorkspacePackages?.(); - return ws ? [...new Set(ws.byName.values())].sort((a, b) => b.length - a.length) : []; + return ws ? workspaceMemberDirs(ws) : []; })(); const moduleScopeOf = (file: string): string => { for (const dir of moduleDirs) { diff --git a/src/resolution/import-resolver.ts b/src/resolution/import-resolver.ts index d7e8447717..9db993d782 100644 --- a/src/resolution/import-resolver.ts +++ b/src/resolution/import-resolver.ts @@ -389,7 +389,7 @@ function resolveImportPathUncached( // alias map first (custom prefixes like `@components/*` would // otherwise be misclassified as npm). What still looks like a package // may name a file through the importing app's own config. - if (isExternalImport(importPath, language, context)) { + if (isExternalImport(importPath, language, context, { fromFile })) { return resolveFromOwnConfig(importPath, fromFile, language, context); } @@ -1105,12 +1105,15 @@ const RUST_STDLIB_ROOTS = new Set(['std', 'core', 'alloc', 'proc_macro']); * asked `resolveImportPath` whether an alias maps the specifier to a file. * Matching a prefix proves nothing by itself: a catch-all `"*"` pattern * (`"*": ["./typings/*"]`) has an empty prefix and matches every package. + * + * `fromFile` (projectRoot-relative) is the importing file, for the path + * dependencies only the files under a package.json see. */ export function isExternalImport( importPath: string, language: Language, context?: ResolutionContext, - options: { aliasPrefixes?: boolean } = {} + options: { aliasPrefixes?: boolean; fromFile?: string } = {} ): boolean { // Relative imports are not external if (importPath.startsWith('.')) { @@ -1122,7 +1125,7 @@ export function isExternalImport( // workspace map first so they aren't misclassified as external (#629). The // map is null for single-package repos, so this is a no-op there. const workspaces = context?.getWorkspacePackages?.(); - if (workspaces && resolveWorkspaceImport(importPath, workspaces)) { + if (workspaces && resolveWorkspaceImport(importPath, workspaces, options.fromFile)) { return false; } @@ -1302,7 +1305,7 @@ function resolveAliasedImport( // extension/index permutations below then find its barrel (#629). const workspaces = context.getWorkspacePackages?.(); if (workspaces) { - const base = resolveWorkspaceImport(importPath, workspaces); + const base = resolveWorkspaceImport(importPath, workspaces, fromFile); if (base) { const hit = tryWithExt(base); if (hit) return hit; @@ -4218,7 +4221,7 @@ export function isBoundToOutOfRepoImport( if (imp.localName !== name) continue; // A package the repository declares itself (`declare module 'x'`) has // its declarations in the repository. - return isExternalImport(imp.source, ref.language, context) && + return isExternalImport(imp.source, ref.language, context, { fromFile: ref.filePath }) && resolveImportPath(imp.source, ref.filePath, ref.language, context) === null && !ambientModuleDeclares(imp.source, imp.exportedName, imp.isDefault, ref, context); } @@ -4291,7 +4294,7 @@ export function importBindingTail(ref: UnresolvedRef, context: ResolutionContext const binding = mappings.find((m) => m.localName === name) ?? (receiver && ref.referenceKind === 'calls' ? localTypeBinding(receiver, ref, context, mappings) : undefined); if (!binding) return undefined; - if (isExternalImport(binding.source, ref.language, context) && + if (isExternalImport(binding.source, ref.language, context, { fromFile: ref.filePath }) && // A monorepo app's own tsconfig alias (`#/views/…`): isExternalImport reads the root's only. !context.getNearestAliases?.(ref.filePath)?.patterns.some((p) => p.prefix !== '' && binding.source.startsWith(p.prefix)) && // A module the app's `baseUrl` or catch-all `paths` reads (`layout/MainLayout`). diff --git a/src/resolution/index.ts b/src/resolution/index.ts index 055e96ba68..12cd4082b4 100644 --- a/src/resolution/index.ts +++ b/src/resolution/index.ts @@ -700,7 +700,7 @@ export class ReferenceResolver { // `"lit/decorators": ["./node_modules/lit/decorators.js"]` lands in // `node_modules`, topcoder's `config` package on its `config/` folder. isOutOfRepoImport: (source, fromFile, language) => { - if (!isExternalImport(source, language, this.context, { aliasPrefixes: false })) return false; + if (!isExternalImport(source, language, this.context, { aliasPrefixes: false, fromFile })) return false; const resolved = resolveImportPath(source, fromFile, language, this.context); if (resolved !== null && this.isIndexedFile(resolved)) return false; return this.isDeclaredOutsidePackage(source, fromFile); diff --git a/src/resolution/workspace-packages.ts b/src/resolution/workspace-packages.ts index 0d1e2de8ea..47a630aed3 100644 --- a/src/resolution/workspace-packages.ts +++ b/src/resolution/workspace-packages.ts @@ -17,13 +17,17 @@ * Scope deliberately small for v1 (mirrors path-aliases.ts): * - reads `workspaces` (array OR `{ packages: [...] }`) from package.json, * plus a minimal `pnpm-workspace.yaml` `packages:` list + * - reads a sibling directory a package.json depends on by path + * (`"acme-shared": "file:../common"`, `link:`, `portal:`), which needs + * no workspace declaration at all — for the files under that manifest, + * as npm installs it for that package only; the root manifest's for all * - expands `*`, `?` and `{a,b}` within a folder name and `**` across * folders (`packages/*`, pnpm's nested `packages/**\/*`), and leaves out * what a negated glob (`!**\/example`) matches * - subpath resolution is directory-based (`@scope/ui/sub` → `/sub`); * it does NOT yet honour a member's `exports` map or `main` field - * - returns null when the project declares no workspaces, so single- - * package repos pay nothing and see no behaviour change. + * - returns null when the project declares neither, so single- + * package repos see no behaviour change. */ import * as fs from 'fs'; @@ -43,12 +47,21 @@ export interface WorkspacePackages { * list). Absent for npm/pnpm members (their index conventions cover it). */ entryByName?: Map; + /** + * The path dependencies a package.json below the root declares, visible + * to the files under its directory only: that directory → dep `name` → + * target directory (both relative to projectRoot, posix). A fixture app's + * `"left-pad": "file:../stub-left-pad"` is the stub for the fixture, and + * leaves `left-pad` the npm package for everyone else. + */ + scopedByName?: Map>; } /** * Load workspace member packages for `projectRoot`. Returns `null` when - * the project declares no workspaces (the common single-package case) — - * callers then skip all workspace logic. + * the project declares no workspaces and no dependency on a directory of its + * own (the common single-package case) — callers then skip all workspace + * logic. * * Cheap to call repeatedly only via the resolver's per-instance cache; * this function itself touches the filesystem, so the resolver memoises it @@ -75,19 +88,33 @@ export function loadWorkspacePackages(projectRoot: string): WorkspacePackages | // oh-package.json5 declares its local siblings as `"data": "file:../../ // core/data"` dependencies, and code then imports the bare name // (`import { CartRepository } from "data"`). Same monorepo problem as npm - // workspaces, different manifest. + // workspaces, different manifest. An npm package does the same with + // `"acme-shared": "file:../common"` and no root `workspaces` (#2456). const entryByName = new Map(); - for (const [name, dir] of collectOhpmFileDeps(projectRoot)) { + const fileDeps = collectFileDeps(projectRoot); + for (const [name, dir] of fileDeps.get('') ?? []) { if (byName.has(name)) continue; byName.set(name, dir); const entry = readOhpmMain(projectRoot, dir); if (entry) entryByName.set(name, entry); } + fileDeps.delete(''); - if (byName.size === 0) return null; + if (byName.size === 0 && fileDeps.size === 0) return null; + + logDebug('workspace packages loaded', { count: byName.size, scoped: fileDeps.size }); + return { + byName, + entryByName: entryByName.size > 0 ? entryByName : undefined, + scopedByName: fileDeps.size > 0 ? fileDeps : undefined, + }; +} - logDebug('workspace packages loaded', { count: byName.size }); - return { byName, entryByName: entryByName.size > 0 ? entryByName : undefined }; +/** Every directory the workspace map names, longest first: the members, and what a path dependency points at. */ +export function workspaceMemberDirs(ws: WorkspacePackages): string[] { + const dirs = new Set(ws.byName.values()); + for (const names of ws.scopedByName?.values() ?? []) for (const dir of names.values()) dirs.add(dir); + return [...dirs].sort((a, b) => b.length - a.length); } /** @@ -114,21 +141,26 @@ function readOhpmMain(projectRoot: string, dirRel: string): string | null { } /** - * Scan the project for `oh-package.json5` manifests and collect their - * `file:`-protocol dependencies as workspace members: dep name (what the - * source imports) → target directory (projectRoot-relative posix). + * Scan the project for `oh-package.json5` and `package.json` manifests and + * collect their `file:`-protocol dependencies as workspace members, by the + * directory they are visible from: `''` (the whole project) for every ohpm + * manifest and the root package.json, a package.json's own directory + * otherwise. Under each, dep name (what the source imports) → target + * directory (projectRoot-relative posix). An npm dependency counts when it + * names a directory of the project other than the root — not a tarball, and + * nothing under `node_modules`. * * Precision rule: a name declared with DIFFERENT target directories in - * different manifests (e.g. every sample in a samples monorepo has its own - * "common") is AMBIGUOUS and dropped entirely — a missing edge beats a wrong - * cross-module link. Registry dependencies (`@ohos/axios: "^2.0.0"`) don't - * use `file:` and are ignored, staying external. + * different manifests of one scope (e.g. every sample in a samples monorepo + * has its own "common") is AMBIGUOUS and dropped entirely — a missing edge + * beats a wrong cross-module link. Registry dependencies (`@ohos/axios: + * "^2.0.0"`) don't use `file:` and are ignored, staying external. * * The walk is bounded (depth + directory budget) and prunes build/dependency - * dirs, so non-ArkTS projects pay one readdir at the root and nothing else - * (they have no oh-package.json5 anywhere shallow). + * dirs; a manifest is parsed only where the walk finds one. */ const OHPM_MANIFEST = 'oh-package.json5'; +const NPM_MANIFEST = 'package.json'; const OHPM_WALK_MAX_DEPTH = 6; const OHPM_WALK_DIR_BUDGET = 8000; const OHPM_SKIP_DIRS = new Set([ @@ -136,8 +168,8 @@ const OHPM_SKIP_DIRS = new Set([ 'build', 'dist', 'out', 'oh-package-lock.json5', ]); -function collectOhpmFileDeps(projectRoot: string): Map { - const byName = new Map(); +function collectFileDeps(projectRoot: string): Map> { + const byScope = new Map>(); const ambiguous = new Set(); const queue: Array<{ rel: string; depth: number }> = [{ rel: '', depth: 0 }]; @@ -161,29 +193,46 @@ function collectOhpmFileDeps(projectRoot: string): Map { queue.push({ rel: rel ? `${rel}/${e.name}` : e.name, depth: depth + 1 }); continue; } - if (e.name !== OHPM_MANIFEST) continue; + if (e.name !== OHPM_MANIFEST && e.name !== NPM_MANIFEST) continue; - const deps = readOhpmFileDeps(path.join(abs, e.name)); + const deps = readFileDeps(path.join(abs, e.name)); + const scope = e.name === NPM_MANIFEST ? rel : ''; + let byName = byScope.get(scope); for (const [name, target] of deps) { const targetAbs = path.resolve(abs, target); const targetRel = path.relative(projectRoot, targetAbs).replace(/\\/g, '/'); if (targetRel.startsWith('..')) continue; // escapes the project + if (e.name === NPM_MANIFEST && (targetRel === '' || /(?:^|\/)node_modules(?:\/|$)/.test(targetRel) || !isDirectory(targetAbs))) continue; + if (!byName) byScope.set(scope, (byName = new Map())); const existing = byName.get(name); if (existing === undefined) { - if (!ambiguous.has(name)) byName.set(name, targetRel); + if (!ambiguous.has(`${scope}|${name}`)) byName.set(name, targetRel); } else if (existing !== targetRel) { byName.delete(name); - ambiguous.add(name); + ambiguous.add(`${scope}|${name}`); } } + if (byName?.size === 0) byScope.delete(scope); } } - return byName; + return byScope; } -/** Parse one oh-package.json5's dependencies → [name, file-target] pairs. */ -function readOhpmFileDeps(manifestAbs: string): Array<[string, string]> { +function isDirectory(abs: string): boolean { + try { + return fs.statSync(abs).isDirectory(); + } catch { + return false; + } +} + +/** + * Parse one manifest's dependencies → [name, path-target] pairs: an + * oh-package.json5's `file:` dependencies, and a package.json's `file:`, + * `link:` and `portal:` ones from every dependency field. + */ +function readFileDeps(manifestAbs: string): Array<[string, string]> { const out: Array<[string, string]> = []; let parsed: unknown; try { @@ -194,12 +243,16 @@ function readOhpmFileDeps(manifestAbs: string): Array<[string, string]> { } catch { return out; } - const deps = (parsed as { dependencies?: Record } | null)?.dependencies; - if (!deps || typeof deps !== 'object') return out; - for (const [name, value] of Object.entries(deps)) { - if (typeof value !== 'string' || !value.startsWith('file:')) continue; - const target = value.slice('file:'.length).trim(); - if (target) out.push([name, target]); + const npm = path.basename(manifestAbs) === NPM_MANIFEST; + const protocol = npm ? /^(?:file|link|portal):/ : /^file:/; + for (const field of npm ? ['dependencies', 'devDependencies', 'peerDependencies', 'optionalDependencies'] : ['dependencies']) { + const deps = (parsed as Record | null)?.[field]; + if (!deps || typeof deps !== 'object') continue; + for (const [name, value] of Object.entries(deps)) { + if (typeof value !== 'string' || !protocol.test(value)) continue; + const target = value.replace(protocol, '').trim(); + if (target) out.push([name, target]); + } } return out; } @@ -209,20 +262,35 @@ function readOhpmFileDeps(manifestAbs: string): Array<[string, string]> { * WITHOUT an extension — the caller applies the language's extension/index * resolution. `@scope/ui/widgets` → `packages/ui/widgets`; the bare package * name `@scope/ui` → its directory. Returns `null` when no member package - * name matches. + * name matches. A path dependency a package.json below the root declares is + * a member for the files under that manifest only, so those are read with + * `fromFile` (projectRoot-relative), the nearest declaring manifest first. */ export function resolveWorkspaceImport( importPath: string, - ws: WorkspacePackages + ws: WorkspacePackages, + fromFile?: string ): string | null { // Longest matching package name wins, so `@scope/ui/core` prefers a // `@scope/ui/core` package over a `@scope/ui` one when both exist. - let bestName: string | null = null; - for (const name of ws.byName.keys()) { - if (importPath === name || importPath.startsWith(name + '/')) { - if (!bestName || name.length > bestName.length) bestName = name; + const longest = (names: Map): string | null => { + let bestName: string | null = null; + for (const name of names.keys()) { + if (importPath === name || importPath.startsWith(name + '/')) { + if (!bestName || name.length > bestName.length) bestName = name; + } + } + return bestName; + }; + if (ws.scopedByName && fromFile !== undefined) { + const file = fromFile.replace(/\\/g, '/'); + const scopes = [...ws.scopedByName].filter(([dir]) => file.startsWith(dir + '/')).sort((a, b) => b[0].length - a[0].length); + for (const [, names] of scopes) { + const name = longest(names); + if (name) return (names.get(name)! + importPath.slice(name.length)).replace(/\/{2,}/g, '/'); } } + const bestName = longest(ws.byName); if (!bestName) return null; const dir = ws.byName.get(bestName)!; const subpath = importPath.slice(bestName.length); // '' or '/widgets'