tools: enable additional eslint rules

Enable additional rules that node either already adheres to
or it makes sense to do so going forward: for-direction,
accessor-pairs, no-lonely-if and symbol-description.

Fix all instances of no-lonely-if in lib & test and disable
accessor-pairs in test-util-inspect.

PR-URL: https://github.com/nodejs/node/pull/16243
Refs: https://eslint.org/docs/rules/for-direction
Refs: https://eslint.org/docs/rules/accessor-pairs
Refs: https://eslint.org/docs/rules/no-lonely-if
Refs: https://eslint.org/docs/rules/symbol-description
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

.eslintrc.yaml

@@ -18,6 +18,7 @@ overrides:
 rules:
   # Possible Errors
   # http://eslint.org/docs/rules/#possible-errors
+  for-direction: error
   no-control-regex: error
   no-debugger: error
   no-dupe-args: error
@@ -41,6 +42,7 @@ rules:
 
   # Best Practices
   # http://eslint.org/docs/rules/#best-practices
+  accessor-pairs: error
   dot-location: [error, property]
   eqeqeq: [error, smart]
   no-fallthrough: error
@@ -122,6 +124,7 @@ rules:
                 ignoreUrls: true,
                 tabWidth: 2}]
   new-parens: error
+  no-lonely-if: error
   no-mixed-spaces-and-tabs: error
   no-multiple-empty-lines: [error, {max: 2, maxEOF: 0, maxBOF: 0}]
   no-restricted-syntax: [error, {
@@ -172,6 +175,7 @@ rules:
   no-this-before-super: error
   prefer-const: [error, {ignoreReadBeforeAssign: true}]
   rest-spread-spacing: error
+  symbol-description: error
   template-curly-spacing: error
 
   # Custom rules in tools/eslint-rules

doc/.eslintrc.yaml

@@ -6,6 +6,7 @@ rules:
   no-undef: off
   no-unused-vars: off
   strict: off
+  symbol-description: off
 
   # add new ECMAScript features gradually
   no-var: error

lib/_http_incoming.js

@@ -301,10 +301,9 @@ function _addHeaderLine(field, value, dest) {
     } else {
       dest['set-cookie'] = [value];
     }
-  } else {
+  } else if (dest[field] === undefined) {
     // Drop duplicates
-    if (dest[field] === undefined)
+    dest[field] = value;
-      dest[field] = value;
   }
 }
 

lib/_http_outgoing.js

@@ -404,20 +404,18 @@ function _storeHeader(firstLine, headers) {
       this.chunkedEncoding = false;
     } else if (!this.useChunkedEncodingByDefault) {
       this._last = true;
+    } else if (!state.trailer &&
+               !this._removedContLen &&
+               typeof this._contentLength === 'number') {
+      state.header += 'Content-Length: ' + this._contentLength + CRLF;
+    } else if (!this._removedTE) {
+      state.header += 'Transfer-Encoding: chunked\r\n';
+      this.chunkedEncoding = true;
     } else {
-      if (!state.trailer &&
+      // We should only be able to get here if both Content-Length and
-          !this._removedContLen &&
+      // Transfer-Encoding are removed by the user.
-          typeof this._contentLength === 'number') {
+      // See: test/parallel/test-http-remove-header-stays-removed.js
-        state.header += 'Content-Length: ' + this._contentLength + CRLF;
+      debug('Both Content-Length and Transfer-Encoding are removed');
-      } else if (!this._removedTE) {
-        state.header += 'Transfer-Encoding: chunked\r\n';
-        this.chunkedEncoding = true;
-      } else {
-        // We should only be able to get here if both Content-Length and
-        // Transfer-Encoding are removed by the user.
-        // See: test/parallel/test-http-remove-header-stays-removed.js
-        debug('Both Content-Length and Transfer-Encoding are removed');
-      }
     }
   }
 

lib/_http_server.js

@@ -433,8 +433,8 @@ function socketOnEnd(server, socket, parser, state) {
     state.outgoing[state.outgoing.length - 1]._last = true;
   } else if (socket._httpMessage) {
     socket._httpMessage._last = true;
-  } else {
+  } else if (socket.writable) {
-    if (socket.writable) socket.end();
+    socket.end();
   }
 }
 
