errors: improve ERR_INVALID_ARG_TYPE

ERR_INVALID_ARG_TYPE is the most common error used throughout the
code base. This improves the error message by providing more details
to the user and by indicating more precisely which values are allowed
ones and which ones are not.

It adds the actual input to the error message in case it's a primitive.
If it's a class instance, it'll print the class name instead of
"object" and "falsy" or similar entries are not named "type" anymore.

PR-URL: #29675
Reviewed-By: Rich Trott <rtrott@gmail.com>

Author: BridgeAR

Diff for: lib/internal/errors.js

@@ -23,6 +23,21 @@ const {
 const messages = new Map();
 const codes = {};
 
+const classRegExp = /^([A-Z][a-z0-9]*)+$/;
+// Sorted by a rough estimate on most frequently used entries.
+const kTypes = [
+  'string',
+  'function',
+  'number',
+  'object',
+  // Accept 'Function' and 'Object' as alternative to the lower cased version.
+  'Function',
+  'Object',
+  'boolean',
+  'bigint',
+  'symbol'
+];
+
 const { kMaxLength } = internalBinding('buffer');
 
 const MainContextError = Error;
@@ -610,26 +625,6 @@ function isStackOverflowError(err) {
          err.message === maxStack_ErrorMessage;
 }
 
-function oneOf(expected, thing) {
-  assert(typeof thing === 'string', '`thing` has to be of type string');
-  if (ArrayIsArray(expected)) {
-    const len = expected.length;
-    assert(len > 0,
-           'At least one expected value needs to be specified');
-    expected = expected.map((i) => String(i));
-    if (len > 2) {
-      return `one of ${thing} ${expected.slice(0, len - 1).join(', ')}, or ` +
-             expected[len - 1];
-    } else if (len === 2) {
-      return `one of ${thing} ${expected[0]} or ${expected[1]}`;
-    } else {
-      return `of ${thing} ${expected[0]}`;
-    }
-  } else {
-    return `of ${thing} ${String(expected)}`;
-  }
-}
-
 // Only use this for integers! Decimal numbers do not work with this function.
 function addNumericalSeparator(val) {
   let res = '';
@@ -926,27 +921,114 @@ E('ERR_INVALID_ADDRESS_FAMILY', function(addressType, host, port) {
 E('ERR_INVALID_ARG_TYPE',
   (name, expected, actual) => {
     assert(typeof name === 'string', "'name' must be a string");
+    if (!ArrayIsArray(expected)) {
+      expected = [expected];
+    }
+
+    let msg = 'The ';
+    if (name.endsWith(' argument')) {
+      // For cases like 'first argument'
+      msg += `${name} `;
+    } else {
+      const type = name.includes('.') ? 'property' : 'argument';
+      msg += `"${name}" ${type} `;
+    }
 
     // determiner: 'must be' or 'must not be'
-    let determiner;
     if (typeof expected === 'string' && expected.startsWith('not ')) {
-      determiner = 'must not be';
+      msg += 'must not be ';
       expected = expected.replace(/^not /, '');
     } else {
-      determiner = 'must be';
+      msg += 'must be ';
     }
 
-    let msg;
-    if (name.endsWith(' argument')) {
-      // For cases like 'first argument'
-      msg = `The ${name} ${determiner} ${oneOf(expected, 'type')}`;
-    } else {
-      const type = name.includes('.') ? 'property' : 'argument';
-      msg = `The "${name}" ${type} ${determiner} ${oneOf(expected, 'type')}`;
+    const types = [];
+    const instances = [];
+    const other = [];
+
+    for (const value of expected) {
+      assert(typeof value === 'string',
+             'All expected entries have to be of type string');
+      if (kTypes.includes(value)) {
+        types.push(value.toLowerCase());
+      } else if (classRegExp.test(value)) {
+        instances.push(value);
+      } else {
+        assert(value !== 'object',
+               'The value "object" should be written as "Object"');
+        other.push(value);
+      }
     }
 
-    // TODO(BridgeAR): Improve the output by showing `null` and similar.
-    msg += `. Received type ${typeof actual}`;
+    // Special handle `object` in case other instances are allowed to outline
+    // the differences between each other.
+    if (instances.length > 0) {
+      const pos = types.indexOf('object');
+      if (pos !== -1) {
+        types.splice(pos, 1);
+        instances.push('Object');
+      }
+    }
+
+    if (types.length > 0) {
+      if (types.length > 2) {
+        const last = types.pop();
+        msg += `one of type ${types.join(', ')}, or ${last}`;
+      } else if (types.length === 2) {
+        msg += `one of type ${types[0]} or ${types[1]}`;
+      } else {
+        msg += `of type ${types[0]}`;
+      }
+      if (instances.length > 0 || other.length > 0)
+        msg += ' or ';
+    }
+
+    if (instances.length > 0) {
+      if (instances.length > 2) {
+        const last = instances.pop();
+        msg += `an instance of ${instances.join(', ')}, or ${last}`;
+      } else {
+        msg += `an instance of ${instances[0]}`;
+        if (instances.length === 2) {
+          msg += ` or ${instances[1]}`;
+        }
+      }
+      if (other.length > 0)
+        msg += ' or ';
+    }
+
+    if (other.length > 0) {
+      if (other.length > 2) {
+        const last = other.pop();
+        msg += `one of ${other.join(', ')}, or ${last}`;
+      } else if (other.length === 2) {
+        msg += `one of ${other[0]} or ${other[1]}`;
+      } else {
+        if (other[0].toLowerCase() !== other[0])
+          msg += 'an ';
+        msg += `${other[0]}`;
+      }
+    }
+
+    if (actual == null) {
+      msg += `. Received ${actual}`;
+    } else if (typeof actual === 'function' && actual.name) {
+      msg += `. Received function ${actual.name}`;
+    } else if (typeof actual === 'object') {
+      if (actual.constructor && actual.constructor.name) {
+        msg += `. Received an instance of ${actual.constructor.name}`;
+      } else {
+        const inspected = lazyInternalUtilInspect()
+          .inspect(actual, { depth: -1 });
+        msg += `. Received ${inspected}`;
+      }
+    } else {
+      let inspected = lazyInternalUtilInspect()
+        .inspect(actual, { colors: false });
+      if (inspected.length > 25)
+        inspected = `${inspected.slice(0, 25)}...`;
+      msg += `. Received type ${typeof actual} (${inspected})`;
+    }
     return msg;
   }, TypeError);
 E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => {
@@ -1034,7 +1116,15 @@ E('ERR_INVALID_URL', function(input) {
   return `Invalid URL: ${input}`;
 }, TypeError);
 E('ERR_INVALID_URL_SCHEME',
-  (expected) => `The URL must be ${oneOf(expected, 'scheme')}`, TypeError);
+  (expected) => {
+    if (typeof expected === 'string')
+      expected = [expected];
+    assert(expected.length <= 2);
+    const res = expected.length === 2 ?
+      `one of scheme ${expected[0]} or ${expected[1]}` :
+      `of scheme ${expected[0]}`;
+    return `The URL must be ${res}`;
+  }, TypeError);
 E('ERR_IPC_CHANNEL_CLOSED', 'Channel closed', Error);
 E('ERR_IPC_DISCONNECTED', 'IPC channel is already disconnected', Error);
 E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe', Error);

Diff for: test/common/index.js

@@ -718,6 +718,26 @@ function runWithInvalidFD(func) {
   printSkipMessage('Could not generate an invalid fd');
 }
 
+// A helper function to simplify checking for ERR_INVALID_ARG_TYPE output.
+function invalidArgTypeHelper(input) {
+  if (input == null) {
+    return ` Received ${input}`;
+  }
+  if (typeof input === 'function' && input.name) {
+    return ` Received function ${input.name}`;
+  }
+  if (typeof input === 'object') {
+    if (input.constructor && input.constructor.name) {
+      return ` Received an instance of ${input.constructor.name}`;
+    }
+    return ` Received ${util.inspect(input, { depth: -1 })}`;
+  }
+  let inspected = util.inspect(input, { colors: false });
+  if (inspected.length > 25)
+    inspected = `${inspected.slice(0, 25)}...`;
+  return ` Received type ${typeof input} (${inspected})`;
+}
+
 module.exports = {
   allowGlobals,
   buildType,
@@ -735,6 +755,7 @@ module.exports = {
   hasIntl,
   hasCrypto,
   hasMultiLocalhost,
+  invalidArgTypeHelper,
   isAIX,
   isAlive,
   isFreeBSD,

Diff for: test/es-module/test-esm-loader-modulemap.js

@@ -25,7 +25,8 @@ common.expectsError(
   {
     code: 'ERR_INVALID_ARG_TYPE',
     type: TypeError,
-    message: 'The "url" argument must be of type string. Received type number'
+    message: 'The "url" argument must be of type string. Received type number' +
+             ' (1)'
   }
 );
 
@@ -34,7 +35,8 @@ common.expectsError(
   {
     code: 'ERR_INVALID_ARG_TYPE',
     type: TypeError,
-    message: 'The "url" argument must be of type string. Received type number'
+    message: 'The "url" argument must be of type string. Received type number' +
+             ' (1)'
   }
 );
 
@@ -43,8 +45,8 @@ common.expectsError(
   {
     code: 'ERR_INVALID_ARG_TYPE',
     type: TypeError,
-    message: 'The "job" argument must be of type ModuleJob. ' +
-             'Received type string'
+    message: 'The "job" argument must be an instance of ModuleJob. ' +
+             "Received type string ('notamodulejob')"
   }
 );
 
@@ -53,6 +55,7 @@ common.expectsError(
   {
     code: 'ERR_INVALID_ARG_TYPE',
     type: TypeError,
-    message: 'The "url" argument must be of type string. Received type number'
+    message: 'The "url" argument must be of type string. Received type number' +
+             ' (1)'
   }
 );

Diff for: test/internet/test-dns-promises-resolve.js

@@ -26,7 +26,7 @@ const dnsPromises = require('dns').promises;
       code: 'ERR_INVALID_ARG_TYPE',
       type: TypeError,
       message: 'The "rrtype" argument must be of type string. ' +
-               `Received type ${typeof rrtype}`
+               `Received type ${typeof rrtype} (${rrtype})`
     }
   );
 }

Diff for: test/parallel/test-assert-async.js

@@ -125,8 +125,8 @@ promises.push(assert.rejects(
   assert.rejects('fail', {}),
   {
     code: 'ERR_INVALID_ARG_TYPE',
-    message: 'The "promiseFn" argument must be one of type ' +
-             'Function or Promise. Received type string'
+    message: 'The "promiseFn" argument must be of type function or an ' +
+             "instance of Promise. Received type string ('fail')"
   }
 ));
 
@@ -225,8 +225,8 @@ promises.push(assert.rejects(
     assert.doesNotReject(123),
     {
       code: 'ERR_INVALID_ARG_TYPE',
-      message: 'The "promiseFn" argument must be one of type ' +
-               'Function or Promise. Received type number'
+      message: 'The "promiseFn" argument must be of type ' +
+               'function or an instance of Promise. Received type number (123)'
     }
   ));
   /* eslint-enable no-restricted-syntax */

Diff for: test/parallel/test-assert.js

@@ -414,8 +414,8 @@ assert.throws(
       {
         code: 'ERR_INVALID_ARG_TYPE',
         type: TypeError,
-        message: 'The "fn" argument must be of type Function. Received ' +
-                 `type ${typeof fn}`
+        message: 'The "fn" argument must be of type function.' +
+                 common.invalidArgTypeHelper(fn)
       }
     );
   };
@@ -484,8 +484,8 @@ assert.throws(() => {
       {
         code: 'ERR_INVALID_ARG_TYPE',
         name: 'TypeError',
-        message: 'The "options" argument must be of type Object. ' +
-                 `Received type ${typeof input}`
+        message: 'The "options" argument must be of type object.' +
+                 common.invalidArgTypeHelper(input)
       });
   });
 }
@@ -931,8 +931,9 @@ common.expectsError(
   {
     code: 'ERR_INVALID_ARG_TYPE',
     type: TypeError,
-    message: 'The "error" argument must be one of type Object, Error, ' +
-             'Function, or RegExp. Received type string'
+    message: 'The "error" argument must be of type function or ' +
+             'an instance of Error, RegExp, or Object. Received type string ' +
+             "('Error message')"
   }
 );
 
@@ -945,8 +946,9 @@ common.expectsError(
     () => assert.throws(() => {}, input),
     {
       code: 'ERR_INVALID_ARG_TYPE',
-      message: 'The "error" argument must be one of type Object, Error, ' +
-               `Function, or RegExp. Received type ${typeof input}`
+      message: 'The "error" argument must be of type function or ' +
+               'an instance of Error, RegExp, or Object.' +
+               common.invalidArgTypeHelper(input)
     }
   );
 });
