fix(ext/node): fs.rename and fs.renameSync compatibility (#30245)

ext/node/polyfills/_fs/_fs_rename.ts

@@ -1,32 +1,63 @@
 // Copyright 2018-2025 the Deno authors. MIT license.
 
-// TODO(petamoriken): enable prefer-primordials for node polyfills
-// deno-lint-ignore-file prefer-primordials
-
-import { pathFromURL } from "ext:deno_web/00_infra.js";
 import { promisify } from "ext:deno_node/internal/util.mjs";
+import { getValidatedPathToString } from "ext:deno_node/internal/fs/utils.mjs";
+import { validateFunction } from "ext:deno_node/internal/validators.mjs";
+import { primordials } from "ext:core/mod.js";
+import type { Buffer } from "node:buffer";
+import * as pathModule from "node:path";
+import { denoErrorToNodeError } from "ext:deno_node/internal/errors.ts";
+
+const {
+  PromisePrototypeThen,
+} = primordials;
 
 export function rename(
-  oldPath: string | URL,
-  newPath: string | URL,
+  oldPath: string | Buffer | URL,
+  newPath: string | Buffer | URL,
   callback: (err?: Error) => void,
 ) {
-  oldPath = oldPath instanceof URL ? pathFromURL(oldPath) : oldPath;
-  newPath = newPath instanceof URL ? pathFromURL(newPath) : newPath;
+  oldPath = getValidatedPathToString(oldPath, "oldPath");
+  newPath = getValidatedPathToString(newPath, "newPath");
+  validateFunction(callback, "callback");
 
-  if (!callback) throw new Error("No callback function supplied");
-
-  Deno.rename(oldPath, newPath).then((_) => callback(), callback);
+  PromisePrototypeThen(
+    Deno.rename(
+      pathModule.toNamespacedPath(oldPath),
+      pathModule.toNamespacedPath(newPath),
+    ),
+    () => callback(),
+    (err: Error) =>
+      callback(denoErrorToNodeError(err, {
+        syscall: "rename",
+        path: oldPath,
+        dest: newPath,
+      })),
+  );
 }
 
 export const renamePromise = promisify(rename) as (
-  oldPath: string | URL,
-  newPath: string | URL,
+  oldPath: string | Buffer | URL,
+  newPath: string | Buffer | URL,
 ) => Promise<void>;
 
-export function renameSync(oldPath: string | URL, newPath: string | URL) {
-  oldPath = oldPath instanceof URL ? pathFromURL(oldPath) : oldPath;
-  newPath = newPath instanceof URL ? pathFromURL(newPath) : newPath;
+export function renameSync(
+  oldPath: string | Buffer | URL,
+  newPath: string | Buffer | URL,
+) {
+  oldPath = getValidatedPathToString(oldPath, "oldPath");
+  newPath = getValidatedPathToString(newPath, "newPath");
 
-  Deno.renameSync(oldPath, newPath);
+  try {
+    Deno.renameSync(
+      pathModule.toNamespacedPath(oldPath),
+      pathModule.toNamespacedPath(newPath),
+    );
+  } catch (err) {
+    throw denoErrorToNodeError(err as Error, {
+      syscall: "rename",
+      path: oldPath,
+      dest: newPath,
+    });
+  }
 }

ext/node/polyfills/internal/errors.ts

@@ -2666,6 +2666,7 @@ export class ERR_INVALID_STATE extends NodeError {
 interface UvExceptionContext {
   syscall: string;
   path?: string;
+  dest?: string;
 }
 export function denoErrorToNodeError(e: Error, ctx: UvExceptionContext) {
   const errno = extractOsErrorNumberFromErrorMessage(e);

tests/node_compat/config.toml

@@ -452,6 +452,7 @@
 "parallel/test-fs-readfilesync-enoent.js" = {}
 "parallel/test-fs-readv-promisify.js" = {}
 "parallel/test-fs-ready-event-stream.js" = {}
+"parallel/test-fs-rename-type-check.js" = {}
 "parallel/test-fs-rmdir-type-check.js" = {}
 "parallel/test-fs-stream-fs-options.js" = {}
 "parallel/test-fs-stream-options.js" = {}

tests/unit_node/_fs/_fs_rename_test.ts

@@ -4,6 +4,7 @@ import { assertCallbackErrorUncaught } from "../_test_utils.ts";
 import { rename, renameSync } from "node:fs";
 import { existsSync } from "node:fs";
 import { join, parse } from "@std/path";
+import { Buffer } from "node:buffer";
 
 Deno.test({
   name: "ASYNC: renaming a file",
@@ -50,3 +51,72 @@ Deno.test("[std/node/fs] rename callback isn't called twice if error is thrown",
     },
   });
 });
+
+Deno.test("[std/node/fs] rename: accepts Buffer paths", async () => {
+  const file = Deno.makeTempFileSync();
+  const bufferOldPath = Buffer.from(file, "utf-8");
+  const newPath = join(parse(file).dir, `${parse(file).base}_renamed`);
+  const bufferNewPath = Buffer.from(newPath, "utf-8");
+
+  await new Promise<void>((resolve, reject) => {
+    rename(bufferOldPath, bufferNewPath, (err) => {
+      if (err) reject(err);
+      resolve();
+    });
+  })
+    .then(() => {
+      assertEquals(existsSync(newPath), true);
+      assertEquals(existsSync(file), false);
+    }, () => fail());
+});
+
+Deno.test("[std/node/fs] rename: convert Deno errors to Node.js errors", async () => {
+  const dir = Deno.makeTempDirSync();
+  const oldPath = join(dir, "non_existent_file");
+  const newPath = join(dir, "new_file");
+
+  await new Promise<void>((resolve, reject) => {
+    rename(oldPath, newPath, (err) => {
+      if (err) reject(err);
+      resolve();
+    });
+  })
+    .then(() => fail())
+    .catch((err) => {
+      assertEquals(err.code, "ENOENT");
+      assertEquals(err.syscall, "rename");
+      assertEquals(err.path, oldPath);
+      assertEquals(err.dest, newPath);
+    });
+});
+
+Deno.test("[std/node/fs] renameSync: accepts Buffer paths", () => {
+  const file = Deno.makeTempFileSync();
+  const bufferOldPath = Buffer.from(file, "utf-8");
+  const newPath = join(parse(file).dir, `${parse(file).base}_renamed`);
+  const bufferNewPath = Buffer.from(newPath, "utf-8");
+
+  renameSync(bufferOldPath, bufferNewPath);
+  assertEquals(existsSync(newPath), true);
+  assertEquals(existsSync(file), false);
+});
+
+Deno.test("[std/node/fs] renameSync: convert Deno errors to Node.js errors", () => {
+  const dir = Deno.makeTempDirSync();
+  const oldPath = join(dir, "non_existent_file");
+  const newPath = join(dir, "new_file");
+
+  try {
+    renameSync(oldPath, newPath);
+    fail();
+  } catch (err) {
+    // deno-lint-ignore no-explicit-any
+    assertEquals((err as any).code, "ENOENT");
+    // deno-lint-ignore no-explicit-any
+    assertEquals((err as any).syscall, "rename");
+    // deno-lint-ignore no-explicit-any
+    assertEquals((err as any).path, oldPath);
+    // deno-lint-ignore no-explicit-any
+    assertEquals((err as any).dest, newPath);
+  }
+});