lib,test: improve faulty assert usage detection

This improves our custom eslint rules to detect assertions to detect
assertions with only a single argument and fixes false negatives in
case unary expressions are used.
Some rules were extended to also lint our docs and tools and the lib
rule was simplified to prohibit most assertion calls.

PR-URL: https://github.com/nodejs/node/pull/26569
Refs: https://github.com/nodejs/node/pull/26565
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

5 files changed, 51 insertions, 60 deletions

diff --git a/.eslintrc.js b/.eslintrc.js
index 38344c9e5b5..d80507467bc 100644
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -170,32 +170,32 @@ module.exports = {
         message: '__defineSetter__ is deprecated.',
       },
     ],
-    // If this list is modified, please copy the change to lib/.eslintrc.yaml
-    // and test/.eslintrc.yaml.
+    // If this list is modified, please copy changes that should apply to ./lib
+    // as well to lib/.eslintrc.yaml.
     'no-restricted-syntax': [
       'error',
       {
-        selector: "CallExpression[callee.object.name='assert'][callee.property.name='deepStrictEqual'][arguments.2.type='Literal']",
+        selector: "CallExpression[callee.property.name='deepStrictEqual'][arguments.2.type='Literal']",
         message: 'Do not use a literal for the third argument of assert.deepStrictEqual()',
       },
       {
-        selector: "CallExpression[callee.object.name='assert'][callee.property.name='doesNotThrow']",
+        selector: "CallExpression[callee.property.name='doesNotThrow']",
         message: 'Please replace `assert.doesNotThrow()` and add a comment next to the code instead.',
       },
       {
-        selector: "CallExpression[callee.object.name='assert'][callee.property.name='rejects'][arguments.length<2]",
+        selector: "CallExpression[callee.property.name='rejects'][arguments.length<2]",
         message: '`assert.rejects()` must be invoked with at least two arguments.',
       },
       {
-        selector: "CallExpression[callee.object.name='assert'][callee.property.name='strictEqual'][arguments.2.type='Literal']",
+        selector: "CallExpression[callee.property.name='strictEqual'][arguments.2.type='Literal']",
         message: 'Do not use a literal for the third argument of assert.strictEqual()',
       },
       {
-        selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.1.type='Literal']:not([arguments.1.regex])",
+        selector: "CallExpression[callee.property.name='throws'][arguments.1.type='Literal']:not([arguments.1.regex])",
         message: 'Use an object as second argument of `assert.throws()`.',
       },
       {
-        selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.length<2]",
+        selector: "CallExpression[callee.property.name='throws'][arguments.length<2]",
         message: '`assert.throws()` must be invoked with at least two arguments.',
       },
       {
@@ -210,6 +210,38 @@ module.exports = {
         selector: 'ThrowStatement > CallExpression[callee.name=/Error$/]',
         message: 'Use `new` keyword when throwing an `Error`.',
       },
+      {
+        selector: "CallExpression[callee.property.name='notDeepStrictEqual'][arguments.length<2]",
+        message: 'assert.notDeepStrictEqual() must be invoked with at least two arguments.',
+      },
+      {
+        selector: "CallExpression[callee.property.name='deepStrictEqual'][arguments.length<2]",
+        message: 'assert.deepStrictEqual() must be invoked with at least two arguments.',
+      },
+      {
+        selector: "CallExpression[callee.property.name='notStrictEqual'][arguments.length<2]",
+        message: 'assert.notStrictEqual() must be invoked with at least two arguments.',
+      },
+      {
+        selector: "CallExpression[callee.property.name='strictEqual'][arguments.length<2]",
+        message: 'assert.strictEqual() must be invoked with at least two arguments.',
+      },
+      {
+        selector: "CallExpression[callee.property.name='notDeepStrictEqual'][arguments.0.type='Literal']:not([arguments.1.type='Literal']):not([arguments.1.type='ObjectExpression']):not([arguments.1.type='ArrayExpression']):not([arguments.1.type='UnaryExpression'])",
+        message: 'The first argument should be the `actual`, not the `expected` value.',
+      },
+      {
+        selector: "CallExpression[callee.property.name='notStrictEqual'][arguments.0.type='Literal']:not([arguments.1.type='Literal']):not([arguments.1.type='ObjectExpression']):not([arguments.1.type='ArrayExpression']):not([arguments.1.type='UnaryExpression'])",
+        message: 'The first argument should be the `actual`, not the `expected` value.',
+      },
+      {
+        selector: "CallExpression[callee.property.name='deepStrictEqual'][arguments.0.type='Literal']:not([arguments.1.type='Literal']):not([arguments.1.type='ObjectExpression']):not([arguments.1.type='ArrayExpression']):not([arguments.1.type='UnaryExpression'])",
+        message: 'The first argument should be the `actual`, not the `expected` value.',
+      },
+      {
+        selector: "CallExpression[callee.property.name='strictEqual'][arguments.0.type='Literal']:not([arguments.1.type='Literal']):not([arguments.1.type='ObjectExpression']):not([arguments.1.type='ArrayExpression']):not([arguments.1.type='UnaryExpression'])",
+        message: 'The first argument should be the `actual`, not the `expected` value.',
+      }
     ],
     /* eslint-enable max-len */
     'no-return-await': 'error',
