fs: deprecation warning on recursive rmdir

This is a follow up to nodejs#35494 to add a deprecation warning when
using recursive rmdir. This only warns if you are attempting
to remove a file or a nonexistent path.

PR-URL: nodejs#35562
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>

doc/api/deprecations.md

@@ -2666,12 +2666,15 @@ changes:
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/35579
     description: Documentation-only deprecation.
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/35562
+    description: Runtime deprecation.
 -->
 
-Type: Documentation-only
+Type: Runtime
 
 In future versions of Node.js, `fs.rmdir(path, { recursive: true })` will throw
-on nonexistent paths, or when given a file as a target.
+if `path` does not exist or is a file.
 Use `fs.rm(path, { recursive: true, force: true })` instead.
 
 [Legacy URL API]: url.md#url_legacy_url_api

lib/fs.js

@@ -859,14 +859,18 @@ function rmdir(path, options, callback) {
   path = pathModule.toNamespacedPath(getValidatedPath(path));
 
   if (options && options.recursive) {
-    validateRmOptions(path, { ...options, force: true }, (err, options) => {
-      if (err) {
-        return callback(err);
-      }
+    options = validateRmOptions(
+      path,
+      { ...options, force: true },
+      true,
+      (err, options) => {
+        if (err) {
+          return callback(err);
+        }
 
-      lazyLoadRimraf();
-      return rimraf(path, options, callback);
-    });
+        lazyLoadRimraf();
+        return rimraf(path, options, callback);
+      });
   } else {
     validateRmdirOptions(options);
     const req = new FSReqCallback();
@@ -879,7 +883,7 @@ function rmdirSync(path, options) {
   path = getValidatedPath(path);
 
   if (options && options.recursive) {
-    options = validateRmOptionsSync(path, { ...options, force: true });
+    options = validateRmOptionsSync(path, { ...options, force: true }, true);
     lazyLoadRimraf();
     return rimrafSync(pathModule.toNamespacedPath(path), options);
   }
@@ -896,7 +900,7 @@ function rm(path, options, callback) {
     options = undefined;
   }
 
-  validateRmOptions(path, options, (err, options) => {
+  validateRmOptions(path, options, false, (err, options) => {
     if (err) {
       return callback(err);
     }
@@ -906,7 +910,7 @@ function rm(path, options, callback) {
 }
 
 function rmSync(path, options) {
-  options = validateRmOptionsSync(path, options);
+  options = validateRmOptionsSync(path, options, false);
 
   lazyLoadRimraf();
   return rimrafSync(pathModule.toNamespacedPath(path), options);

lib/internal/fs/promises.js

@@ -421,7 +421,7 @@ async function ftruncate(handle, len = 0) {
 
 async function rm(path, options) {
   path = pathModule.toNamespacedPath(getValidatedPath(path));
-  options = await validateRmOptionsPromise(path, options);
+  options = await validateRmOptionsPromise(path, options, false);
   return rimrafPromises(path, options);
 }
 

lib/internal/fs/utils.js

@@ -672,18 +672,25 @@ const defaultRmdirOptions = {
   recursive: false,
 };
 
-const validateRmOptions = hideStackFrames((path, options, callback) => {
+const validateRmOptions = hideStackFrames((path, options, warn, callback) => {
   options = validateRmdirOptions(options, defaultRmOptions);
   validateBoolean(options.force, 'options.force');
 
   lazyLoadFs().stat(path, (err, stats) => {
     if (err) {
       if (options.force && err.code === 'ENOENT') {
+        if (warn) {
+          emitPermissiveRmdirWarning();
+        }
         return callback(null, options);
       }
       return callback(err, options);
     }
 
+    if (warn && !stats.isDirectory()) {
+      emitPermissiveRmdirWarning();
+    }
+
     if (stats.isDirectory() && !options.recursive) {
       return callback(new ERR_FS_EISDIR({
         code: 'EISDIR',
@@ -697,13 +704,17 @@ const validateRmOptions = hideStackFrames((path, options, callback) => {
   });
 });
 
-const validateRmOptionsSync = hideStackFrames((path, options) => {
+const validateRmOptionsSync = hideStackFrames((path, options, warn) => {
   options = validateRmdirOptions(options, defaultRmOptions);
   validateBoolean(options.force, 'options.force');
 
   try {
     const stats = lazyLoadFs().statSync(path);
 
+    if (warn && !stats.isDirectory()) {
+      emitPermissiveRmdirWarning();
+    }
+
     if (stats.isDirectory() && !options.recursive) {
       throw new ERR_FS_EISDIR({
         code: 'EISDIR',
@@ -718,12 +729,29 @@ const validateRmOptionsSync = hideStackFrames((path, options) => {
       throw err;
     } else if (err.code === 'ENOENT' && !options.force) {
       throw err;
+    } else if (warn && err.code === 'ENOENT') {
+      emitPermissiveRmdirWarning();
     }
   }
 
   return options;
 });
 
+let permissiveRmdirWarned = false;
+
+function emitPermissiveRmdirWarning() {
+  if (!permissiveRmdirWarned) {
+    process.emitWarning(
+      'In future versions of Node.js, fs.rmdir(path, { recursive: true }) ' +
+      'will throw if path does not exist or is a file. Use fs.rm(path, ' +
+      '{ recursive: true, force: true }) instead',
+      'DeprecationWarning',
+      'DEP0147'
+    );
+    permissiveRmdirWarned = true;
+  }
+}
+
 const validateRmdirOptions = hideStackFrames(
   (options, defaults = defaultRmdirOptions) => {
     if (options === undefined)

test/parallel/test-fs-rmdir-recursive-warns-not-found.js

@@ -0,0 +1,24 @@
+'use strict';
+const common = require('../common');
+const tmpdir = require('../common/tmpdir');
+const fs = require('fs');
+const path = require('path');
+
+tmpdir.refresh();
+
+
+{
+  // Should warn when trying to delete a nonexistent path
+  common.expectWarning(
+    'DeprecationWarning',
+    'In future versions of Node.js, fs.rmdir(path, { recursive: true }) ' +
+    'will throw if path does not exist or is a file. Use fs.rm(path, ' +
+    '{ recursive: true, force: true }) instead',
+    'DEP0147'
+  );
+  fs.rmdir(
+    path.join(tmpdir.path, 'noexist.txt'),
+    { recursive: true },
+    common.mustCall()
+  );
+}

test/parallel/test-fs-rmdir-recursive-warns-on-file.js

@@ -0,0 +1,21 @@
+'use strict';
+const common = require('../common');
+const tmpdir = require('../common/tmpdir');
+const fs = require('fs');
+const path = require('path');
+
+tmpdir.refresh();
+
+{
+  // Should warn when trying to delete a file
+  common.expectWarning(
+    'DeprecationWarning',
+    'In future versions of Node.js, fs.rmdir(path, { recursive: true }) ' +
+    'will throw if path does not exist or is a file. Use fs.rm(path, ' +
+    '{ recursive: true, force: true }) instead',
+    'DEP0147'
+  );
+  const filePath = path.join(tmpdir.path, 'rmdir-recursive.txt');
+  fs.writeFileSync(filePath, '');
+  fs.rmdirSync(filePath, { recursive: true });
+}