fix(eslint-plugin): [strict-boolean-expressions] check all conditions…

… in a logical operator chain (#5539)

* fix(eslint-plugin): [strict-bool-expr] fix logical chain

* make code more logical hopefully

* more tests

packages/eslint-plugin/src/rules/strict-boolean-expressions.ts

@@ -166,16 +166,16 @@ export default util.createRule<Options, MessageId>({
       });
     }
 
-    const checkedNodes = new Set<TSESTree.Node>();
+    const traversedNodes = new Set<TSESTree.Node>();
 
     return {
-      ConditionalExpression: checkTestExpression,
-      DoWhileStatement: checkTestExpression,
-      ForStatement: checkTestExpression,
-      IfStatement: checkTestExpression,
-      WhileStatement: checkTestExpression,
-      'LogicalExpression[operator!="??"]': checkNode,
-      'UnaryExpression[operator="!"]': checkUnaryLogicalExpression,
+      ConditionalExpression: traverseTestExpression,
+      DoWhileStatement: traverseTestExpression,
+      ForStatement: traverseTestExpression,
+      IfStatement: traverseTestExpression,
+      WhileStatement: traverseTestExpression,
+      'LogicalExpression[operator!="??"]': traverseLogicalExpression,
+      'UnaryExpression[operator="!"]': traverseUnaryLogicalExpression,
     };
 
     type TestExpression =
