From 734ced4f4ff5ef1ca55944a127269a3433a39719 Mon Sep 17 00:00:00 2001 From: Chris Tate Date: Sun, 25 Oct 2026 13:49:34 -0500 Subject: [PATCH] Address review feedback --- .../src/frontend/type-patches/layer.ts | 23 ++++- .../src/frontend/type-patches/locate.ts | 44 ++++++++- packages/compiler/test/type-patches.test.ts | 97 ++++++++++++++++++- 3 files changed, 156 insertions(+), 8 deletions(-) diff --git a/packages/compiler/src/frontend/type-patches/layer.ts b/packages/compiler/src/frontend/type-patches/layer.ts index 48ef6ecf0..79b409110 100644 --- a/packages/compiler/src/frontend/type-patches/layer.ts +++ b/packages/compiler/src/frontend/type-patches/layer.ts @@ -59,6 +59,8 @@ interface MatchRecord extends PatchMatch { interface State { manifest: string; + /** The directory of the project's package.json. */ + projectDir: string; manifestText: string; parse: TypePatchParse; byPackage: Map; @@ -104,6 +106,7 @@ export function configureTypePatches(entryPath: string, parse: TypePatchParse): } state = { manifest: config.manifest, + projectDir: dirname(config.manifest), manifestText: config.manifestText ?? "", parse, byPackage, @@ -142,6 +145,22 @@ function versionAt(dir: string): string | null { } } +/** The version of the runtime package `pkg` an `@types` twin under + * `nodeModules` describes: the copy installed beside it, else the one the + * project resolves (pnpm keeps each package in its own directory, so a + * twin has no runtime package beside it), else null. */ +function twinRuntimeVersion(s: State, nodeModules: string, pkg: string): string | null { + const beside = versionAt(nodeModules + pkg); + if (beside !== null) return beside; + for (let dir = s.projectDir; ;) { + const version = versionAt(`${dir}/node_modules/${pkg}`); + if (version !== null) return version; + const parent = dirname(dir); + if (parent === dir) return null; + dir = parent; + } +} + /** The installed package copy a file belongs to, with the requests that * apply to it; null for files outside node_modules or of unpatched * packages. */ @@ -162,9 +181,9 @@ function copyOf(s: State, path: string): { copy: CopyInfo; rel: string } | null const pkg = name.startsWith("@types/") ? unmangleTypesName(name) : name; const requests = s.byPackage.get(pkg); if (requests !== undefined) { - // A twin describes the runtime package installed beside it. + // A twin is matched by the runtime package's version, never its own. const version = name.startsWith("@types/") - ? (versionAt(path.slice(0, at + marker.length) + pkg) ?? versionAt(root)) + ? twinRuntimeVersion(s, path.slice(0, at + marker.length), pkg) : versionAt(root); const applicable = requests.filter( (r) => version !== null && satisfies(version, r.entry.range), diff --git a/packages/compiler/src/frontend/type-patches/locate.ts b/packages/compiler/src/frontend/type-patches/locate.ts index fc85a1634..6c762f5c3 100644 --- a/packages/compiler/src/frontend/type-patches/locate.ts +++ b/packages/compiler/src/frontend/type-patches/locate.ts @@ -596,6 +596,26 @@ function parseJsDoc(text: string, start: number, end: number): JsDocComment { return { start, end, tags }; } +/** The spans of a file's string, template and regular expression literal + * tokens. */ +function literalSpans(sf: ts.SourceFile): { start: number; end: number }[] { + const out: { start: number; end: number }[] = []; + const kinds = new Set([ + ts.SyntaxKind.StringLiteral, + ts.SyntaxKind.NoSubstitutionTemplateLiteral, + ts.SyntaxKind.TemplateHead, + ts.SyntaxKind.TemplateMiddle, + ts.SyntaxKind.TemplateTail, + ts.SyntaxKind.RegularExpressionLiteral, + ]); + const visit = (n: ts.Node): void => { + if (kinds.has(n.kind)) out.push({ start: tokenStartOfNode(n), end: n.end }); + ts.forEachChild(n, visit); + }; + visit(sf); + return out.sort((a, b) => a.start - b.start); +} + const RETURN_TAGS = new Set(["returns", "return"]); const PROPERTY_TAGS = new Set(["property", "prop"]); @@ -615,15 +635,27 @@ class JsResolver { private readonly text: string, sf: ts.SourceFile, ) { + // JSDoc comments, not text inside string, template or regular + // expression literals that looks like one. + const literals = literalSpans(sf); const re = /\/\*\*(?!\/)[\s\S]*?\*\//g; - for (let m = re.exec(text); m !== null; m = re.exec(text)) + for (let m = re.exec(text); m !== null; m = re.exec(text)) { + const inside = literals.find((l) => l.start <= m!.index && m!.index < l.end); + if (inside !== undefined) { + re.lastIndex = inside.end; + continue; + } this.comments.push(parseJsDoc(text, m.index, m.index + m[0].length)); + } for (const c of this.comments) c.tags.forEach((t, i) => { if ((t.tag === "typedef" || t.tag === "callback") && t.name !== null) this.add(t.name, { kind: "typedef", comment: c, tagIndex: i, prefix: "" }); }); - for (const s of sf.statements.nodes) this.collect(s); + // Declarations first: `exports.name = name` may come before the + // function declaration it exports, which is hoisted. + for (const s of sf.statements.nodes) if (!ts.isExpressionStatement(s)) this.collect(s); + for (const s of sf.statements.nodes) if (ts.isExpressionStatement(s)) this.collect(s); } private add(name: string, pos: JsPos): void { @@ -681,9 +713,11 @@ class JsResolver { if (named !== undefined) this.exportEquals.push(...named); } } else if (ts.isIdentifier(e.right) && this.roots.has(e.right.text)) { - // `exports.name = name`: the named declaration is the export. - for (const pos of this.roots.get(e.right.text)!) - if (pos.kind === "decl") this.add(target, pos); + // `exports.name = local`: the named declaration is the export (and + // already is under its own name). + if (target !== e.right.text) + for (const pos of [...this.roots.get(e.right.text)!]) + if (pos.kind === "decl") this.add(target, pos); } else this.add(target, this.declPos(s, s, e.right)); } diff --git a/packages/compiler/test/type-patches.test.ts b/packages/compiler/test/type-patches.test.ts index 729a0da37..b4c0af9bc 100644 --- a/packages/compiler/test/type-patches.test.ts +++ b/packages/compiler/test/type-patches.test.ts @@ -2,7 +2,7 @@ * packages: commander's HelpContext.helpWidth @typedef property and ms's * @return tags, patched from the fixtures' package.json; installed-version * selection; and the warnings for patches that do not apply as written. */ -import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, expect, test } from "vitest"; @@ -122,3 +122,98 @@ test("an @types twin is matched by the runtime package's installed version", () rmSync(root, { recursive: true, force: true }); } }); + +test("an @types twin in pnpm's layout is matched by the runtime package the project resolves", () => { + // pnpm keeps each package in its own directory under node_modules/.pnpm + // and links the project's dependencies from node_modules: the twin has no + // runtime package beside it. + const root = mkdtempSync(join(tmpdir(), "scriptc-type-patches-pnpm-")); + try { + const store = join(root, "node_modules/.pnpm"); + const runtime = join(store, "tallyjs@0.5.0/node_modules/tallyjs"); + const types = join(store, "@types+tallyjs@0.4.0/node_modules/@types/tallyjs"); + mkdirSync(runtime, { recursive: true }); + mkdirSync(types, { recursive: true }); + writeFileSync(join(runtime, "package.json"), '{ "name": "tallyjs", "version": "0.5.0" }'); + writeFileSync(join(runtime, "index.js"), "exports.totals = () => ({});\n"); + writeFileSync(join(types, "package.json"), '{ "name": "@types/tallyjs", "version": "0.4.0" }'); + writeFileSync( + join(types, "index.d.ts"), + "export type Totals = {\n max: number;\n};\nexport declare function totals(): Totals;\n", + ); + mkdirSync(join(root, "node_modules/@types"), { recursive: true }); + symlinkSync(runtime, join(root, "node_modules/tallyjs")); + symlinkSync(types, join(root, "node_modules/@types/tallyjs")); + const patchFor = (range: string) => + writeFileSync( + join(root, "package.json"), + JSON.stringify({ + scriptc: { + typePatches: { + [`tallyjs@${range}`]: { declarations: { "Totals.max": "| undefined" } }, + }, + }, + }), + ); + // A range the runtime (0.5.0) satisfies applies, though the twin's own + // version (0.4.0) does not satisfy it. + patchFor("0.5.0"); + configureTypePatches(join(root, "main.ts"), parse); + expect(typePatchedText(join(types, "index.d.ts"))).toContain("max: number | undefined;"); + // A range only the twin's own version satisfies does not. + patchFor("0.4.0"); + configureTypePatches(join(root, "main.ts"), parse); + expect(typePatchedText(join(types, "index.d.ts"))).toBeUndefined(); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("an export before its hoisted function is patched, and string text that looks like JSDoc is not", () => { + const root = mkdtempSync(join(tmpdir(), "scriptc-type-patches-js-")); + try { + const pkg = join(root, "node_modules/hoistkit"); + mkdirSync(pkg, { recursive: true }); + writeFileSync(join(pkg, "package.json"), '{ "name": "hoistkit", "version": "1.0.0" }'); + const source = [ + '"use strict";', + "exports.measure = size;", + 'exports.doc = "/** @typedef {object} Opts\\n * @property {number} width */";', + "", + "/**", + " * @returns {number}", + " */", + "function size(s) {", + " return s.length || undefined;", + "}", + "", + ].join("\n"); + writeFileSync(join(pkg, "index.js"), source); + writeFileSync( + join(root, "package.json"), + JSON.stringify({ + scriptc: { + typePatches: { + "hoistkit@^1.0.0": { + declarations: { "measure()": "| undefined", "Opts.width": "| undefined" }, + }, + }, + }, + }), + ); + configureTypePatches(join(root, "main.ts"), parse); + // The export, under another name, comes before the function it exports: + // the function's @returns tag widens. + expect(typePatchedText(join(pkg, "index.js"))).toBe( + source.replace(" * @returns {number}\n", " * @returns {number | undefined}\n"), + ); + // The string's text is a value, not a typedef: the patch finds nothing. + expect( + typePatchDiagnostics()! + .diagnostics.map((d) => d.message) + .join("\n"), + ).toContain("Opts.width"); + } finally { + rmSync(root, { recursive: true, force: true }); + } +});