Revert "fs: Revert throw on invalid callbacks"

This reverts commit 8250bfd. PR-URL: #18668 Refs: #12562 Refs: #12976 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
nodejs · Feb 26, 2018 · e9f2cec · e9f2cec
1 parent 009e418
commit e9f2cec
Show file tree

Hide file tree

Showing 13 changed files with 173 additions and 116 deletions.
diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
@@ -162,9 +162,10 @@ explicitly via error event handlers set on the domain instead.
 <a id="DEP0013"></a>
 ### DEP0013: fs asynchronous function without callback
 
-Type: Runtime
+Type: End-of-Life
 
-Calling an asynchronous function without a callback is deprecated.
+Calling an asynchronous function without a callback throws a `TypeError`
+REPLACEME onwards. Refer: [PR 12562](https://github.com/nodejs/node/pull/12562)
 
 <a id="DEP0014"></a>
 ### DEP0014: fs.read legacy String interface

diff --git a/doc/api/fs.md b/doc/api/fs.md
diff --git a/lib/fs.js b/lib/fs.js
@@ -77,7 +77,6 @@ const { kMaxLength } = require('buffer');
 
 const isWindows = process.platform === 'win32';
 
-const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG);
 const errnoException = errors.errnoException;
 
 let truncateWarn = true;
@@ -106,46 +105,17 @@ function handleErrorFromBinding(ctx) {
   }
 }
 