@@ -185,45 +185,81 @@ export default util.createRule<Options, MessageId>({
       | TSESTree.IfStatement
       | TSESTree.WhileStatement;
 
-    function checkTestExpression(node: TestExpression): void {
+    /**
+     * Inspects condition of a test expression. (`if`, `while`, `for`, etc.)
+     */
+    function traverseTestExpression(node: TestExpression): void {
       if (node.test == null) {
         return;
       }
-      checkNode(node.test, true);
+      traverseNode(node.test, true);
+    }
+
+    /**
+     * Inspects the argument of a unary logical expression (`!`).
+     */
+    function traverseUnaryLogicalExpression(
+      node: TSESTree.UnaryExpression,
+    ): void {
+      traverseNode(node.argument, true);
     }
 
-    function checkUnaryLogicalExpression(node: TSESTree.UnaryExpression): void {
-      checkNode(node.argument, true);
+    /**
+     * Inspects the arguments of a logical expression (`&&`, `||`).
+     *
+     * If the logical expression is a descendant of a test expression,
+     * the `isCondition` flag should be set to true.
+     * Otherwise, if the logical expression is there on it's own,
+     * it's used for control flow and is not a condition itself.
+     */
+    function traverseLogicalExpression(
+      node: TSESTree.LogicalExpression,
+      isCondition = false,
+    ): void {
+      // left argument is always treated as a condition
+      traverseNode(node.left, true);
+      // if the logical expression is used for control flow,
+      // then it's right argument is used for it's side effects only
+      traverseNode(node.right, isCondition);
     }
 
     /**
-     * This function analyzes the type of a node and checks if it is allowed in a boolean context.
-     * It can recurse when checking nested logical operators, so that only the outermost operands are reported.
-     * The right operand of a logical expression is ignored unless it's a part of a test expression (if/while/ternary/etc).
-     * @param node The AST node to check.
-     * @param isTestExpr Whether the node is a descendant of a test expression.
+     * Inspects any node.
+     *
+     * If it's a logical expression then it recursively traverses its arguments.
+     * If it's any other kind of node then it's type is finally checked against the rule,
+     * unless `isCondition` flag is set to false, in which case
+     * it's assumed to be used for side effects only and is skipped.
      */
-    function checkNode(node: TSESTree.Node, isTestExpr = false): void {
+    function traverseNode(node: TSESTree.Node, isCondition: boolean): void {
       // prevent checking the same node multiple times
-      if (checkedNodes.has(node)) {
+      if (traversedNodes.has(node)) {
         return;
       }
-      checkedNodes.add(node);
+      traversedNodes.add(node);
 
       // for logical operator, we check its operands
       if (
         node.type === AST_NODE_TYPES.LogicalExpression &&
         node.operator !== '??'
       ) {
-        checkNode(node.left, isTestExpr);
+        traverseLogicalExpression(node, isCondition);
+        return;
+      }
 
-        // we ignore the right operand when not in a context of a test expression
-        if (isTestExpr) {
-          checkNode(node.right, isTestExpr);
-        }
+      // skip if node is not a condition
+      if (!isCondition) {
         return;
       }
 
+      checkNode(node);
+    }
+
+    /**
+     * This function does the actual type check on a node.
+     * It analyzes the type of a node and checks if it is allowed in a boolean context.
+     */
+    function checkNode(node: TSESTree.Node): void {
       const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node);
       const type = util.getConstrainedTypeAtLocation(typeChecker, tsNode);
       const types = inspectVariantTypes(tsutils.unionTypeParts(type));

packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts

@@ -117,6 +117,20 @@ ruleTester.run('strict-boolean-expressions', rule, {
         <T extends any>(x: T) => x ? 1 : 0;
       `,
     }),
+
+    // logical operator
+    ...batchedSingleLineTests<Options>({
+      options: [{ allowString: true, allowNumber: true }],
+      code: `
+        1 && true && 'x' && {};
+        let x = 0 || false || '' || null;
+        if (1 && true && 'x') void 0;
+        if (0 || false || '') void 0;
+        1 && true && 'x' ? {} : null;
+        0 || false || '' ? null : {};
+      `,
+    }),
+
     {
       code: `
 declare const x: string[] | null;
@@ -304,6 +318,89 @@ if (y) {
       ],
     },
 
+    // shouldn't check last logical operand when used for control flow
+    {
+      options: [{ allowString: false, allowNumber: false }],
+      code: "'asd' && 123 && [] && null;",
+      errors: [
+        { messageId: 'conditionErrorString', line: 1, column: 1 },
+        { messageId: 'conditionErrorNumber', line: 1, column: 10 },
+        { messageId: 'conditionErrorObject', line: 1, column: 17 },
+      ],
+    },
+    {
+      options: [{ allowString: false, allowNumber: false }],
+      code: "'asd' || 123 || [] || null;",
+      errors: [
+        { messageId: 'conditionErrorString', line: 1, column: 1 },
+        { messageId: 'conditionErrorNumber', line: 1, column: 10 },
+        { messageId: 'conditionErrorObject', line: 1, column: 17 },
+      ],
+    },
+    {
+      options: [{ allowString: false, allowNumber: false }],
+      code: "let x = (1 && 'a' && null) || 0 || '' || {};",
+      errors: [
+        { messageId: 'conditionErrorNumber', line: 1, column: 10 },
+        { messageId: 'conditionErrorString', line: 1, column: 15 },
+        { messageId: 'conditionErrorNullish', line: 1, column: 22 },
+        { messageId: 'conditionErrorNumber', line: 1, column: 31 },
+        { messageId: 'conditionErrorString', line: 1, column: 36 },
+      ],
+    },
+    {
+      options: [{ allowString: false, allowNumber: false }],
+      code: "return (1 || 'a' || null) && 0 && '' && {};",
+      errors: [
+        { messageId: 'conditionErrorNumber', line: 1, column: 9 },
+        { messageId: 'conditionErrorString', line: 1, column: 14 },
+        { messageId: 'conditionErrorNullish', line: 1, column: 21 },
+        { messageId: 'conditionErrorNumber', line: 1, column: 30 },
+        { messageId: 'conditionErrorString', line: 1, column: 35 },
+      ],
+    },
+    {
+      options: [{ allowString: false, allowNumber: false }],
+      code: "console.log((1 && []) || ('a' && {}));",
+      errors: [
+        { messageId: 'conditionErrorNumber', line: 1, column: 14 },
+        { messageId: 'conditionErrorObject', line: 1, column: 19 },
+        { messageId: 'conditionErrorString', line: 1, column: 27 },
+      ],
+    },
+
+    // should check all logical operands when used in a condition
+    {
+      options: [{ allowString: false, allowNumber: false }],
+      code: "if ((1 && []) || ('a' && {})) void 0;",
+      errors: [
+        { messageId: 'conditionErrorNumber', line: 1, column: 6 },
+        { messageId: 'conditionErrorObject', line: 1, column: 11 },
+        { messageId: 'conditionErrorString', line: 1, column: 19 },
+        { messageId: 'conditionErrorObject', line: 1, column: 26 },
+      ],
+    },
+    {
+      options: [{ allowString: false, allowNumber: false }],
+      code: "let x = null || 0 || 'a' || [] ? {} : undefined;",
+      errors: [
+        { messageId: 'conditionErrorNullish', line: 1, column: 9 },
+        { messageId: 'conditionErrorNumber', line: 1, column: 17 },
+        { messageId: 'conditionErrorString', line: 1, column: 22 },
+        { messageId: 'conditionErrorObject', line: 1, column: 29 },
+      ],
+    },
+    {
+      options: [{ allowString: false, allowNumber: false }],
+      code: "return !(null || 0 || 'a' || []);",
+      errors: [
+        { messageId: 'conditionErrorNullish', line: 1, column: 10 },
+        { messageId: 'conditionErrorNumber', line: 1, column: 18 },
+        { messageId: 'conditionErrorString', line: 1, column: 23 },
+        { messageId: 'conditionErrorObject', line: 1, column: 30 },
+      ],
+    },
+
     // nullish in boolean context
     ...batchedSingleLineTests<MessageId, Options>({
       code: noFormat`