feat(eslint-plugin): [no-unnecessary-condition] remove ignoreRHS option

The 'ignoreRHS' option was an unnecessary part of this lint rule; it forced the user to choose between false-negatives (e.g. if(expr && true)) or false positives `return expr && <ReactComponent>`.

The new logic only checks the right hand side of a logical operator if the logical expression is in a conditional context: e.g. it's inside an if block, or it's the left hand side of another logical expression.

packages/eslint-plugin/src/rules/no-unnecessary-condition.ts

@@ -60,7 +60,6 @@ const isLiteral = (type: ts.Type): boolean =>
 export type Options = [
   {
     allowConstantLoopConditions?: boolean;
-    ignoreRhs?: boolean;
     checkArrayPredicates?: boolean;
   },
 ];
@@ -93,9 +92,6 @@ export default createRule<Options, MessageId>({
           allowConstantLoopConditions: {
             type: 'boolean',
           },
-          ignoreRhs: {
-            type: 'boolean',
-          },
           checkArrayPredicates: {
             type: 'boolean',
           },
@@ -124,14 +120,10 @@ export default createRule<Options, MessageId>({
   defaultOptions: [
     {
       allowConstantLoopConditions: false,
-      ignoreRhs: false,
       checkArrayPredicates: false,
     },
   ],
-  create(
-    context,
-    [{ allowConstantLoopConditions, checkArrayPredicates, ignoreRhs }],
-  ) {
+  create(context, [{ allowConstantLoopConditions, checkArrayPredicates }]) {
     const service = getParserServices(context);
     const checker = service.program.getTypeChecker();
     const sourceCode = context.getSourceCode();
@@ -151,6 +143,11 @@ export default createRule<Options, MessageId>({
      * if the type of the node is always true or always false, it's not necessary.
      */
     function checkNode(node: TSESTree.Expression): void {
+      // When checking logical expressions, only check the right side
+      //  as the left side has been checked by checkLogicalExpressionForUnnecessaryConditionals
+      if (node.type === AST_NODE_TYPES.LogicalExpression) {
+        return checkNode(node.right);
+      }
       const type = getNodeType(node);
 
       // Conditional is always necessary if it involves:
@@ -228,20 +225,6 @@ export default createRule<Options, MessageId>({
       }
     }
 
-    /**
-     * Checks that a testable expression is necessarily conditional, reports otherwise.
-     * Filters all LogicalExpressions to prevent some duplicate reports.
-     */
-    function checkIfTestExpressionIsNecessaryConditional(
-      node: TSESTree.ConditionalExpression | TSESTree.IfStatement,
-    ): void {
-      if (node.test.type === AST_NODE_TYPES.LogicalExpression) {
-        return;
-      }
-
-      checkNode(node.test);
-    }
-
     /**
      * Checks that a logical expression contains a boolean, reports otherwise.
      */
@@ -252,10 +235,9 @@ export default createRule<Options, MessageId>({
         checkNodeForNullish(node.left);
         return;
       }
+      // Only checks the left side, since the right side might not be "conditional" at all.
+      // The right side will be checked if the LogicalExpression is used in a conditional context
       checkNode(node.left);
-      if (!ignoreRhs) {
-        checkNode(node.right);
-      }
     }
 
     /**
@@ -267,10 +249,8 @@ export default createRule<Options, MessageId>({
         | TSESTree.ForStatement
         | TSESTree.WhileStatement,
     ): void {
-      if (
-        node.test === null ||
-        node.test.type === AST_NODE_TYPES.LogicalExpression
-      ) {
+      if (node.test === null) {
+        // e.g. `for(;;)`
         return;
       }
 
@@ -415,10 +395,10 @@ export default createRule<Options, MessageId>({
     return {
       BinaryExpression: checkIfBinaryExpressionIsNecessaryConditional,
       CallExpression: checkCallExpression,
-      ConditionalExpression: checkIfTestExpressionIsNecessaryConditional,
+      ConditionalExpression: (node): void => checkNode(node.test),
       DoWhileStatement: checkIfLoopIsNecessaryConditional,
       ForStatement: checkIfLoopIsNecessaryConditional,
-      IfStatement: checkIfTestExpressionIsNecessaryConditional,
+      IfStatement: (node): void => checkNode(node.test),
       LogicalExpression: checkLogicalExpressionForUnnecessaryConditionals,
       WhileStatement: checkIfLoopIsNecessaryConditional,
       OptionalMemberExpression: checkOptionalMemberExpression,

packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts

@@ -1,11 +1,11 @@
-import {
-  TestCaseError,
-  InvalidTestCase,
-} from '@typescript-eslint/experimental-utils/dist/ts-eslint';
 import rule, {
   Options,
   MessageId,
 } from '../../src/rules/no-unnecessary-condition';
+import {
+  TestCaseError,
+  InvalidTestCase,
+} from '@typescript-eslint/experimental-utils/dist/ts-eslint';
 import { RuleTester, getFixturesRootDir } from '../RuleTester';
 
 const rootPath = getFixturesRootDir();
@@ -52,7 +52,8 @@ const t2 = b1 || b2;
 if(b1 && b2) {}
 while(b1 && b2) {}
 for (let i = 0; (b1 && b2); i++) { break; }
-const t1 = (b1 && b2) ? 'yes' : 'no'`,
+const t1 = (b1 && b2) ? 'yes' : 'no'
+for(;;)`,
     necessaryConditionTest('false | 5'), // Truthy literal and falsy literal
     necessaryConditionTest('boolean | "foo"'), // boolean and truthy literal
     necessaryConditionTest('0 | boolean'), // boolean and falsy literal
@@ -150,13 +151,13 @@ function test(a: string | null | undefined) {
 function test(a: unknown) {
   return a ?? "default";
 }`,
-    // Supports ignoring the RHS
+    // Doesn't check the right-hand side of a logical expression
+    //  in a non-conditional context
     {
       code: `
 declare const b1: boolean;
 declare const b2: true;
-if(b1 && b2) {}`,
-      options: [{ ignoreRhs: true }],
+const x = b1 && b2;`,
     },
     {
       code: `
@@ -232,16 +233,22 @@ declare const b2: boolean;
 const t1 = b1 && b2;
 const t2 = b1 || b2;
 if(b1 && b2) {}
+if(b2 && b1) {}
 while(b1 && b2) {}
+while(b2 && b1) {}
 for (let i = 0; (b1 && b2); i++) { break; }
-const t1 = (b1 && b2) ? 'yes' : 'no'`,
+const t1 = (b1 && b2) ? 'yes' : 'no';
+const t1 = (b2 && b1) ? 'yes' : 'no'`,
       errors: [
         ruleError(4, 12, 'alwaysTruthy'),
         ruleError(5, 12, 'alwaysTruthy'),
         ruleError(6, 4, 'alwaysTruthy'),
-        ruleError(7, 7, 'alwaysTruthy'),
-        ruleError(8, 18, 'alwaysTruthy'),
+        ruleError(7, 10, 'alwaysTruthy'),
+        ruleError(8, 7, 'alwaysTruthy'),
         ruleError(9, 13, 'alwaysTruthy'),
+        ruleError(10, 18, 'alwaysTruthy'),
+        ruleError(11, 13, 'alwaysTruthy'),
+        ruleError(12, 19, 'alwaysTruthy'),
       ],
     },
     // Ensure that it's complaining about the right things
@@ -254,6 +261,22 @@ const t1 = (b1 && b2) ? 'yes' : 'no'`,
     unnecessaryConditionTest('void', 'alwaysFalsy'),
     unnecessaryConditionTest('never', 'never'),
 
+    // More complex logical expressions
+    {
+      code: `
+declare const b1: boolean;
+declare const b2: boolean;
+if(true && b1 && b2) {}
+if(b1 && false && b2) {}
+if(b1 || b2 || true) {}
+`,
+      errors: [
+        ruleError(4, 4, 'alwaysTruthy'),
+        ruleError(5, 10, 'alwaysFalsy'),
+        ruleError(6, 16, 'alwaysTruthy'),
+      ],
+    },
+
     // Generic type params
     {
       code: `
@@ -390,28 +413,6 @@ function falsy() {}
     // `,
     //       errors: [ruleError(6, 23, 'alwaysTruthyFunc')],
     //     },
-
-    // Still errors on in the expected locations when ignoring RHS
-    {
-      options: [{ ignoreRhs: true }],
-      code: `
-const b1 = true;
-const b2 = false;
-const t1 = b1 && b2;
-const t2 = b1 || b2;
-if(b1 && b2) {}
-while(b1 && b2) {}
-for (let i = 0; (b1 && b2); i++) { break; }
-const t1 = (b1 && b2) ? 'yes' : 'no'`,
-      errors: [
-        ruleError(4, 12, 'alwaysTruthy'),
-        ruleError(5, 12, 'alwaysTruthy'),
-        ruleError(6, 4, 'alwaysTruthy'),
-        ruleError(7, 7, 'alwaysTruthy'),
-        ruleError(8, 18, 'alwaysTruthy'),
-        ruleError(9, 13, 'alwaysTruthy'),
-      ],
-    },
     {
       code: `
 while(true) {}