tools,lib: forbid native Error constructors

This adds a rule that forbids the use of native Error constructors in the `lib` directory. This is to encourage use of the `internal/errors` mechanism. The rule is disabled for errors that are not created with the `internal/errors` module but are still assigned an error code. PR-URL: #19373 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · Mar 21, 2018 · 6a9f049 · 6a9f049
1 parent ab8bf26
commit 6a9f049
Show file tree

Hide file tree

Showing 16 changed files with 39 additions and 0 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -148,6 +148,7 @@ module.exports = {
       }
     ],
     /* eslint-disable max-len, quotes */
+    // If this list is modified, please copy the change to lib/.eslintrc.yaml
     'no-restricted-syntax': [
       'error',
       {

diff --git a/lib/.eslintrc.yaml b/lib/.eslintrc.yaml
@@ -1,4 +1,22 @@
 rules:
+  no-restricted-syntax:
+    # Config copied from .eslintrc.js
+    - error
+    - 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='throws'][arguments.1.type='Literal']:not([arguments.1.regex])"
+      message: "use a regular expression for 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."
+    # Config specific to lib
+    - selector: "NewExpression[callee.name=/Error$/]:not([callee.name=/^(AssertionError|NghttpError)$/])"
+      message: "Use an error exported by the internal/errors module."
   # Custom rules in tools/eslint-rules
   node-core/require-buffer: error
   node-core/buffer-constructor: error

diff --git a/lib/_http_client.js b/lib/_http_client.js
@@ -307,6 +307,7 @@ function emitAbortNT() {
 
 
 function createHangUpError() {
+  // eslint-disable-next-line no-restricted-syntax
   var error = new Error('socket hang up');
   error.code = 'ECONNRESET';
   return error;

diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
@@ -775,6 +775,7 @@ function onSocketClose(err) {
   // Emit ECONNRESET
   if (!this._controlReleased && !this[kErrorEmitted]) {
     this[kErrorEmitted] = true;
+    // eslint-disable-next-line no-restricted-syntax
     const connReset = new Error('socket hang up');
     connReset.code = 'ECONNRESET';
     this._tlsOptions.server.emit('tlsClientError', connReset, this);
@@ -1103,6 +1104,7 @@ function onConnectEnd() {
   if (!this._hadError) {
     const options = this[kConnectOptions];
     this._hadError = true;
+    // eslint-disable-next-line no-restricted-syntax
     const error = new Error('Client network socket disconnected before ' +
                             'secure TLS connection was established');
     error.code = 'ECONNRESET';

diff --git a/lib/assert.js b/lib/assert.js
@@ -216,6 +216,7 @@ function innerOk(fn, argLen, value, message) {
     } else if (message == null) {
       // Use the call as error message if possible.
       // This does not work with e.g. the repl.
+      // eslint-disable-next-line no-restricted-syntax
       const err = new Error();
       // Make sure the limit is set to 1. Otherwise it could fail (<= 0) or it
       // does to much work.

diff --git a/lib/buffer.js b/lib/buffer.js
@@ -1073,6 +1073,7 @@ if (process.binding('config').hasIntl) {
       return result;
 
     const code = icuErrName(result);
+    // eslint-disable-next-line no-restricted-syntax
     const err = new Error(`Unable to transcode Buffer [${code}]`);
     err.code = code;
     err.errno = result;

diff --git a/lib/child_process.js b/lib/child_process.js
@@ -278,6 +278,7 @@ exports.execFile = function(file /*, args, options, callback*/) {
       cmd += ` ${args.join(' ')}`;
 
     if (!ex) {
+      // eslint-disable-next-line no-restricted-syntax
       ex = new Error('Command failed: ' + cmd + '\n' + stderr);
       ex.killed = child.killed || killed;
       ex.code = code < 0 ? getSystemErrorName(code) : code;
@@ -588,6 +589,7 @@ function checkExecSyncError(ret, args, cmd) {
     msg += cmd || args.join(' ');
     if (ret.stderr && ret.stderr.length > 0)
       msg += `\n${ret.stderr.toString()}`;
+    // eslint-disable-next-line no-restricted-syntax
     err = new Error(msg);
   }
   if (err) {

diff --git a/lib/domain.js b/lib/domain.js
@@ -89,6 +89,7 @@ if (process.hasUncaughtExceptionCaptureCallback()) {
 }
 
 // Get the stack trace at the point where `domain` was required.
+// eslint-disable-next-line no-restricted-syntax
 const domainRequireStack = new Error('require(`domain`) at this point').stack;
 
 const { setUncaughtExceptionCaptureCallback } = process;

diff --git a/lib/events.js b/lib/events.js
@@ -240,6 +240,7 @@ function _addListener(target, type, listener, prepend) {
       if (m && m > 0 && existing.length > m) {
         existing.warned = true;
         // No error code for this since it is a Warning
+        // eslint-disable-next-line no-restricted-syntax
         const w = new Error('Possible EventEmitter memory leak detected. ' +
                             `${existing.length} ${String(type)} listeners ` +
                             'added. Use emitter.setMaxListeners() to ' +

diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js
@@ -127,6 +127,7 @@
       // Model the error off the internal/errors.js model, but
       // do not use that module given that it could actually be
       // the one causing the error if there's a bug in Node.js
+      // eslint-disable-next-line no-restricted-syntax
       const err = new Error(`No such built-in module: ${id}`);
       err.code = 'ERR_UNKNOWN_BUILTIN_MODULE';
       err.name = 'Error [ERR_UNKNOWN_BUILTIN_MODULE]';

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -444,6 +444,7 @@ function uvException(ctx) {
 
   // Pass the message to the constructor instead of setting it on the object
   // to make sure it is the same as the one created in C++
+  // eslint-disable-next-line no-restricted-syntax
   const err = new Error(message);
 
   for (const prop of Object.keys(ctx)) {
@@ -482,6 +483,7 @@ function errnoException(err, syscall, original) {
   const message = original ?
     `${syscall} ${code} ${original}` : `${syscall} ${code}`;
 
+  // eslint-disable-next-line no-restricted-syntax
   const ex = new Error(message);
   // TODO(joyeecheung): errno is supposed to err, like in uvException
   ex.code = ex.errno = code;
@@ -517,6 +519,7 @@ function exceptionWithHostPort(err, syscall, address, port, additional) {
     details += ` - Local (${additional})`;
   }
 
+  // eslint-disable-next-line no-restricted-syntax
   const ex = new Error(`${syscall} ${code}${details}`);
   // TODO(joyeecheung): errno is supposed to err, like in uvException
   ex.code = ex.errno = code;
@@ -537,6 +540,7 @@ function exceptionWithHostPort(err, syscall, address, port, additional) {
  * @returns {Error}
  */
 function dnsException(err, syscall, hostname) {
+  // eslint-disable-next-line no-restricted-syntax
   const ex = new Error();
   // FIXME(bnoordhuis) Remove this backwards compatibility nonsense and pass
   // the true error to the user. ENOTFOUND is not even a proper POSIX error!

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -566,6 +566,7 @@ Module._resolveFilename = function(request, parent, isMain, options) {
   // look up the filename first, since that's the cache key.
   var filename = Module._findPath(request, paths, isMain);
   if (!filename) {
+    // eslint-disable-next-line no-restricted-syntax
     var err = new Error(`Cannot find module '${request}'`);
     err.code = 'MODULE_NOT_FOUND';
     throw err;

diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js
@@ -30,6 +30,7 @@ function handledRejection(promise) {
     if (promiseInfo.warned) {
       const { uid } = promiseInfo;
       // Generate the warning object early to get a good stack trace.
+      // eslint-disable-next-line no-restricted-syntax
       const warning = new Error('Promise rejection was handled ' +
                                 `asynchronously (rejection id: ${uid})`);
       warning.name = 'PromiseRejectionHandledWarning';
@@ -53,6 +54,7 @@ function emitWarning(uid, reason) {
     // ignored
   }
 
+  // eslint-disable-next-line no-restricted-syntax
   const warning = new Error(
     'Unhandled promise rejection. This error originated either by ' +
     'throwing inside of an async function without a catch block, ' +

diff --git a/lib/internal/process/warning.js b/lib/internal/process/warning.js
@@ -126,6 +126,7 @@ function setupProcessWarnings() {
     if (type !== undefined && typeof type !== 'string')
       throw new ERR_INVALID_ARG_TYPE('type', 'string');
     if (warning === undefined || typeof warning === 'string') {
+      // eslint-disable-next-line no-restricted-syntax
       warning = new Error(warning);
       warning.name = String(type || 'Warning');
       if (code !== undefined) warning.code = code;

diff --git a/lib/net.js b/lib/net.js
@@ -385,6 +385,7 @@ function writeAfterFIN(chunk, encoding, cb) {
     encoding = null;
   }
 
+  // eslint-disable-next-line no-restricted-syntax
   var er = new Error('This socket has been ended by the other party');
   er.code = 'EPIPE';
   // TODO: defer error events consistently everywhere, not just the cb

diff --git a/lib/zlib.js b/lib/zlib.js
@@ -144,6 +144,7 @@ function zlibOnError(message, errno) {
   _close(self);
   self._hadError = true;
 
+  // eslint-disable-next-line no-restricted-syntax
   const error = new Error(message);
   error.errno = errno;
   error.code = codes[errno];