From 27135eab251dd493bd5824a03580a277ac1a1143 Mon Sep 17 00:00:00 2001 From: iqad Date: Wed, 9 Sep 2026 18:38:44 +0200 Subject: [PATCH] fix(pd001,pd002): resolve imports per workspace member and ignore type-only imports Two independent false-positive sources in the phantom-dependency rules, both on the same comparison in the PD detectors. Workspace roots: imports were collected from the whole tree but declarations were resolved against the root manifest only, so scanning a monorepo root reported every dependency a member declares and imports in its own source as a transitive-only phantom. buildOverrideContext now discovers workspace members (root `workspaces`, pnpm-workspace.yaml) and their declared packages, and PD001/PD002 resolve each importing file against the nearest enclosing member before falling back to the root. The finding lists only the files whose owning package leaves the import undeclared. Type-only imports: the usage scanner is a regex pass over raw file text, so a JSDoc annotation such as `/** @type {import('postcss-load-config') .Config} */` counted as a dynamic import, and `import type { X } from 'pkg'` counted as a runtime import. Comments are now blanked out (string-aware) before matching, and `import type` / `export type` / all-`type` specifier lists are skipped. This applies to every consumer of the scanner: PD001, PD002, the OA009 guard, and the --usage filter, where a type-only reference to a vulnerable package no longer counts as usage. Closes #966 --- src/overrides/context-builder.ts | 11 ++ src/overrides/context.ts | 13 +++ .../detectors/pd001-override-only-phantom.ts | 7 +- .../pd002-transitive-only-phantom.ts | 7 +- src/overrides/detectors/phantom-utils.ts | 35 +++++- src/usage/scanner.ts | 109 +++++++++++++++--- src/utils/package-json.ts | 24 ++++ tests/overrides/context-builder.test.ts | 72 ++++++++++++ tests/overrides/detectors/pd001.test.ts | 32 +++++ tests/overrides/detectors/pd002.test.ts | 94 +++++++++++++++ tests/usage.test.ts | 83 ++++++++++++- website/docs/override-hygiene/pd001.md | 11 ++ website/docs/override-hygiene/pd002.md | 11 ++ 13 files changed, 488 insertions(+), 21 deletions(-) diff --git a/src/overrides/context-builder.ts b/src/overrides/context-builder.ts index e72e6d9d..c116aa33 100644 --- a/src/overrides/context-builder.ts +++ b/src/overrides/context-builder.ts @@ -9,6 +9,8 @@ import { loadFromPnpmLock } from "../parsers/pnpm-lock.js"; import { loadFromYarnLock } from "../parsers/yarn-lock.js"; import { loadFromBunLock } from "../parsers/bun-lock.js"; import { scanAllImports } from "../usage/scanner.js"; +import { readWorkspaceMemberManifests } from "../utils/package-json.js"; +import { getDeclaredPackages } from "./detectors/phantom-utils.js"; type LockfileReadResult = | { kind: "ok"; names: Set } @@ -84,6 +86,14 @@ export function buildOverrideContext( const importedPackageNames = scanAllImports(projectPath); + // Imports are collected from the whole tree, so on a monorepo root a member's + // own source is included. The PD detectors need each member's manifest to + // decide whether an import found under that member is actually declared. + const workspaceMembers = readWorkspaceMemberManifests(projectPath).map((member) => ({ + dir: member.dir, + declared: getDeclaredPackages(member.manifest), + })); + return { projectPath, packageJson: parsed, @@ -97,6 +107,7 @@ export function buildOverrideContext( registryDistTags, skippedDetectors: skipped, importedPackageNames, + workspaceMembers, auditLog: opts.auditLog, logger: opts.logger, }; diff --git a/src/overrides/context.ts b/src/overrides/context.ts index 52e6f513..d2b7edda 100644 --- a/src/overrides/context.ts +++ b/src/overrides/context.ts @@ -59,6 +59,14 @@ export interface RegistryDistTags { [tag: string]: string | undefined; } +/** A workspace member's location and declared dependencies - for PD001/PD002. */ +export interface WorkspaceMember { + /** Directory relative to the project root, POSIX-separated (e.g. "apps/web"). */ + dir: string; + /** Names declared in any dependency section of the member's package.json. */ + declared: Set; +} + /** Reason a detector was pre-emptively skipped (e.g., missing node_modules). */ export interface SkippedDetector { ruleId: string; @@ -88,6 +96,11 @@ export interface OverrideContext { /** All bare module names imported in source files, mapped to relative file paths. * Empty map if no source files were found. Populated by context-builder. */ importedPackageNames: Map; + /** Workspace members (npm/yarn/bun `workspaces`, pnpm-workspace.yaml) and the + * packages each one declares. A source file under a member's directory + * resolves its imports against that member's manifest, not only the root's. + * Absent or empty for a single-package project. */ + workspaceMembers?: WorkspaceMember[]; auditLog: AuditLogHandle; logger: Logger; } diff --git a/src/overrides/detectors/pd001-override-only-phantom.ts b/src/overrides/detectors/pd001-override-only-phantom.ts index 9ed3e8a4..83979960 100644 --- a/src/overrides/detectors/pd001-override-only-phantom.ts +++ b/src/overrides/detectors/pd001-override-only-phantom.ts @@ -1,6 +1,6 @@ import type { OverrideContext } from "../context.js"; import type { OverrideFinding } from "../types.js"; -import { getDeclaredPackages, installCmd } from "./phantom-utils.js"; +import { getDeclaredPackages, installCmd, undeclaredImportFiles } from "./phantom-utils.js"; const RULE_ID = "PD001" as const; @@ -13,9 +13,10 @@ export function detect(ctx: OverrideContext): OverrideFinding[] { const cmd = installCmd(ctx.packageManager); const findings: OverrideFinding[] = []; - for (const [pkgName, files] of ctx.importedPackageNames) { - if (declared.has(pkgName)) continue; + for (const [pkgName, importedIn] of ctx.importedPackageNames) { if (!overrideNames.has(pkgName)) continue; + const files = undeclaredImportFiles(ctx, pkgName, importedIn, declared); + if (files.length === 0) continue; const shown = files.slice(0, 3); const extra = files.length > 3 ? ` (+${files.length - 3} more)` : ""; diff --git a/src/overrides/detectors/pd002-transitive-only-phantom.ts b/src/overrides/detectors/pd002-transitive-only-phantom.ts index b703a865..e805cd8e 100644 --- a/src/overrides/detectors/pd002-transitive-only-phantom.ts +++ b/src/overrides/detectors/pd002-transitive-only-phantom.ts @@ -1,6 +1,6 @@ import type { OverrideContext } from "../context.js"; import type { OverrideFinding } from "../types.js"; -import { getDeclaredPackages, installCmd } from "./phantom-utils.js"; +import { getDeclaredPackages, installCmd, undeclaredImportFiles } from "./phantom-utils.js"; const RULE_ID = "PD002" as const; @@ -13,10 +13,11 @@ export function detect(ctx: OverrideContext): OverrideFinding[] { const cmd = installCmd(ctx.packageManager); const findings: OverrideFinding[] = []; - for (const [pkgName, files] of ctx.importedPackageNames) { - if (declared.has(pkgName)) continue; + for (const [pkgName, importedIn] of ctx.importedPackageNames) { if (overrideNames.has(pkgName)) continue; // PD001 owns this if (!ctx.lockfilePackageNames.has(pkgName)) continue; // not in graph + const files = undeclaredImportFiles(ctx, pkgName, importedIn, declared); + if (files.length === 0) continue; const shown = files.slice(0, 3); const extra = files.length > 3 ? ` (+${files.length - 3} more)` : ""; diff --git a/src/overrides/detectors/phantom-utils.ts b/src/overrides/detectors/phantom-utils.ts index 84ab5308..ae7ca1e6 100644 --- a/src/overrides/detectors/phantom-utils.ts +++ b/src/overrides/detectors/phantom-utils.ts @@ -1,4 +1,4 @@ -import type { OverrideContext } from "../context.js"; +import type { OverrideContext, WorkspaceMember } from "../context.js"; export function getDeclaredPackages(packageJson: Record): Set { const declared = new Set(); @@ -13,6 +13,39 @@ export function getDeclaredPackages(packageJson: Record): Set, +): string[] { + if (rootDeclared.has(pkgName)) return []; + const members = ctx.workspaceMembers ?? []; + if (members.length === 0) return files; + + return files.filter((file) => { + const owner = owningWorkspaceMember(members, file); + return !owner || !owner.declared.has(pkgName); + }); +} + +function owningWorkspaceMember(members: WorkspaceMember[], file: string): WorkspaceMember | null { + const normalized = file.replace(/\\/g, "/"); + let best: WorkspaceMember | null = null; + for (const member of members) { + if (!normalized.startsWith(`${member.dir}/`)) continue; + // Nested members (packages/a and packages/a/plugins/b): the deepest wins. + if (!best || member.dir.length > best.dir.length) best = member; + } + return best; +} + export function installCmd(pm: OverrideContext["packageManager"]): string { if (pm === "pnpm") return "pnpm add"; if (pm === "yarn") return "yarn add"; diff --git a/src/usage/scanner.ts b/src/usage/scanner.ts index a2ae7143..41638b89 100644 --- a/src/usage/scanner.ts +++ b/src/usage/scanner.ts @@ -25,6 +25,97 @@ const ALL_IMPORTS_EXCLUDED_DIRS = new Set([ // await import('pkg') const IMPORT_REQUIRE_REGEX = /(?:(?:import|export)\s+[\w\s{},*]+\s+from\s+['"]([^'"]+)['"]|import\s+['"]([^'"]+)['"]|require\s*\(\s*['"]([^'"]+)['"]\s*\)|import\s*\(\s*['"]([^'"]+)['"]\s*\))/g; +// Blank out `//` and `/* */` comments so the import regex never matches text +// that is not code. The motivating case is a JSDoc annotation such as +// `/** @type {import('postcss-load-config').Config} */`: nothing loads that +// package at runtime, but a raw-text regex sees `import('...')` and counts it +// as a dynamic import. String literals are tracked so a `//` inside +// 'https://...' is not mistaken for a line comment. Comment bodies are +// replaced with spaces (newlines kept) so nothing else shifts position. +export function stripComments(source: string): string { + let out = ""; + let i = 0; + const n = source.length; + + while (i < n) { + const ch = source[i]; + const next = source[i + 1]; + + if (ch === "/" && next === "/") { + while (i < n && source[i] !== "\n") i++; + out += " "; + continue; + } + + if (ch === "/" && next === "*") { + i += 2; + while (i < n && !(source[i] === "*" && source[i + 1] === "/")) { + out += source[i] === "\n" ? "\n" : " "; + i++; + } + i += 2; + out += " "; + continue; + } + + if (ch === "'" || ch === '"' || ch === "`") { + const quote = ch; + out += ch; + i++; + while (i < n) { + const c = source[i]; + out += c; + i++; + if (c === "\\" && i < n) { + out += source[i]; + i++; + continue; + } + if (c === quote) break; + // An unterminated ' or " string ends at the line break; template + // literals may span lines. + if (c === "\n" && quote !== "`") break; + } + continue; + } + + out += ch; + i++; + } + + return out; +} + +// `import type { X } from 'pkg'`, `export type { X } from 'pkg'`, and +// `import { type A, type B } from 'pkg'` are erased by the TypeScript compiler +// and never load `pkg` at runtime. A default import that happens to be named +// `type` (`import type from 'pkg'`) or a mixed list (`import { type A, b }`) +// still loads the module and is kept. +export function isTypeOnlyImportStatement(statement: string): boolean { + const clause = /^(?:import|export)\s+([\s\S]*?)\s+from\s/.exec(statement)?.[1]?.trim(); + if (!clause) return false; + + if (/^type\s+[\w{*]/.test(clause)) return true; + + const braced = /^\{([\s\S]*)\}$/.exec(clause); + if (!braced) return false; + const specifiers = braced[1].split(",").map(s => s.trim()).filter(Boolean); + return specifiers.length > 0 && specifiers.every(s => /^type\s+\S/.test(s)); +} + +// Every module specifier that `content` would actually load at runtime: +// static imports and re-exports, side-effect imports, `require()` and dynamic +// `import()`, minus anything inside a comment or in a type-only import. +function collectRuntimeImportPaths(content: string): string[] { + const paths: string[] = []; + for (const match of stripComments(content).matchAll(IMPORT_REQUIRE_REGEX)) { + if (match[1] && isTypeOnlyImportStatement(match[0])) continue; + const importPath = match[1] || match[2] || match[3] || match[4]; + if (importPath) paths.push(importPath); + } + return paths; +} + function getBareModuleName(importPath: string): string { if (importPath.startsWith(".") || importPath.startsWith("/")) { return ""; @@ -99,16 +190,11 @@ export function scanProjectForPackageUsage( } if (!hasPotentialMatch) return; - const matches = content.matchAll(IMPORT_REQUIRE_REGEX); const foundPackages = new Set(); - - for (const match of matches) { - const importPath = match[1] || match[2] || match[3] || match[4]; - if (importPath) { - const bare = getBareModuleName(importPath); - if (bare && packagesToLookFor.has(bare)) { - foundPackages.add(bare); - } + for (const importPath of collectRuntimeImportPaths(content)) { + const bare = getBareModuleName(importPath); + if (bare && packagesToLookFor.has(bare)) { + foundPackages.add(bare); } } @@ -162,12 +248,9 @@ export function scanAllImports(projectPath: string): Map { } if (!content.includes("import") && !content.includes("require") && !content.includes("export")) return; - const matches = content.matchAll(IMPORT_REQUIRE_REGEX); const relPath = path.relative(projectPath, filePath); - for (const match of matches) { - const importPath = match[1] || match[2] || match[3] || match[4]; - if (!importPath) continue; + for (const importPath of collectRuntimeImportPaths(content)) { const bare = getBareModuleName(importPath); if (!bare) continue; const existing = results.get(bare) ?? []; diff --git a/src/utils/package-json.ts b/src/utils/package-json.ts index 87e85eb0..a15cdc19 100644 --- a/src/utils/package-json.ts +++ b/src/utils/package-json.ts @@ -30,6 +30,30 @@ export function readDirectDependencyNames(projectPath: string, prodOnly: boolean } } +export type WorkspaceMemberManifest = { + /** Member directory relative to the project root, POSIX-separated (e.g. "apps/web"). */ + dir: string; + manifest: Record; +}; + +// Every workspace member declared by the root manifest's `workspaces` field or +// by pnpm-workspace.yaml, with its parsed package.json. Empty for a +// single-package project. Best-effort: unreadable members are skipped. +export function readWorkspaceMemberManifests(projectPath: string): WorkspaceMemberManifest[] { + const rootManifest = readPackageJsonObject(path.join(projectPath, "package.json")); + if (!rootManifest) return []; + + const members: WorkspaceMemberManifest[] = []; + for (const packageJsonPath of resolveWorkspacePackageJsonPaths(projectPath, readWorkspacePatterns(rootManifest, projectPath))) { + const manifest = readPackageJsonObject(packageJsonPath); + if (!manifest) continue; + const dir = path.relative(projectPath, path.dirname(packageJsonPath)).split(path.sep).join("/"); + if (!dir || dir === ".") continue; + members.push({ dir, manifest }); + } + return members; +} + function readPackageJsonObject(filePath: string): Record | null { try { const raw = JSON.parse(fs.readFileSync(filePath, "utf8")); diff --git a/tests/overrides/context-builder.test.ts b/tests/overrides/context-builder.test.ts index d3e0eade..19920505 100644 --- a/tests/overrides/context-builder.test.ts +++ b/tests/overrides/context-builder.test.ts @@ -73,6 +73,78 @@ describe("buildOverrideContext", () => { expect(ctx.importedPackageNames.get("semver")).toEqual([expect.stringMatching(/src.index\.ts/)]); }); + it("collects workspace members and their declared packages for a pnpm workspace root (#966)", () => { + writeFileSync(join(dir, "package.json"), JSON.stringify({ + name: "pd002-repro", private: true, version: "1.0.0", + })); + writeFileSync(join(dir, "pnpm-workspace.yaml"), "packages:\n - 'apps/*'\n"); + mkdirSync(join(dir, "apps", "web", "src"), { recursive: true }); + writeFileSync(join(dir, "apps", "web", "package.json"), JSON.stringify({ + name: "@repro/web", dependencies: { "js-yaml": "^4.1.0" }, devDependencies: { vitest: "^2.0.0" }, + })); + writeFileSync(join(dir, "apps", "web", "src", "index.ts"), "import yaml from 'js-yaml';"); + + const ctx = buildOverrideContext(dir, { + auditLog: NULL_AUDIT_LOG, + logger: makeNoopLogger() as any, + checkNetwork: false, + }); + + expect(ctx.workspaceMembers).toHaveLength(1); + expect(ctx.workspaceMembers![0]!.dir).toBe("apps/web"); + expect([...ctx.workspaceMembers![0]!.declared].sort()).toEqual(["js-yaml", "vitest"]); + expect(ctx.importedPackageNames.get("js-yaml")).toEqual([expect.stringMatching(/apps.web.src.index\.ts/)]); + }); + + it("collects workspace members from the root manifest's workspaces field", () => { + writeFileSync(join(dir, "package.json"), JSON.stringify({ + name: "root", workspaces: ["packages/*"], + })); + mkdirSync(join(dir, "packages", "a"), { recursive: true }); + mkdirSync(join(dir, "packages", "b"), { recursive: true }); + writeFileSync(join(dir, "packages", "a", "package.json"), JSON.stringify({ name: "a", dependencies: { lodash: "^4" } })); + writeFileSync(join(dir, "packages", "b", "package.json"), JSON.stringify({ name: "b", peerDependencies: { react: "^18" } })); + + const ctx = buildOverrideContext(dir, { + auditLog: NULL_AUDIT_LOG, + logger: makeNoopLogger() as any, + checkNetwork: false, + }); + + expect(ctx.workspaceMembers!.map((m) => [m.dir, [...m.declared]])).toEqual( + expect.arrayContaining([["packages/a", ["lodash"]], ["packages/b", ["react"]]]), + ); + }); + + it("leaves workspaceMembers empty for a single-package project", () => { + writeFileSync(join(dir, "package.json"), JSON.stringify({ name: "x" })); + + const ctx = buildOverrideContext(dir, { + auditLog: NULL_AUDIT_LOG, + logger: makeNoopLogger() as any, + checkNetwork: false, + }); + + expect(ctx.workspaceMembers).toEqual([]); + }); + + it("does not count a JSDoc import() type annotation as an import (#966)", () => { + writeFileSync(join(dir, "package.json"), JSON.stringify({ name: "x" })); + writeFileSync(join(dir, "postcss.config.mjs"), [ + "/** @type {import('postcss-load-config').Config} */", + "const config = { plugins: { tailwindcss: {}, autoprefixer: {} } };", + "export default config;", + ].join("\n")); + + const ctx = buildOverrideContext(dir, { + auditLog: NULL_AUDIT_LOG, + logger: makeNoopLogger() as any, + checkNetwork: false, + }); + + expect(ctx.importedPackageNames.has("postcss-load-config")).toBe(false); + }); + it("flags OA001/OA004/OA006/OA008 as skipped when node_modules is absent", () => { writeFileSync(join(dir, "package.json"), JSON.stringify({ name: "x", diff --git a/tests/overrides/detectors/pd001.test.ts b/tests/overrides/detectors/pd001.test.ts index 079a7e9d..ca3f6177 100644 --- a/tests/overrides/detectors/pd001.test.ts +++ b/tests/overrides/detectors/pd001.test.ts @@ -93,4 +93,36 @@ describe("PD001 - override-only phantom", () => { ctx.skippedDetectors = [{ ruleId: "PD001", reason: "no source files" }]; expect(detect(ctx)).toHaveLength(0); }); + + describe("workspace roots (#966)", () => { + it("does NOT fire when the importing workspace member declares the package", () => { + const ctx = ctxOf(["js-yaml"], {}, [["js-yaml", ["apps/web/src/index.ts"]]]); + ctx.workspaceMembers = [{ dir: "apps/web", declared: new Set(["js-yaml"]) }]; + expect(detect(ctx)).toHaveLength(0); + }); + + it("fires for a member that imports the package without declaring it", () => { + const ctx = ctxOf(["js-yaml"], {}, [["js-yaml", ["apps/api/src/index.ts"]]]); + ctx.workspaceMembers = [ + { dir: "apps/web", declared: new Set(["js-yaml"]) }, + { dir: "apps/api", declared: new Set() }, + ]; + const findings = detect(ctx); + expect(findings).toHaveLength(1); + expect(findings[0]!.details).toContain("apps/api/src/index.ts"); + }); + + it("only lists the files whose member does not declare the package", () => { + const ctx = ctxOf(["js-yaml"], {}, [["js-yaml", ["apps/web/src/index.ts", "apps/api/src/index.ts", "scripts/build.ts"]]]); + ctx.workspaceMembers = [ + { dir: "apps/web", declared: new Set(["js-yaml"]) }, + { dir: "apps/api", declared: new Set() }, + ]; + const findings = detect(ctx); + expect(findings).toHaveLength(1); + expect(findings[0]!.details).toContain("apps/api/src/index.ts"); + expect(findings[0]!.details).toContain("scripts/build.ts"); + expect(findings[0]!.details).not.toContain("apps/web/src/index.ts"); + }); + }); }); diff --git a/tests/overrides/detectors/pd002.test.ts b/tests/overrides/detectors/pd002.test.ts index 80d583f4..54bdce33 100644 --- a/tests/overrides/detectors/pd002.test.ts +++ b/tests/overrides/detectors/pd002.test.ts @@ -10,6 +10,7 @@ function ctxOf(opts: { lockfile?: string[]; imported?: [string, string[]][]; pm?: OverrideContext["packageManager"]; + workspaceMembers?: OverrideContext["workspaceMembers"]; }): OverrideContext { const overrideEntries: OverrideEntry[] = (opts.overrides ?? []).map((name) => ({ key: name, packageName: name, value: "1.0.0", @@ -28,6 +29,7 @@ function ctxOf(opts: { registryDistTags: new Map(), skippedDetectors: [], importedPackageNames: new Map(opts.imported ?? []), + workspaceMembers: opts.workspaceMembers, auditLog: NULL_AUDIT_LOG, logger: noopLogger, }; @@ -105,4 +107,96 @@ describe("PD002 - transitive-only phantom", () => { ctx.skippedDetectors = [{ ruleId: "PD002", reason: "no source files" }]; expect(detect(ctx)).toHaveLength(0); }); + + describe("workspace roots (#966)", () => { + // Minimal repro from the issue: a pnpm workspace root that declares nothing, + // apps/web declares js-yaml and imports it. Scanning the root must be clean. + it("does NOT fire when the importing workspace member declares the package", () => { + const findings = detect(ctxOf({ + pm: "pnpm", + lockfile: ["js-yaml"], + imported: [["js-yaml", ["apps/web/src/index.ts"]]], + workspaceMembers: [{ dir: "apps/web", declared: new Set(["js-yaml"]) }], + })); + expect(findings).toHaveLength(0); + }); + + it("still fires for a member that imports without declaring", () => { + const findings = detect(ctxOf({ + pm: "pnpm", + lockfile: ["js-yaml"], + imported: [["js-yaml", ["apps/api/src/index.ts"]]], + workspaceMembers: [ + { dir: "apps/web", declared: new Set(["js-yaml"]) }, + { dir: "apps/api", declared: new Set() }, + ], + })); + expect(findings).toHaveLength(1); + expect(findings[0]!.details).toContain("apps/api/src/index.ts"); + }); + + it("still fires for files outside any member when the root does not declare", () => { + const findings = detect(ctxOf({ + lockfile: ["js-yaml"], + imported: [["js-yaml", ["scripts/release.ts"]]], + workspaceMembers: [{ dir: "apps/web", declared: new Set(["js-yaml"]) }], + })); + expect(findings).toHaveLength(1); + expect(findings[0]!.details).toContain("scripts/release.ts"); + }); + + it("lists only the undeclared files when a package is imported across members", () => { + const findings = detect(ctxOf({ + lockfile: ["react"], + imported: [["react", ["apps/web/src/app.tsx", "apps/api/src/render.tsx"]]], + workspaceMembers: [ + { dir: "apps/web", declared: new Set(["react"]) }, + { dir: "apps/api", declared: new Set() }, + ], + })); + expect(findings).toHaveLength(1); + expect(findings[0]!.details).toContain("apps/api/src/render.tsx"); + expect(findings[0]!.details).not.toContain("apps/web/src/app.tsx"); + }); + + it("resolves against the deepest enclosing member for nested workspaces", () => { + const findings = detect(ctxOf({ + lockfile: ["lodash"], + imported: [["lodash", ["packages/core/plugins/extra/src/index.ts"]]], + workspaceMembers: [ + { dir: "packages/core", declared: new Set(["lodash"]) }, + { dir: "packages/core/plugins/extra", declared: new Set() }, + ], + })); + expect(findings).toHaveLength(1); + }); + + it("does not treat a sibling directory with a shared prefix as the member", () => { + const findings = detect(ctxOf({ + lockfile: ["lodash"], + imported: [["lodash", ["apps/web-admin/src/index.ts"]]], + workspaceMembers: [{ dir: "apps/web", declared: new Set(["lodash"]) }], + })); + expect(findings).toHaveLength(1); + }); + + it("a root declaration satisfies every member", () => { + const findings = detect(ctxOf({ + declared: { lodash: "^4.0.0" }, + lockfile: ["lodash"], + imported: [["lodash", ["apps/web/src/index.ts"]]], + workspaceMembers: [{ dir: "apps/web", declared: new Set() }], + })); + expect(findings).toHaveLength(0); + }); + + it("accepts Windows-style separators in import file paths", () => { + const findings = detect(ctxOf({ + lockfile: ["js-yaml"], + imported: [["js-yaml", ["apps\\web\\src\\index.ts"]]], + workspaceMembers: [{ dir: "apps/web", declared: new Set(["js-yaml"]) }], + })); + expect(findings).toHaveLength(0); + }); + }); }); diff --git a/tests/usage.test.ts b/tests/usage.test.ts index fd4901d6..db1c67ad 100644 --- a/tests/usage.test.ts +++ b/tests/usage.test.ts @@ -32,7 +32,7 @@ describe("scanProjectForPackageUsage", () => { createTestFile("src/other.ts", ` import 'side-effect-pkg'; - import type { Foo } from '@scope/types'; + import { Foo } from '@scope/types'; `); const packagesToLookFor = new Set([ @@ -57,6 +57,43 @@ describe("scanProjectForPackageUsage", () => { expect(results["not-found-pkg"].length).toBe(0); }); + it("does not count type-only imports or commented-out imports as usage (#966)", () => { + createTestFile("src/types.ts", ` + import type { Foo } from 'type-only-pkg'; + import { type A, type B } from 'inline-type-only-pkg'; + export type { Bar } from 'reexported-type-pkg'; + import Runtime, { type Shape } from 'mixed-default-pkg'; + import { type Shape2, helper } from 'mixed-named-pkg'; + // import legacy from 'line-commented-pkg'; + /* const old = require('block-commented-pkg'); */ + const url = 'https://example.com'; import live from 'after-string-pkg'; + `); + createTestFile("postcss.config.mjs", ` + /** @type {import('postcss-load-config').Config} */ + const config = { plugins: {} }; + export default config; + `); + + const results = scanProjectForPackageUsage(tempDir, new Set([ + "type-only-pkg", "inline-type-only-pkg", "reexported-type-pkg", "mixed-default-pkg", + "mixed-named-pkg", "line-commented-pkg", "block-commented-pkg", "after-string-pkg", + "postcss-load-config", + ])); + + expect(results["type-only-pkg"]).toHaveLength(0); + expect(results["inline-type-only-pkg"]).toHaveLength(0); + expect(results["reexported-type-pkg"]).toHaveLength(0); + expect(results["line-commented-pkg"]).toHaveLength(0); + expect(results["block-commented-pkg"]).toHaveLength(0); + expect(results["postcss-load-config"]).toHaveLength(0); + + // A default or a non-type named specifier still loads the module. + expect(results["mixed-default-pkg"]).toHaveLength(1); + expect(results["mixed-named-pkg"]).toHaveLength(1); + // `//` inside a string literal is not a comment. + expect(results["after-string-pkg"]).toHaveLength(1); + }); + it("should ignore node_modules and other configured directories", () => { createTestFile("node_modules/bad-pkg/index.js", "import 'lodash';"); createTestFile(".git/hooks/pre-commit", "import 'lodash';"); @@ -164,6 +201,50 @@ describe("scanAllImports", () => { expect(result.has("bar")).toBe(false); }); + it("ignores JSDoc import() type annotations and type-only imports (#966)", () => { + // Stock create-next-app + Tailwind 3 postcss config: the only reference to + // postcss-load-config in the repo, and it is erased at compile time. + createFile("postcss.config.mjs", [ + "/** @type {import('postcss-load-config').Config} */", + "const config = { plugins: { tailwindcss: {}, autoprefixer: {} } };", + "export default config;", + ].join("\n")); + createFile("src/types.ts", [ + "import type { Options } from 'type-only-pkg';", + "import { type Config } from 'inline-type-only-pkg';", + "export type { Result } from 'reexported-type-pkg';", + "import { parse } from 'runtime-pkg';", + "import type from 'default-named-type-pkg';", + ].join("\n")); + + const result = scanAllImports(tempDir); + + expect(result.has("postcss-load-config")).toBe(false); + expect(result.has("type-only-pkg")).toBe(false); + expect(result.has("inline-type-only-pkg")).toBe(false); + expect(result.has("reexported-type-pkg")).toBe(false); + expect(result.get("runtime-pkg")).toEqual([expect.stringMatching(/src.types\.ts/)]); + // A default import that happens to be called `type` is a real import. + expect(result.get("default-named-type-pkg")).toEqual([expect.stringMatching(/src.types\.ts/)]); + }); + + it("does not treat // inside string literals as a comment", () => { + createFile("src/index.ts", [ + "const base = 'https://registry.example.com'; import a from 'pkg-a';", + "const tpl = `//not-a-comment`; import b from 'pkg-b';", + "// import c from 'pkg-c';", + "/* import d from 'pkg-d'; */ import e from 'pkg-e';", + ].join("\n")); + + const result = scanAllImports(tempDir); + + expect(result.has("pkg-a")).toBe(true); + expect(result.has("pkg-b")).toBe(true); + expect(result.has("pkg-c")).toBe(false); + expect(result.has("pkg-d")).toBe(false); + expect(result.has("pkg-e")).toBe(true); + }); + it("excludes example, test, and fixture imports", () => { createFile("examples/pd001-override-phantom/src/index.ts", `import yaml from 'js-yaml';`); createFile("tests/usage.test.ts", `const fixture = \`import yaml from 'js-yaml';\`;`); diff --git a/website/docs/override-hygiene/pd001.md b/website/docs/override-hygiene/pd001.md index 07b067ac..6e780d6c 100644 --- a/website/docs/override-hygiene/pd001.md +++ b/website/docs/override-hygiene/pd001.md @@ -75,6 +75,17 @@ After declaring the dependency, the override pin may still be needed if you requ --- +## What counts as an import + +PD001 only considers references that load the package at runtime: static `import ... from`, `export ... from`, side-effect imports, `require()`, and dynamic `import()`. It does not count: + +- **Type-only imports** - `import type { X } from "pkg"`, `export type { X } from "pkg"`, or a specifier list where every entry is `type`-prefixed. These are erased by the TypeScript compiler. +- **Anything inside a comment** - including JSDoc annotations such as `/** @type {import("postcss-load-config").Config} */`. + +In a workspace (npm/Yarn/Bun `workspaces`, or `pnpm-workspace.yaml`), an import found under a member directory is resolved against that member's `package.json` first, then the root. A dependency declared in `apps/web/package.json` is a real direct dependency for `apps/web/src/*`, not a phantom. + +--- + ## Relationship to OA009 If a package triggers PD001, OA009 (Stale Floor) will not fire on the same override entry. OA009 recommends removing redundant override floors - but if the override is the only thing keeping the package available to source imports, removing it would break the import. The OA009 safety guard suppresses that finding when the package is undeclared. diff --git a/website/docs/override-hygiene/pd002.md b/website/docs/override-hygiene/pd002.md index d35042b2..7d628fb1 100644 --- a/website/docs/override-hygiene/pd002.md +++ b/website/docs/override-hygiene/pd002.md @@ -80,6 +80,17 @@ pnpm add -D semver --- +## What counts as an import + +PD002 only considers references that load the package at runtime: static `import ... from`, `export ... from`, side-effect imports, `require()`, and dynamic `import()`. It does not count: + +- **Type-only imports** - `import type { X } from "pkg"`, `export type { X } from "pkg"`, or a specifier list where every entry is `type`-prefixed. These are erased by the TypeScript compiler. +- **Anything inside a comment** - including JSDoc annotations such as `/** @type {import("postcss-load-config").Config} */`. + +In a workspace (npm/Yarn/Bun `workspaces`, or `pnpm-workspace.yaml`), an import found under a member directory is resolved against that member's `package.json` first, then the root. Scanning a monorepo root does not report a member's own declared dependencies as phantoms; the finding lists only the files whose owning package leaves the import undeclared. + +--- + ## PD001 vs PD002 | Aspect | PD001 | PD002 |