fixup! fs: remove permissive rmdir recursive

Author: aduh95

doc/api/deprecations.md

@@ -2664,7 +2664,7 @@ The [`crypto.Certificate()` constructor][] is deprecated. Use
 <!-- YAML
 changes:
   - version: REPLACEME
-    pr-url: https://github.com/nodejs/node/pull/
+    pr-url: https://github.com/nodejs/node/pull/37216
     description: End-of-Life.
   - version: v15.0.0
     pr-url: https://github.com/nodejs/node/pull/35562
@@ -2676,9 +2676,12 @@ changes:
 
 Type: End-of-Life
 
-`fs.rmdir(path, { recursive: true })` will throw
+`fs.rmdir(path, { recursive: true })`, `fs.rmdirSync(path, { recursive:true })`
+and `fs.promises.rmdir(path, { recursive:true })` will throw
 if `path` does not exist or is a file.
-Use `fs.rm(path, { recursive: true, force: true })` instead.
+Use `fs.rm(path, { recursive: true, force: true })`,
+`fs.rmSync(path, { recursive: true, force: true })` or
+`fs.promises.rm(path, { recursive: true, force: true })` instead.
 
 ### DEP0148: Folder mappings in `"exports"` (trailing `"/"`)
 <!-- YAML

doc/api/fs.md

@@ -3584,15 +3584,15 @@ Synchronous rename(2). Returns `undefined`.
 added: v0.0.2
 changes:
   - version: REPLACEME
-    pr-url: https://github.com/nodejs/node/pull/
-    description: Using `fs.rmdir(path, { recursive: true })` on a `path` that is
+    pr-url: https://github.com/nodejs/node/pull/37216
+    description: "Using `fs.rmdir(path, { recursive: true })` on a `path` that is
                  a file is no longer permitted and results in an `ENOENT` error
-                 on Windows and an `ENOTDIR` error on POSIX.
+                 on Windows and an `ENOTDIR` error on POSIX."
   - version: REPLACEME
-    pr-url: https://github.com/nodejs/node/pull/
-    description: Using `fs.rmdir(path, { recursive: true })` on a `path` that
+    pr-url: https://github.com/nodejs/node/pull/37216
+    description: "Using `fs.rmdir(path, { recursive: true })` on a `path` that
                  does not exist is no longer permitted and results in a `ENOENT`
-                 error.
+                 error."
   - version:
      - v13.3.0
      - v12.16.0
@@ -3649,15 +3649,15 @@ with options `{ recursive: true, force: true }`.
 added: v0.1.21
 changes:
   - version: REPLACEME
-    pr-url: https://github.com/nodejs/node/pull/
-    description: Using `fs.rmdirSync(path, { recursive: true })` on a `path`
+    pr-url: https://github.com/nodejs/node/pull/37216
+    description: "Using `fs.rmdirSync(path, { recursive: true })` on a `path`
                  that is a file is no longer permitted and results in an
-                 `ENOENT` error on Windows and an `ENOTDIR` error on POSIX.
+                 `ENOENT` error on Windows and an `ENOTDIR` error on POSIX."
   - version: REPLACEME
-    pr-url: https://github.com/nodejs/node/pull/
-    description: Using `fs.rmdirSync(path, { recursive: true })` on a `path`
+    pr-url: https://github.com/nodejs/node/pull/37216
+    description: "Using `fs.rmdirSync(path, { recursive: true })` on a `path`
                  that does not exist is no longer permitted and results in a
-                 `ENOENT` error.
+                 `ENOENT` error."
   - version:
      - v13.3.0
      - v12.16.0
@@ -5677,6 +5677,16 @@ upon success.
 <!-- YAML
 added: v10.0.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/37216
+    description: "Using `fsPromises.rmdir(path, { recursive: true })` on a `path`
+                 that is a file is no longer permitted and results in an
+                 `ENOENT` error on Windows and an `ENOTDIR` error on POSIX."
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/37216
+    description: "Using `fsPromises.rmdir(path, { recursive: true })` on a `path`
+                 that does not exist is no longer permitted and results in a
+                 `ENOENT` error."
   - version:
      - v13.3.0
      - v12.16.0
@@ -5700,8 +5710,7 @@ changes:
     represents the number of retries. This option is ignored if the `recursive`
     option is not `true`. **Default:** `0`.
   * `recursive` {boolean} If `true`, perform a recursive directory removal. In
-    recursive mode, errors are not reported if `path` does not exist, and
-    operations are retried on failure. **Default:** `false`.
+    recursive mode, operations are retried on failure. **Default:** `false`.
   * `retryDelay` {integer} The amount of time in milliseconds to wait between
     retries. This option is ignored if the `recursive` option is not `true`.
     **Default:** `100`.
@@ -5714,11 +5723,8 @@ Using `fsPromises.rmdir()` on a file (not a directory) results in the
 `Promise` being rejected with an `ENOENT` error on Windows and an `ENOTDIR`
 error on POSIX.
 
-Setting `recursive` to `true` results in behavior similar to the Unix command
-`rm -rf`: an error will not be raised for paths that do not exist, and paths
-that represent files will be deleted. The permissive behavior of the
-`recursive` option is deprecated, `ENOTDIR` and `ENOENT` will be thrown in
-the future.
+To get a behavior similar to the `rm -rf` Unix command, use
+[`fsPromises.rm()`][] with options `{ recursive: true, force: true }`.
 
 ### `fsPromises.rm(path[, options])`
 <!-- YAML
@@ -6312,6 +6318,8 @@ the file contents.
 [`fs.readdirSync()`]: #fs_fs_readdirsync_path_options
 [`fs.readv()`]: #fs_fs_readv_fd_buffers_position_callback
 [`fs.realpath()`]: #fs_fs_realpath_path_options_callback
