Revert "fs: make callback mandatory to all async functions"

This reverts commit 9359de9dd2eae06ed5afcb75dad9bd4c466eb69d.

Original Commit Message:

    The "fs" module has two functions called `maybeCallback` and
    `makeCallback`, as of now.

    The `maybeCallback` creates a default function to report errors, if the
    parameter passed is not a function object. Basically, if the callback
    is omitted in some cases, this function is used to create a default
    callback function.

    The `makeCallback`, OTOH, creates a default function only if the
    parameter passed is `undefined`, and if it is not a function object it
    will throw an `Error`.

    This patch removes the `maybeCallback` function and makes the callback
    function argument mandatory for all the async functions.

    PR-URL: https://github.com/nodejs/node/pull/7168
    Reviewed-By: Trevor Norris <trev.norris@gmail.com>

PR-URL: https://github.com/nodejs/node/pull/7846
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>

9 files changed, 50 insertions, 24 deletions

diff --git a/test/fixtures/test-fs-readfile-error.js b/test/fixtures/test-fs-readfile-error.js
new file mode 100644
index 0000000000..0638d01ac7
--- /dev/null
+++ b/test/fixtures/test-fs-readfile-error.js
@@ -0,0 +1 @@
+require('fs').readFile('/');  // throws EISDIR
diff --git a/test/parallel/test-fs-chmod.js b/test/parallel/test-fs-chmod.js
index 1564734ec6..954916cbdb 100644
--- a/test/parallel/test-fs-chmod.js
+++ b/test/parallel/test-fs-chmod.js
@@ -101,7 +101,7 @@ fs.open(file2, 'a', function(err, fd) {
         assert.equal(mode_sync, fs.fstatSync(fd).mode & 0o777);
       }
       success_count++;
-      fs.closeSync(fd);
+      fs.close(fd);
     }
   });
 });
diff --git a/test/parallel/test-fs-link.js b/test/parallel/test-fs-link.js
index 89d4c25d5b..2cba47bfec 100644
--- a/test/parallel/test-fs-link.js
+++ b/test/parallel/test-fs-link.js
@@ -23,14 +23,14 @@ fs.link(srcPath, dstPath, common.mustCall(callback));
 
 assert.throws(
   function() {
-    fs.link(undefined, undefined, () => {});
+    fs.link();
   },
   /src must be a string or Buffer/
 );
 
 assert.throws(
   function() {
-    fs.link('abc', undefined, () => {});
+    fs.link('abc');
   },
   /dest must be a string or Buffer/
 );