@@ -602,13 +602,11 @@ function parserOnIncoming(server, socket, state, req, keepAlive) {
         res.writeContinue();
         server.emit('request', req, res);
       }
+    } else if (server.listenerCount('checkExpectation') > 0) {
+      server.emit('checkExpectation', req, res);
     } else {
-      if (server.listenerCount('checkExpectation') > 0) {
+      res.writeHead(417);
-        server.emit('checkExpectation', req, res);
+      res.end();
-      } else {
-        res.writeHead(417);
-        res.end();
-      }
     }
   } else {
     server.emit('request', req, res);

lib/_stream_readable.js

@@ -40,20 +40,19 @@ const kProxyEvents = ['error', 'close', 'destroy', 'pause', 'resume'];
 function prependListener(emitter, event, fn) {
   // Sadly this is not cacheable as some libraries bundle their own
   // event emitter implementation with them.
-  if (typeof emitter.prependListener === 'function') {
+  if (typeof emitter.prependListener === 'function')
     return emitter.prependListener(event, fn);
-  } else {
+
-    // This is a hack to make sure that our error handler is attached before any
+  // This is a hack to make sure that our error handler is attached before any
-    // userland ones.  NEVER DO THIS. This is here only because this code needs
+  // userland ones.  NEVER DO THIS. This is here only because this code needs
-    // to continue to work with older versions of Node.js that do not include
+  // to continue to work with older versions of Node.js that do not include
-    // the prependListener() method. The goal is to eventually remove this hack.
+  // the prependListener() method. The goal is to eventually remove this hack.
-    if (!emitter._events || !emitter._events[event])
+  if (!emitter._events || !emitter._events[event])
-      emitter.on(event, fn);
+    emitter.on(event, fn);
-    else if (Array.isArray(emitter._events[event]))
+  else if (Array.isArray(emitter._events[event]))
-      emitter._events[event].unshift(fn);
+    emitter._events[event].unshift(fn);
-    else
+  else
-      emitter._events[event] = [fn, emitter._events[event]];
+    emitter._events[event] = [fn, emitter._events[event]];
-  }
 }
 
 function ReadableState(options, stream) {

lib/_tls_legacy.js

@@ -878,8 +878,8 @@ SecurePair.prototype.error = function(returnOnly) {
              /peer did not return a certificate/.test(err.message)) {
     // Not really an error.
     this.destroy();
-  } else {
+  } else if (!returnOnly) {
-    if (!returnOnly) this.cleartext.emit('error', err);
+    this.cleartext.emit('error', err);
   }
   return err;
 };

lib/child_process.js

@@ -324,11 +324,10 @@ exports.execFile = function(file /*, args, options, callback*/) {
       if (stdoutLen > options.maxBuffer) {
         ex = new Error('stdout maxBuffer exceeded');
         kill();
+      } else if (encoding) {
+        _stdout += chunk;
       } else {
-        if (encoding)
+        _stdout.push(chunk);
-          _stdout += chunk;
-        else
-          _stdout.push(chunk);
       }
     });
   }
@@ -343,11 +342,10 @@ exports.execFile = function(file /*, args, options, callback*/) {
       if (stderrLen > options.maxBuffer) {
         ex = new Error('stderr maxBuffer exceeded');
         kill();
+      } else if (encoding) {
+        _stderr += chunk;
       } else {
-        if (encoding)
+        _stderr.push(chunk);
-          _stderr += chunk;
-        else
-          _stderr.push(chunk);
       }
     });
   }

lib/events.js

@@ -280,13 +280,11 @@ function _addListener(target, type, listener, prepend) {
       // Adding the second element, need to change to array.
       existing = events[type] =
         prepend ? [listener, existing] : [existing, listener];
-    } else {
       // If we've already got an array, just append.
-      if (prepend) {
+    } else if (prepend) {
-        existing.unshift(listener);
+      existing.unshift(listener);
-      } else {
+    } else {
-        existing.push(listener);
+      existing.push(listener);
-      }
     }
 
     // Check for listener leak

lib/fs.js

