Merge remote-tracking branch 'origin/feat/type-patch-command' into feat/type-patch-command

# Conflicts:
#	packages/compiler/src/frontend/type-patches/command.ts
#	packages/compiler/test/type-patches-command.test.ts

packages/compiler/src/frontend/type-patches/command.ts

@@ -131,13 +131,15 @@ interface FileMatch extends PatchMatch {
 }
 
 /** Every declaration a selector matches in a copy, and the problems where
- * it cannot apply. */
+ * it cannot apply (`always`: a problem even where the selector matched
+ * elsewhere, such as an ambiguous replacement, which leaves those
+ * declarations as they are). */
 function matchCopy(
   copy: PackageCopy,
   selector: Selector,
   op: TypePatchOp,
   parse: TypePatchParse,
-): { matches: FileMatch[]; problems: string[] } {
+): { matches: FileMatch[]; problems: { message: string; always: boolean }[] } {
   const words =
     selector.root === "export="
       ? ["exports", "export"]
@@ -145,7 +147,7 @@ function matchCopy(
         ? ["default"]
         : [selector.root];
   const matches: FileMatch[] = [];
-  const problems: string[] = [];
+  const problems: { message: string; always: boolean }[] = [];
   for (const rel of packageFiles(copy.root)) {
     if (selector.file !== null && selector.file !== rel) continue;
     const path = join(copy.root, rel);
@@ -165,7 +167,8 @@ function matchCopy(
       continue;
     }
     for (const m of result.matches) matches.push({ ...m, file: rel });
-    for (const p of result.problems) problems.push(`${p.message} (${rel})`);
+    for (const p of result.problems)
+      problems.push({ message: `${p.message} (${rel})`, always: p.always === true });
   }
   return { matches, problems };
 }
@@ -258,7 +261,7 @@ function patchCommand(
   const lines: string[] = [];
   let matched = 0;
   let changed = 0;
-  const problems: string[] = [];
+  const problems: { message: string; always: boolean }[] = [];
   for (const copy of copies) {
     const found = matchCopy(copy, selector, op, parse);
     problems.push(...found.problems);
@@ -272,9 +275,16 @@ function patchCommand(
   }
   if (matched === 0) {
     const why =
-      problems.length > 0 ? problems.join("\n") : `'${req.selector}' matches no declaration`;
+      problems.length > 0
+        ? problems.map((p) => p.message).join("\n")
+        : `'${req.selector}' matches no declaration`;
     return fail(`${why}\nnothing was written`);
   }
+  // A declaration the patch cannot apply to where it also matched (an
+  // ambiguous replacement) would stay as it is.
+  const unapplied = problems.filter((p) => p.always);
+  if (unapplied.length > 0)
+    return fail(`${unapplied.map((p) => p.message).join("\n")}\nnothing was written`);
   if (changed === 0) {
     lines.push("the declarations already say this; nothing was written");
     return { stdout: `${lines.join("\n")}\n`, stderr: "", exitCode: 0 };
@@ -286,15 +296,30 @@ function patchCommand(
   } catch {
     return fail(`${relative(req.cwd, manifest) || "package.json"} is not valid JSON`);
   }
-  // An existing entry whose range covers the installed version takes the
-  // patch; otherwise a new one under the range asked for.
-  const existing = parseProjectConfig(manifest, text).typePatches.find(
-    (e: TypePatchEntry) =>
-      e.pkg === req.pkg &&
-      (req.range === null
-        ? version !== null && satisfies(version, e.range)
-        : e.range === req.range),
+  // The compiler applies every entry whose range covers the installed
+  // version, and a later entry wins on the same selector. Without a range,
+  // the patch goes to the last covering entry that patches the selector
+  // already, else to the first covering entry; with one, to the entry of
+  // that range. Otherwise a new entry, which comes last, takes it.
+  const entries = parseProjectConfig(manifest, text).typePatches.filter(
+    (e: TypePatchEntry) => e.pkg === req.pkg,
   );
+  const covers = (e: TypePatchEntry): boolean => version !== null && satisfies(version, e.range);
+  const declares = (e: TypePatchEntry): boolean =>
+    e.declarations.some((d) => d.selector === req.selector);
+  const existing =
+    req.range === null
+      ? (entries.findLast((e) => covers(e) && declares(e)) ?? entries.find(covers))
+      : entries.find((e) => e.range === req.range);
+  if (existing !== undefined && covers(existing)) {
+    const later = entries
+      .slice(entries.indexOf(existing) + 1)
+      .find((e) => covers(e) && declares(e));
+    if (later !== undefined)
+      return fail(
+        `scriptc.typePatches["${later.key}"] also patches '${req.selector}' for ${req.pkg} ${version} and takes precedence over scriptc.typePatches["${existing.key}"]\nnothing was written`,
+      );
+  }
   const key =
     existing?.key ?? `${req.pkg}@${req.range ?? (version === null ? "*" : `^${version}`)}`;
   const where = `scriptc.typePatches["${key}"].declarations["${req.selector}"]`;

packages/compiler/test/type-patches-command.test.ts

@@ -4,7 +4,7 @@
  * nearest package.json under a range covering the installed version (or
  * an existing entry that covers it), and nothing is written when the
  * selector matches nothing. */
-import { cpSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
+import { cpSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { afterEach, beforeEach, expect, test } from "vitest";
@@ -118,3 +118,51 @@ test("list shows each patch and what it matches", () => {
     "termctx@^2.0.0\n  does not apply to the installed termctx 1.2.0\n  A.b: | undefined\n",
   );
 });
+
+test("nothing is written when a replacement is ambiguous in one of the files it matches", () => {
+  const pkg = join(root, "node_modules/overloads");
+  mkdirSync(pkg, { recursive: true });
+  writeFileSync(
+    join(pkg, "package.json"),
+    JSON.stringify({ name: "overloads", version: "1.0.0", types: "index.d.ts" }),
+  );
+  writeFileSync(join(pkg, "index.d.ts"), "export declare function pick(a: string): string;\n");
+  writeFileSync(
+    join(pkg, "more.d.ts"),
+    "export declare function pick(a: string): string;\nexport declare function pick(a: number): string;\n",
+  );
+  const result = patch("overloads", "pick()", "string | undefined");
+  expect(result.exitCode).toBe(1);
+  expect(result.stderr).toBe(
+    "scriptc: 'pick()' matches 2 declarations in more.d.ts; a replacement must name one " +
+      "(widen them, or replace the declaration file) (more.d.ts)\nnothing was written\n",
+  );
+  expect(JSON.parse(readFileSync(join(root, "package.json"), "utf8")).scriptc).toBeUndefined();
+});
+
+test("the patch goes to the covering entry the compiler applies", () => {
+  const manifest = join(root, "package.json");
+  const entries = {
+    "termctx@^1.0.0": { declarations: { "OutputContext.width": "| null" } },
+    "termctx@1.x": { declarations: { "OutputContext.width": "| null" } },
+  };
+  writeFileSync(manifest, JSON.stringify({ name: "app", scriptc: { typePatches: entries } }));
+  // Both ranges cover termctx 1.2.0, and the later entry wins on the same
+  // selector: it takes the patch.
+  const result = patch("termctx", "OutputContext.width", "| undefined");
+  expect(result.exitCode).toBe(0);
+  expect(result.stdout).toContain(
+    `wrote package.json scriptc.typePatches["termctx@1.x"].declarations["OutputContext.width"]\n`,
+  );
+  expect(JSON.parse(readFileSync(manifest, "utf8")).scriptc.typePatches).toEqual({
+    "termctx@^1.0.0": { declarations: { "OutputContext.width": "| null" } },
+    "termctx@1.x": { declarations: { "OutputContext.width": "| undefined" } },
+  });
+  // Asked for the earlier range, the later entry would still win.
+  const earlier = patch("termctx", "OutputContext.width", "| undefined", { range: "^1.0.0" });
+  expect(earlier.exitCode).toBe(1);
+  expect(earlier.stderr).toBe(
+    `scriptc: scriptc.typePatches["termctx@1.x"] also patches 'OutputContext.width' for termctx 1.2.0 ` +
+      `and takes precedence over scriptc.typePatches["termctx@^1.0.0"]\nnothing was written\n`,
+  );
+});