diff --git a/lib/.eslintrc.yaml b/lib/.eslintrc.yaml
index 181fb9cead2..fcb9ea7feba 100644
--- a/lib/.eslintrc.yaml
+++ b/lib/.eslintrc.yaml
@@ -4,18 +4,8 @@ rules:
   no-restricted-syntax:
     # Config copied from .eslintrc.js
     - error
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='deepStrictEqual'][arguments.2.type='Literal']"
-      message: "Do not use a literal for the third argument of assert.deepStrictEqual()"
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='doesNotThrow']"
-      message: "Please replace `assert.doesNotThrow()` and add a comment next to the code instead."
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='rejects'][arguments.length<2]"
-      message: "assert.rejects() must be invoked with at least two arguments."
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='strictEqual'][arguments.2.type='Literal']"
-      message: "Do not use a literal for the third argument of assert.strictEqual()"
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.1.type='Literal']:not([arguments.1.regex])"
-      message: "Use an object as second argument of assert.throws()"
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.length<2]"
-      message: "assert.throws() must be invoked with at least two arguments."
+    - selector: "CallExpression[callee.object.name='assert']:not([callee.property.name='ok']):not([callee.property.name='fail']):not([callee.property.name='ifError'])"
+      message: "Please only use simple assertions in ./lib"
     - selector: "CallExpression[callee.name='setTimeout'][arguments.length<2]"
       message: "setTimeout() must be invoked with at least two arguments."
     - selector: "CallExpression[callee.name='setInterval'][arguments.length<2]"
diff --git a/lib/internal/js_stream_socket.js b/lib/internal/js_stream_socket.js
index 8343b6c2645..038aa56c428 100644
--- a/lib/internal/js_stream_socket.js
+++ b/lib/internal/js_stream_socket.js
@@ -1,6 +1,6 @@
 'use strict';
 
-const assert = require('assert');
+const assert = require('internal/assert');
 const util = require('util');
 const { Socket } = require('net');
 const { JSStream } = internalBinding('js_stream');
@@ -122,8 +122,8 @@ class JSStreamSocket extends Socket {
       this[kPendingShutdownRequest] = req;
       return 0;
     }
-    assert.strictEqual(this[kCurrentWriteRequest], null);
-    assert.strictEqual(this[kCurrentShutdownRequest], null);
+    assert(this[kCurrentWriteRequest] === null);
+    assert(this[kCurrentShutdownRequest] === null);
     this[kCurrentShutdownRequest] = req;
 
     const handle = this._handle;
