From 32853c0a136b0616844cdf5c276d2f1eb9a4fc50 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sun, 10 Mar 2019 15:37:51 +0100 Subject: [PATCH] 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 Reviewed-By: Rich Trott Reviewed-By: Richard Lau --- .eslintrc.js | 48 ++++++++++++++++++++++++++------ lib/.eslintrc.yaml | 14 ++-------- lib/internal/js_stream_socket.js | 12 ++++---- test/.eslintrc.yaml | 30 -------------------- test/parallel/test-assert.js | 7 ++--- 5 files changed, 51 insertions(+), 60 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 23b707b55d9062..f8e6e78769cf3f 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 181fb9cead2090..fcb9ea7febac52 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 8343b6c2645842..038aa56c4288c6 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 12be376357a81a..1140cff966a5c4 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 a7c37e8fb33024..83b97cfec3e923 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