diff --git a/test/parallel/test-fs-make-callback.js b/test/parallel/test-fs-make-callback.js
index 8ded34e2b0..4fbe64437e 100644
--- a/test/parallel/test-fs-make-callback.js
+++ b/test/parallel/test-fs-make-callback.js
@@ -13,6 +13,9 @@ function test(cb) {
 // Verify the case where a callback function is provided
 assert.doesNotThrow(test(function() {}));
 
+// Passing undefined calls rethrow() internally, which is fine
+assert.doesNotThrow(test(undefined));
+
 // Anything else should throw
 assert.throws(test(null));
 assert.throws(test(true));
diff --git a/test/parallel/test-fs-mkdtemp.js b/test/parallel/test-fs-mkdtemp.js
index 60d1196c5a..dd4ab75c22 100644
--- a/test/parallel/test-fs-mkdtemp.js
+++ b/test/parallel/test-fs-mkdtemp.js
@@ -29,3 +29,8 @@ fs.mkdtemp(path.join(common.tmpDir, '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(common.tmpDir, 'bar.'), {}, common.mustCall(handler));
+
+// Making sure that not passing a callback doesn't crash, as a default function
+// is passed internally.
+assert.doesNotThrow(() => fs.mkdtemp(path.join(common.tmpDir, 'bar-')));
+assert.doesNotThrow(() => fs.mkdtemp(path.join(common.tmpDir, 'bar-'), {}));
diff --git a/test/parallel/test-fs-no-callback-errors.js b/test/parallel/test-fs-no-callback-errors.js
deleted file mode 100644
index 0b2bc0d2e3..0000000000
--- a/test/parallel/test-fs-no-callback-errors.js
+++ /dev/null
@@ -1,17 +0,0 @@
-'use strict';
-
-require('../common');
-const fs = require('fs');
-const assert = require('assert');
-
-Object.getOwnPropertyNames(fs).filter(
-  (d) => !d.endsWith('Stream') &&       // ignore stream creation functions
-         !d.endsWith('Sync') &&         // ignore synchronous functions
-         typeof fs[d] === 'function' && // ignore anything other than functions
-         d.indexOf('_') === -1 &&       // ignore internal use functions
-         !/^[A-Z]/.test(d) &&           // ignore conventional constructors
-         d !== 'watch' &&               // watch's callback is optional
-         d !== 'unwatchFile'            // unwatchFile's callback is optional
-).forEach(
-  (d) => assert.throws(() => fs[d](), /"callback" argument must be a function/)
-);
diff --git a/test/parallel/test-fs-readfile-error.js b/test/parallel/test-fs-readfile-error.js
new file mode 100644
index 0000000000..86a2be4e1b
--- /dev/null
+++ b/test/parallel/test-fs-readfile-error.js
@@ -0,0 +1,34 @@
+'use strict';
+var common = require('../common');
+var assert = require('assert');
+var exec = require('child_process').exec;
+var path = require('path');
+
+// `fs.readFile('/')` does not fail on FreeBSD, because you can open and read
+// the directory there.
+if (process.platform === 'freebsd') {
+  common.skip('platform not supported.');
+  return;
+}
+
+function test(env, cb) {
+  var filename = path.join(common.fixturesDir, 'test-fs-readfile-error.js');
+  var execPath = '"' + process.execPath + '" "' + filename + '"';
+  var options = { env: Object.assign(process.env, env) };
+  exec(execPath, options, function(err, stdout, stderr) {
+    assert(err);
+    assert.equal(stdout, '');
+    assert.notEqual(stderr, '');
+    cb('' + stderr);
+  });
+}
+
+test({ NODE_DEBUG: '' }, common.mustCall(function(data) {
+  assert(/EISDIR/.test(data));
+  assert(!/test-fs-readfile-error/.test(data));
+}));
+
+test({ NODE_DEBUG: 'fs' }, common.mustCall(function(data) {
+  assert(/EISDIR/.test(data));
+  assert(/test-fs-readfile-error/.test(data));
+}));
diff --git a/test/parallel/test-fs-stat.js b/test/parallel/test-fs-stat.js
index 081ace714b..ac68a9ae42 100644
--- a/test/parallel/test-fs-stat.js
+++ b/test/parallel/test-fs-stat.js
@@ -28,7 +28,7 @@ fs.open('.', 'r', undefined, common.mustCall(function(err, fd) {
   fs.fstat(fd, common.mustCall(function(err, stats) {
     assert.ifError(err);
     assert.ok(stats.mtime instanceof Date);
-    fs.closeSync(fd);
+    fs.close(fd);
     assert(this === global);
   }));
 
@@ -47,7 +47,7 @@ fs.open('.', 'r', undefined, common.mustCall(function(err, fd) {
     console.dir(stats);
     assert.ok(stats.mtime instanceof Date);
   }
-  fs.closeSync(fd);
+  fs.close(fd);
 }));
 
 console.log(`stating:  ${__filename}`);
diff --git a/test/parallel/test-fs-watchfile.js b/test/parallel/test-fs-watchfile.js
index 270bf270d7..d30261859c 100644
--- a/test/parallel/test-fs-watchfile.js
+++ b/test/parallel/test-fs-watchfile.js
@@ -8,11 +8,11 @@ const assert = require('assert');
 // Basic usage tests.
 assert.throws(function() {
   fs.watchFile('./some-file');
-}, /"callback" argument must be a function/);
+}, /"watchFile\(\)" requires a listener function/);
 
 assert.throws(function() {
   fs.watchFile('./another-file', {}, 'bad listener');
-}, /"callback" argument must be a function/);
+}, /"watchFile\(\)" requires a listener function/);
 
 assert.throws(function() {
   fs.watchFile(new Object(), function() {});