-// TODO(joyeecheung): explore how the deprecation could be solved via linting
-// rules. See https://github.com/nodejs/node/pull/12976
-function rethrow() {
-  process.emitWarning(
-    'Calling an asynchronous function without callback is deprecated.',
-    'DeprecationWarning', 'DEP0013', rethrow
-  );
-
-  // Only enable in debug mode. A backtrace uses ~1000 bytes of heap space and
-  // is fairly slow to generate.
-  if (DEBUG) {
-    var backtrace = new Error();
-    return function(err) {
-      if (err) {
-        backtrace.stack = `${err.name}: ${err.message}` +
-                          backtrace.stack.substr(backtrace.name.length);
-        throw backtrace;
-      }
-    };
-  }
-
-  return function(err) {
-    if (err) {
-      throw err;  // Forgot a callback but don't know where? Use NODE_DEBUG=fs
-    }
-  };
-}
-
 function maybeCallback(cb) {
-  return typeof cb === 'function' ? cb : rethrow();
+  if (typeof cb === 'function')
+    return cb;
+
+  throw new errors.TypeError('ERR_INVALID_CALLBACK');
 }
 
 // Ensure that callbacks run in the global context. Only use this function
 // for callbacks that are passed to the binding layer, callbacks that are
 // invoked from JS already run in the proper scope.
 function makeCallback(cb) {
-  if (cb === undefined) {
-    return rethrow();
-  }
-
   if (typeof cb !== 'function') {
     throw new errors.TypeError('ERR_INVALID_CALLBACK');
   }
@@ -159,10 +129,6 @@ function makeCallback(cb) {
 // an optimization, since the data passed back to the callback needs to be
 // transformed anyway.
 function makeStatsCallback(cb) {
-  if (cb === undefined) {
-    return rethrow();
-  }
-
   if (typeof cb !== 'function') {
     throw new errors.TypeError('ERR_INVALID_CALLBACK');
   }
@@ -191,8 +157,6 @@ fs.access = function(path, mode, callback) {
   if (typeof mode === 'function') {
     callback = mode;
     mode = fs.F_OK;
-  } else if (typeof callback !== 'function') {
-    throw new errors.TypeError('ERR_INVALID_CALLBACK');
   }
 
   path = getPathFromURL(path);
@@ -218,16 +182,8 @@ fs.accessSync = function(path, mode) {
   handleErrorFromBinding(ctx);
 };
 
-// fs.exists never throws even when the arguments are invalid - if there is
-// a callback it would invoke it with false, otherwise it emits a warning
-// (see the comments of rethrow()).
-// This is to bring it inline with fs.existsSync, which never throws.
-// TODO(joyeecheung): deprecate the never-throw-on-invalid-arguments behavior
 fs.exists = function(path, callback) {
-  if (typeof callback !== 'function') {
-    rethrow();
-    return;
-  }
+  maybeCallback(callback);
 
   function suppressedCallback(err) {
     callback(err ? false : true);

diff --git a/test/fixtures/test-fs-readfile-error.js b/test/fixtures/test-fs-readfile-error.js
@@ -19,4 +19,4 @@
 // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
-require('fs').readFile('/');  // throws EISDIR
+require('fs').readFileSync('/');  // throws EISDIR
diff --git a/test/parallel/test-fs-access.js b/test/parallel/test-fs-access.js
@@ -80,7 +80,8 @@ fs.access(readOnlyFile, fs.F_OK | fs.R_OK, common.mustCall((err) => {
   assert.ifError(err);
 }));
 
-fs.access(readOnlyFile, fs.W_OK, common.mustCall((err) => {
+fs.access(readOnlyFile, fs.W_OK, common.mustCall(function(err) {
+  assert.strictEqual(this, undefined);
   if (hasWriteAccessForReadonlyFile) {
     assert.ifError(err);
   } else {

diff --git a/test/parallel/test-fs-append-file.js b/test/parallel/test-fs-append-file.js
@@ -146,12 +146,9 @@ fs.open(filename5, 'a+', function(e, fd) {
   });
 });
 
-// test that a missing callback emits a warning, even if the last argument is a
-// function.
-const filename6 = join(tmpdir.path, 'append6.txt');
-const warn = 'Calling an asynchronous function without callback is deprecated.';
-common.expectWarning('DeprecationWarning', warn);
-fs.appendFile(filename6, console.log);
+assert.throws(
+  () => fs.appendFile(join(tmpdir.path, 'append6.txt'), console.log),
+  { code: 'ERR_INVALID_CALLBACK' });
 
 process.on('exit', function() {
   assert.strictEqual(12, ncallbacks);

diff --git a/test/parallel/test-fs-exists.js b/test/parallel/test-fs-exists.js
@@ -26,10 +26,9 @@ const fs = require('fs');
 const { URL } = require('url');
 const f = __filename;
 
-// Only warnings are emitted when the callback is invalid
-fs.exists(f);
-fs.exists();
-fs.exists(f, {});
+assert.throws(() => fs.exists(f), { code: 'ERR_INVALID_CALLBACK' });
+assert.throws(() => fs.exists(), { code: 'ERR_INVALID_CALLBACK' });
+assert.throws(() => fs.exists(f, {}), { code: 'ERR_INVALID_CALLBACK' });
 
 fs.exists(f, common.mustCall(function(y) {
   assert.strictEqual(y, true);

diff --git a/test/parallel/test-fs-fchmod.js b/test/parallel/test-fs-fchmod.js
@@ -44,7 +44,7 @@ const fs = require('fs');
 
 // Check for mode values range
 const modeUpperBoundaryValue = 0o777;
-fs.fchmod(1, modeUpperBoundaryValue);
+fs.fchmod(1, modeUpperBoundaryValue, common.mustCall());
 fs.fchmodSync(1, modeUpperBoundaryValue);
 
 // umask of 0o777 is equal to 775

diff --git a/test/parallel/test-fs-make-callback.js b/test/parallel/test-fs-make-callback.js
@@ -4,7 +4,6 @@ const fs = require('fs');
 const callbackThrowValues = [null, true, false, 0, 1, 'foo', /foo/, [], {}];
 
 const { sep } = require('path');
-const warn = 'Calling an asynchronous function without callback is deprecated.';
 
 const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();
@@ -16,11 +15,6 @@ function testMakeCallback(cb) {
   };
 }
 
-common.expectWarning('DeprecationWarning', warn);
-
-// Passing undefined/nothing calls rethrow() internally, which emits a warning
-testMakeCallback()();
-
 function invalidCallbackThrowsTests() {
   callbackThrowValues.forEach((value) => {
     common.expectsError(testMakeCallback(value), {

diff --git a/test/parallel/test-fs-makeStatsCallback.js b/test/parallel/test-fs-makeStatsCallback.js
@@ -2,7 +2,6 @@
 const common = require('../common');
 const fs = require('fs');
 const callbackThrowValues = [null, true, false, 0, 1, 'foo', /foo/, [], {}];
-const warn = 'Calling an asynchronous function without callback is deprecated.';
 
 function testMakeStatsCallback(cb) {
   return function() {
@@ -11,14 +10,9 @@ function testMakeStatsCallback(cb) {
   };
 }
 
-common.expectWarning('DeprecationWarning', warn);
-
 // Verify the case where a callback function is provided
 testMakeStatsCallback(common.mustCall())();
 
-// Passing undefined/nothing calls rethrow() internally, which emits a warning
-testMakeStatsCallback()();
-
 function invalidCallbackThrowsTests() {
   callbackThrowValues.forEach((value) => {
     common.expectsError(testMakeStatsCallback(value), {

diff --git a/test/parallel/test-fs-mkdtemp.js b/test/parallel/test-fs-mkdtemp.js
@@ -29,8 +29,3 @@ fs.mkdtemp(path.join(tmpdir.path, 'bar.'), common.mustCall(handler));
 // Same test as above, but making sure that passing an options object doesn't
 // affect the way the callback function is handled.
 fs.mkdtemp(path.join(tmpdir.path, 'bar.'), {}, common.mustCall(handler));
-
-// Making sure that not passing a callback doesn't crash, as a default function
-// is passed internally.
-fs.mkdtemp(path.join(tmpdir.path, 'bar-'));
-fs.mkdtemp(path.join(tmpdir.path, 'bar-'), {});
diff --git a/test/parallel/test-fs-readfile-error.js b/test/parallel/test-fs-readfile-error.js
@@ -48,7 +48,7 @@ function test(env, cb) {
 
 test({ NODE_DEBUG: '' }, common.mustCall((data) => {
   assert(/EISDIR/.test(data));
-  assert(!/test-fs-readfile-error/.test(data));
+  assert(/test-fs-readfile-error/.test(data));
 }));
 
 test({ NODE_DEBUG: 'fs' }, common.mustCall((data) => {
@@ -57,7 +57,7 @@ test({ NODE_DEBUG: 'fs' }, common.mustCall((data) => {
 }));
 
 common.expectsError(
-  () => { fs.readFile(() => {}); },
+  () => { fs.readFile(() => {}, common.mustNotCall()); },
   {
     code: 'ERR_INVALID_ARG_TYPE',
     message: 'The "path" argument must be one of type string, Buffer, or URL',

diff --git a/test/parallel/test-fs-write-no-fd.js b/test/parallel/test-fs-write-no-fd.js
@@ -1,12 +1,12 @@
 'use strict';
-require('../common');
+const common = require('../common');
 const fs = require('fs');
 const assert = require('assert');
 
 assert.throws(function() {
-  fs.write(null, Buffer.allocUnsafe(1), 0, 1);
+  fs.write(null, Buffer.allocUnsafe(1), 0, 1, common.mustNotCall());
 }, /TypeError/);
 
 assert.throws(function() {
-  fs.write(null, '1', 0, 1);
+  fs.write(null, '1', 0, 1, common.mustNotCall());
 }, /TypeError/);