assert: detect faulty throws usage

One of the biggest downsides to the `assert.throws` API is that it
does not check for the error message in case that is used as second
argument. It will instead be used in case no error is thrown.

This improves the situation by checking the actual error message
against the provided one and throws an error in case they are
identical. It is very unlikely that the user wants to use that error
message as information instead of checking against that message.

PR-URL: https://github.com/nodejs/node/pull/19867
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

doc/api/errors.md

@@ -586,6 +586,14 @@ found [here][online].
 <a id="nodejs-error-codes"></a>
 ## Node.js Error Codes
 
+<a id="ERR_AMBIGUOUS_ARGUMENT"></a>
+### ERR_AMBIGUOUS_ARGUMENT
+
+This is triggered by the `assert` module in case e.g.,
+`assert.throws(fn, message)` is used in a way that the message is the thrown
+error message. This is ambiguous because the message is not verifying the error
+message and will only be thrown in case no error is thrown.
+
 <a id="ERR_ARG_NOT_ITERABLE"></a>
 ### ERR_ARG_NOT_ITERABLE
 

lib/assert.js

@@ -29,6 +29,7 @@ const {
   AssertionError,
   errorCache,
   codes: {
+    ERR_AMBIGUOUS_ARGUMENT,
     ERR_INVALID_ARG_TYPE
   }
 } = require('internal/errors');
@@ -470,6 +471,19 @@ function expectsError(stackStartFn, actual, error, message) {
                                      ['Object', 'Error', 'Function', 'RegExp'],
                                      error);
     }
+    if (typeof actual === 'object' && actual !== null) {
+      if (actual.message === error) {
+        throw new ERR_AMBIGUOUS_ARGUMENT(
+          'error/message',
+          `The error message "${actual.message}" is identical to the message.`
+        );
+      }
+    } else if (actual === error) {
+      throw new ERR_AMBIGUOUS_ARGUMENT(
+        'error/message',
+        `The error "${actual}" is identical to the message.`
+      );
+    }
     message = error;
     error = null;
   }

lib/internal/errors.js

@@ -640,6 +640,7 @@ module.exports = exports = {
 //
 // Note: Node.js specific errors must begin with the prefix ERR_
 
+E('ERR_AMBIGUOUS_ARGUMENT', 'The "%s" argument is ambiguous. %s', TypeError);
 E('ERR_ARG_NOT_ITERABLE', '%s must be iterable', TypeError);
 E('ERR_ASSERTION', '%s', Error);
 E('ERR_ASYNC_CALLBACK', '%s must be a function', TypeError);

test/parallel/test-assert.js

@@ -836,3 +836,32 @@ common.expectsError(
     }
   );
 }
+
+assert.throws(
+  () => a.throws(
+    // eslint-disable-next-line no-throw-literal
+    () => { throw 'foo'; },
+    'foo'
+  ),
+  {
+    code: 'ERR_AMBIGUOUS_ARGUMENT',
+    message: 'The "error/message" argument is ambiguous. ' +
+             'The error "foo" is identical to the message.'
+  }
+);
+
+assert.throws(
+  () => a.throws(
+    () => { throw new TypeError('foo'); },
+    'foo'
+  ),
+  {
+    code: 'ERR_AMBIGUOUS_ARGUMENT',
+    message: 'The "error/message" argument is ambiguous. ' +
+             'The error message "foo" is identical to the message.'
+  }
+);
+
+// Should not throw.
+// eslint-disable-next-line no-restricted-syntax, no-throw-literal
+assert.throws(() => { throw null; }, 'foo');