+[`fs.rm()`]: #fs_fs_rm_path_options_callback
+[`fs.rmSync()`]: #fs_fs_rmsync_path_options
 [`fs.rmdir()`]: #fs_fs_rmdir_path_options_callback
 [`fs.stat()`]: #fs_fs_stat_path_options_callback
 [`fs.symlink()`]: #fs_fs_symlink_target_path_type_callback
@@ -6323,6 +6331,7 @@ the file contents.
 [`fs.writev()`]: #fs_fs_writev_fd_buffers_position_callback
 [`fsPromises.open()`]: #fs_fspromises_open_path_flags_mode
 [`fsPromises.opendir()`]: #fs_fspromises_opendir_path_options
+[`fsPromises.rm()`]: #fs_fspromises_rm_path_options
 [`fsPromises.utimes()`]: #fs_fspromises_utimes_path_atime_mtime
 [`inotify(7)`]: https://man7.org/linux/man-pages/man7/inotify.7.html
 [`kqueue(2)`]: https://www.freebsd.org/cgi/man.cgi?query=kqueue&sektion=2

lib/fs.js

@@ -881,15 +881,20 @@ function rmdir(path, options, callback) {
   if (options?.recursive) {
     validateRmOptions(
       path,
-      { ...options, force: true },
+      { ...options, force: false },
       true,
       (err, options) => {
+        if (err === false) {
+          const req = new FSReqCallback();
+          req.oncomplete = callback;
+          return binding.rmdir(path, req);
+        }
         if (err) {
           return callback(err);
         }
 
         lazyLoadRimraf();
-        return rimraf(path, options, callback);
+        rimraf(path, options, callback);
       });
   } else {
     validateRmdirOptions(options);
@@ -903,12 +908,15 @@ function rmdirSync(path, options) {
   path = getValidatedPath(path);
 
   if (options?.recursive) {
-    options = validateRmOptionsSync(path, { ...options, force: true }, true);
-    lazyLoadRimraf();
-    return rimrafSync(pathModule.toNamespacedPath(path), options);
+    options = validateRmOptionsSync(path, { ...options, force: false }, true);
+    if (options !== false) {
+      lazyLoadRimraf();
+      return rimrafSync(pathModule.toNamespacedPath(path), options);
+    }
+  } else {
+    validateRmdirOptions(options);
   }
 
-  validateRmdirOptions(options);
   const ctx = { path };
   binding.rmdir(pathModule.toNamespacedPath(path), undefined, ctx);
   return handleErrorFromBinding(ctx);

lib/internal/fs/promises.js

@@ -488,7 +488,10 @@ async function rmdir(path, options) {
   options = validateRmdirOptions(options);
 
   if (options.recursive) {
-    return rimrafPromises(path, options);
+    const stats = await stat(path);
+    if (stats.isDirectory()) {
+      return rimrafPromises(path, options);
+    }
   }
 
   return binding.rmdir(path, kUsePromises);

lib/internal/fs/utils.js

@@ -699,7 +699,7 @@ const validateRmOptions = hideStackFrames((path, options, expectDir, cb) => {
     }
 
     if (expectDir && !stats.isDirectory()) {
-      return cb(new Error('Not a directory'));
+      return cb(false);
     }
 
     if (stats.isDirectory() && !options.recursive) {
@@ -724,7 +724,7 @@ const validateRmOptionsSync = hideStackFrames((path, options, expectDir) => {
       .statSync(path, { throwIfNoEntry: !options.force })?.isDirectory();
 
     if (expectDir && !isDirectory) {
-      throw new Error('Not a directory');
+      return false;
     }
 
     if (isDirectory && !options.recursive) {

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

@@ -7,12 +7,32 @@ const path = require('path');
 
 tmpdir.refresh();
 
+{
+  assert.throws(
+    () =>
+      fs.rmdirSync(path.join(tmpdir.path, 'noexist.txt'), { recursive: true }),
+    {
+      code: 'ENOENT',
+    }
+  );
+}
 {
   fs.rmdir(
     path.join(tmpdir.path, 'noexist.txt'),
     { recursive: true },
     common.mustCall((err) => {
-      assert.match(err.message, /Not a directory/);
+      assert.throws(() => { throw err; }, {
+        code: 'ENOENT',
+      });
     })
   );
 }
+{
+  assert.rejects(
+    () => fs.promises.rmdir(path.join(tmpdir.path, 'noexist.txt'),
+                            { recursive: true }),
+    {
+      code: 'ENOENT',
+    }
+  ).then(common.mustCall());
+}

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

@@ -7,10 +7,26 @@ const path = require('path');
 
 tmpdir.refresh();
 
+{
+  const filePath = path.join(tmpdir.path, 'rmdir-recursive.txt');
+  fs.writeFileSync(filePath, '');
+  assert.throws(() => fs.rmdirSync(filePath, { recursive: true }), {
+    code: 'ENOTDIR',
+  });
+}
 {
   const filePath = path.join(tmpdir.path, 'rmdir-recursive.txt');
   fs.writeFileSync(filePath, '');
   fs.rmdir(filePath, { recursive: true }, common.mustCall((err) => {
-    assert.match(err.message, /Not a directory/);
+    assert.throws(() => { throw err; }, {
+      code: 'ENOTDIR',
+    });
   }));
 }
+{
+  const filePath = path.join(tmpdir.path, 'rmdir-recursive.txt');
+  fs.writeFileSync(filePath, '');
+  assert.rejects(() => fs.promises.rmdir(filePath, { recursive: true }), {
+    code: 'ENOTDIR',
+  }).then(common.mustCall());
+}