Skip to content

Commit

Permalink
lib,test: improve faulty assert usage detection
Browse files Browse the repository at this point in the history
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 <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
  • Loading branch information
BridgeAR committed Mar 13, 2019
1 parent 5f38797 commit 32853c0
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 60 deletions.
48 changes: 40 additions & 8 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.',
},
{
Expand All @@ -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',
Expand Down
14 changes: 2 additions & 12 deletions lib/.eslintrc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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]"
Expand Down
12 changes: 6 additions & 6 deletions lib/internal/js_stream_socket.js
Original file line number Diff line number Diff line change
@@ -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');
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
30 changes: 0 additions & 30 deletions test/.eslintrc.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 3 additions & 4 deletions test/parallel/test-assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)');

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 32853c0

Please sign in to comment.