@@ -148,8 +148,8 @@ class JSStreamSocket extends Socket {
   }
 
   doWrite(req, bufs) {
-    assert.strictEqual(this[kCurrentWriteRequest], null);
-    assert.strictEqual(this[kCurrentShutdownRequest], null);
+    assert(this[kCurrentWriteRequest] === null);
+    assert(this[kCurrentShutdownRequest] === null);
 
     const handle = this._handle;
     const self = this;
@@ -214,7 +214,7 @@ class JSStreamSocket extends Socket {
 
     setImmediate(() => {
       // Should be already set by net.js
-      assert.strictEqual(this._handle, null);
+      assert(this._handle === null);
 
       this.finishWrite(handle, uv.UV_ECANCELED);
       this.finishShutdown(handle, uv.UV_ECANCELED);
diff --git a/test/.eslintrc.yaml b/test/.eslintrc.yaml
index 12be376357a..1140cff966a 100644
--- a/test/.eslintrc.yaml
+++ b/test/.eslintrc.yaml
@@ -20,36 +20,6 @@ rules:
   node-core/required-modules: [error, common]
   node-core/no-duplicate-requires: off
 
-  no-restricted-syntax:
-    # Config copied from .eslintrc.js
-    - error
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='deepStrictEqual'][arguments.2.type='Literal']"
-      message: "Do not use a literal for the third argument of assert.deepStrictEqual()"
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='doesNotThrow']"
-      message: "Please replace `assert.doesNotThrow()` and add a comment next to the code instead."
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='rejects'][arguments.length<2]"
-      message: "assert.rejects() must be invoked with at least two arguments."
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='strictEqual'][arguments.2.type='Literal']"
-      message: "Do not use a literal for the third argument of assert.strictEqual()"
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.1.type='Literal']:not([arguments.1.regex])"
-      message: "Use an object as second argument of assert.throws()"
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='throws'][arguments.length<2]"
-      message: "assert.throws() must be invoked with at least two arguments."
-    - selector: "CallExpression[callee.name='setTimeout'][arguments.length<2]"
-      message: "setTimeout() must be invoked with at least two arguments."
-    - selector: "CallExpression[callee.name='setInterval'][arguments.length<2]"
-      message: "setInterval() must be invoked with at least 2 arguments."
-    - selector: "ThrowStatement > CallExpression[callee.name=/Error$/]"
-      message: "Use new keyword when throwing an Error."
-    # Specific to /test
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='notDeepStrictEqual'][arguments.0.type='Literal']:not([arguments.1.type='Literal']):not([arguments.1.type='ObjectExpression']):not([arguments.1.type='ArrayExpression'])"
-      message: "The first argument should be the `actual`, not the `expected` value."
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='notStrictEqual'][arguments.0.type='Literal']:not([arguments.1.type='Literal']):not([arguments.1.type='ObjectExpression']):not([arguments.1.type='ArrayExpression'])"
-      message: "The first argument should be the `actual`, not the `expected` value."
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='deepStrictEqual'][arguments.0.type='Literal']:not([arguments.1.type='Literal']):not([arguments.1.type='ObjectExpression']):not([arguments.1.type='ArrayExpression'])"
-      message: "The first argument should be the `actual`, not the `expected` value."
-    - selector: "CallExpression[callee.object.name='assert'][callee.property.name='strictEqual'][arguments.0.type='Literal']:not([arguments.1.type='Literal']):not([arguments.1.type='ObjectExpression']):not([arguments.1.type='ArrayExpression'])"
-      message: "The first argument should be the `actual`, not the `expected` value."
 # Global scoped methods and vars
 globals:
   WebAssembly: false
diff --git a/test/parallel/test-assert.js b/test/parallel/test-assert.js
index a7c37e8fb33..83b97cfec3e 100644
--- a/test/parallel/test-assert.js
+++ b/test/parallel/test-assert.js
@@ -65,6 +65,7 @@ assert.throws(() => a.notEqual(true, true),
 assert.throws(() => a.strictEqual(2, '2'),
               a.AssertionError, 'strictEqual(2, \'2\')');
 
+/* eslint-disable no-restricted-syntax */
 assert.throws(() => a.strictEqual(null, undefined),
               a.AssertionError, 'strictEqual(null, undefined)');
 
@@ -95,11 +96,9 @@ function thrower(errorConstructor) {
 // The basic calls work.
 assert.throws(() => thrower(a.AssertionError), a.AssertionError, 'message');
 assert.throws(() => thrower(a.AssertionError), a.AssertionError);
-// eslint-disable-next-line no-restricted-syntax
 assert.throws(() => thrower(a.AssertionError));
 
 // If not passing an error, catch all.
-// eslint-disable-next-line no-restricted-syntax
 assert.throws(() => thrower(TypeError));
 
 // When passing a type, only catch errors of the appropriate type.
@@ -309,7 +308,7 @@ try {
 }
 
 try {
-  assert.strictEqual(1, 2, 'oh no'); // eslint-disable-line no-restricted-syntax
+  assert.strictEqual(1, 2, 'oh no');
 } catch (e) {
   assert.strictEqual(e.message, 'oh no');
   // Message should not be marked as generated.
@@ -833,7 +832,6 @@ common.expectsError(
 );
 
 common.expectsError(
-  // eslint-disable-next-line no-restricted-syntax
   () => assert.throws(() => {}, 'Error message', 'message'),
   {
     code: 'ERR_INVALID_ARG_TYPE',
@@ -1010,6 +1008,7 @@ assert.throws(
              'The error message "foo" is identical to the message.'
   }
 );
+/* eslint-enable no-restricted-syntax */
 
 // Should not throw.
 // eslint-disable-next-line no-restricted-syntax, no-throw-literal