Address review feedback

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<string, Request[]>;
@@ -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),

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));
   }
 

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 });
+  }
+});