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: #26569 Refs: #26565 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Richard Lau <[email protected]>
nodejs · Mar 14, 2019 · 1fa5004 · 1fa5004
1 parent 1a0004d
commit 1fa5004
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 60 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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