@@ -1024,8 +1026,8 @@ common.expectsError(
     {
       type: TypeError,
       code: 'ERR_INVALID_ARG_TYPE',
-      message: 'The "expected" argument must be one of type Function or ' +
-               'RegExp. Received type object'
+      message: 'The "expected" argument must be of type function or an ' +
+               'instance of RegExp. Received an instance of Object'
     }
   );
 

Diff for: test/parallel/test-buffer-alloc.js

@@ -967,19 +967,19 @@ common.expectsError(
   {
     code: 'ERR_INVALID_ARG_TYPE',
     type: TypeError,
-    message: 'The "target" argument must be one of type Buffer or Uint8Array.' +
-             ' Received type undefined'
+    message: 'The "target" argument must be an instance of Buffer or ' +
+             'Uint8Array. Received undefined'
   });
 
 assert.throws(() => Buffer.from(), {
   name: 'TypeError',
-  message: 'The first argument must be one of type string, Buffer, ' +
-  'ArrayBuffer, Array, or Array-like Object. Received type undefined'
+  message: 'The first argument must be of type string or an instance of ' +
+  'Buffer, ArrayBuffer, or Array or an Array-like Object. Received undefined'
 });
 assert.throws(() => Buffer.from(null), {
   name: 'TypeError',
-  message: 'The first argument must be one of type string, Buffer, ' +
-  'ArrayBuffer, Array, or Array-like Object. Received type object'
+  message: 'The first argument must be of type string or an instance of ' +
+  'Buffer, ArrayBuffer, or Array or an Array-like Object. Received null'
 });
 
 // Test prototype getters don't throw

Diff for: test/parallel/test-buffer-arraybuffer.js

@@ -43,8 +43,9 @@ assert.throws(function() {
 }, {
   code: 'ERR_INVALID_ARG_TYPE',
   name: 'TypeError',
-  message: 'The first argument must be one of type string, Buffer,' +
-  ' ArrayBuffer, Array, or Array-like Object. Received type object'
+  message: 'The first argument must be of type string or an instance of ' +
+           'Buffer, ArrayBuffer, or Array or an Array-like Object. Received ' +
+           'an instance of AB'
 });
 
 // Test the byteOffset and length arguments