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:
@@ -230,16 +227,13 @@ 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;
+      if (node.test !== null) {
+        checkNode(node.test);
       }
-
-      checkNode(node.test);
     }
 
     /**
@@ -252,10 +246,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 +260,7 @@ 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) {
         return;
       }
 

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

@@ -1,8 +1,5 @@
 import path from 'path';
-import rule, {
-  Options,
-  MessageId,
-} from '../../src/rules/no-unnecessary-condition';
+import rule, { MessageId } from '../../src/rules/no-unnecessary-condition';
 import { RuleTester } from '../RuleTester';
 import {
   TestCaseError,
@@ -38,7 +35,7 @@ const t1 = b1 && b2;
 const unnecessaryConditionTest = (
   condition: string,
   messageId: MessageId,
-): InvalidTestCase<MessageId, Options> => ({
+): InvalidTestCase<MessageId, [{}]> => ({
   code: necessaryConditionTest(condition),
   errors: [ruleError(4, 12, messageId)],
 });
@@ -151,13 +148,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: `
@@ -233,16 +230,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
@@ -255,6 +258,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: `
@@ -391,28 +410,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) {}