@@ -1262,21 +1262,19 @@ function writeAll(fd, isUserFd, buffer, offset, length, position, callback) {
           callback(writeErr);
         });
       }
-    } else {
+    } else if (written === length) {
-      if (written === length) {
+      if (isUserFd) {
-        if (isUserFd) {
+        callback(null);
-          callback(null);
-        } else {
-          fs.close(fd, callback);
-        }
       } else {
-        offset += written;
+        fs.close(fd, callback);
-        length -= written;
-        if (position !== null) {
-          position += written;
-        }
-        writeAll(fd, isUserFd, buffer, offset, length, position, callback);
       }
+    } else {
+      offset += written;
+      length -= written;
+      if (position !== null) {
+        position += written;
+      }
+      writeAll(fd, isUserFd, buffer, offset, length, position, callback);
     }
   });
 }

lib/internal/http2/core.js

@@ -187,12 +187,11 @@ function onSessionHeaders(id, cat, flags, headers) {
       }
     } else if (cat === NGHTTP2_HCAT_PUSH_RESPONSE) {
       event = 'push';
-    } else {  // cat === NGHTTP2_HCAT_HEADERS:
+      // cat === NGHTTP2_HCAT_HEADERS:
-      if (!endOfStream && status !== undefined && status >= 200) {
+    } else if (!endOfStream && status !== undefined && status >= 200) {
-        event = 'response';
+      event = 'response';
-      } else {
+    } else {
-        event = endOfStream ? 'trailers' : 'headers';
+      event = endOfStream ? 'trailers' : 'headers';
-      }
     }
     debug(`[${sessionName(owner[kType])}] emitting stream '${event}' event`);
     process.nextTick(emit, stream, event, obj, flags, headers);

lib/path.js

@@ -455,17 +455,15 @@ const win32 = {
       } else {
         return '';
       }
+    } else if (isAbsolute) {
+      if (tail.length > 0)
+        return device + '\\' + tail;
+      else
+        return device + '\\';
+    } else if (tail.length > 0) {
+      return device + tail;
     } else {
-      if (isAbsolute) {
+      return device;
-        if (tail.length > 0)
-          return device + '\\' + tail;
-        else
-          return device + '\\';
-      } else if (tail.length > 0) {
-        return device + tail;
-      } else {
-        return device;
-      }
     }
   },
 

lib/querystring.js

@@ -315,9 +315,8 @@ function parse(qs, sep, eq, options) {
             sepIdx = eqIdx = 0;
             continue;
           }
-        } else {
+        } else if (lastPos < end) {
-          if (lastPos < end)
+          value += qs.slice(lastPos, end);
-            value += qs.slice(lastPos, end);
         }
 
         if (key.length > 0 && keyEncoded)

lib/url.js

@@ -128,16 +128,14 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) {
       if (isWs)
         continue;
       lastPos = start = i;
-    } else {
+    } else if (inWs) {
-      if (inWs) {
+      if (!isWs) {
-        if (!isWs) {
+        end = -1;
-          end = -1;
+        inWs = false;
-          inWs = false;
-        }
-      } else if (isWs) {
-        end = i;
-        inWs = true;
       }
+    } else if (isWs) {
+      end = i;
+      inWs = true;
     }
 
     // Only convert backslashes while we haven't seen a split character

test/.eslintrc.yaml

@@ -5,6 +5,7 @@ rules:
   # http://eslint.org/docs/rules/#ecmascript-6
   no-var: error
   prefer-const: error
+  symbol-description: off
 
   # Custom rules in tools/eslint-rules
   prefer-assert-iferror: error

test/abort/test-abort-uncaught-exception.js

@@ -24,11 +24,10 @@ function run(flags, signals) {
         assert.strictEqual(code, 0xC0000005);
       else
         assert.strictEqual(code, 1);
+    } else if (signals) {
+      assert(signals.includes(sig), `Unexpected signal ${sig}`);
     } else {
-      if (signals)
+      assert.strictEqual(sig, null);
-        assert(signals.includes(sig), `Unexpected signal ${sig}`);
-      else
-        assert.strictEqual(sig, null);
     }
   }));
 }

test/parallel/test-cluster-disconnect-unshared-tcp.js

@@ -37,10 +37,8 @@ if (cluster.isMaster) {
     unbound.disconnect();
     unbound.on('disconnect', cluster.disconnect);
   }
-} else {
+} else if (process.env.BOUND === 'y') {
-  if (process.env.BOUND === 'y') {
+  const source = net.createServer();
-    const source = net.createServer();
 
-    source.listen(0);
+  source.listen(0);
-  }
 }

test/parallel/test-cluster-disconnect-unshared-udp.js

@@ -40,10 +40,8 @@ if (cluster.isMaster) {
     unbound.disconnect();
     unbound.on('disconnect', cluster.disconnect);
   }
-} else {
+} else if (process.env.BOUND === 'y') {
-  if (process.env.BOUND === 'y') {
+  const source = dgram.createSocket('udp4');
-    const source = dgram.createSocket('udp4');
 
-    source.bind(0);
+  source.bind(0);
-  }
 }

test/parallel/test-cluster-worker-destroy.js

@@ -41,17 +41,15 @@ if (cluster.isMaster) {
     worker.on('disconnect', common.mustCall());
     worker.on('exit', common.mustCall());
   });
-} else {
+} else if (cluster.worker.id === 1) {
-  if (cluster.worker.id === 1) {
+  // Call destroy when worker is disconnected
-    // Call destroy when worker is disconnected
+  cluster.worker.process.on('disconnect', function() {
-    cluster.worker.process.on('disconnect', function() {
-      cluster.worker.destroy();
-    });
-
-    const w = cluster.worker.disconnect();
-    assert.strictEqual(w, cluster.worker);
-  } else {
-    // Call destroy when worker is not disconnected yet
     cluster.worker.destroy();
-  }
+  });
+
+  const w = cluster.worker.disconnect();
+  assert.strictEqual(w, cluster.worker);
+} else {
+  // Call destroy when worker is not disconnected yet
+  cluster.worker.destroy();
 }

test/parallel/test-tls-server-verify.js

@@ -335,21 +335,19 @@ function runTest(port, testIndex) {
     port = server.address().port;
     if (tcase.debug) {
       console.error(`${prefix}TLS server running on port ${port}`);
+    } else if (tcase.renegotiate) {
+      runNextClient(0);
     } else {
-      if (tcase.renegotiate) {
+      let clientsCompleted = 0;
-        runNextClient(0);
+      for (let i = 0; i < tcase.clients.length; i++) {
-      } else {
+        runClient(`${prefix}${i} `, port, tcase.clients[i], function() {
-        let clientsCompleted = 0;
+          clientsCompleted++;
-        for (let i = 0; i < tcase.clients.length; i++) {
+          if (clientsCompleted === tcase.clients.length) {
-          runClient(`${prefix}${i} `, port, tcase.clients[i], function() {
+            server.close();
-            clientsCompleted++;
+            successfulTests++;
-            if (clientsCompleted === tcase.clients.length) {
+            runTest(port, nextTest++);
-              server.close();
+          }
-              successfulTests++;
+        });
-              runTest(port, nextTest++);
-            }
-          });
-        }
       }
     }
   });

test/parallel/test-util-inspect.js

@@ -26,6 +26,8 @@ const JSStream = process.binding('js_stream').JSStream;
 const util = require('util');
 const vm = require('vm');
 
+/* eslint-disable accessor-pairs */
+
 assert.strictEqual(util.inspect(1), '1');
 assert.strictEqual(util.inspect(false), 'false');
 assert.strictEqual(util.inspect(''), "''");
@@ -1149,3 +1151,4 @@ if (typeof Symbol !== 'undefined') {
 }
 
 assert.doesNotThrow(() => util.inspect(process));
+/* eslint-enable accessor-pairs */

tools/eslint-rules/rules-utils.js

@@ -43,10 +43,8 @@ module.exports.inSkipBlock = function(node) {
         }
         return false;
       });
-    } else {
+    } else if (hasSkip(consequent.expression)) {
-      if (hasSkip(consequent.expression)) {
+      hasSkipBlock = true;
-        hasSkipBlock = true;
-      }
     }
   }
   return hasSkipBlock;