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.
typescript-eslint · Oct 30, 2019 · 5014390 · 5014390
1 parent d4703e1
commit 5014390
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 65 deletions.
diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts
@@ -48,18 +48,12 @@ type ExpressionWithTest =
   | TSESTree.IfStatement
   | TSESTree.WhileStatement;
 
-export type Options = [
-  {
-    ignoreRhs?: boolean;
-  },
-];
-
 export type MessageId =
   | 'alwaysTruthy'
   | 'alwaysFalsy'
   | 'literalBooleanExpression'
   | 'never';
-export default createRule<Options, MessageId>({
+export default createRule<[], MessageId>({
   name: 'no-unnecessary-conditionals',
   meta: {
     type: 'suggestion',
@@ -70,17 +64,7 @@ export default createRule<Options, MessageId>({
       recommended: false,
       requiresTypeChecking: true,
     },
-    schema: [
-      {
-        type: 'object',
-        properties: {
-          ignoreRhs: {
-            type: 'boolean',
-          },
-        },
-        additionalProperties: false,
-      },
-    ],
+    schema: [],
     messages: {
       alwaysTruthy: 'Unnecessary conditional, value is always truthy.',
       alwaysFalsy: 'Unnecessary conditional, value is always falsy.',
@@ -89,12 +73,8 @@ export default createRule<Options, MessageId>({
       never: 'Unnecessary conditional, value is `never`',
     },
   },
-  defaultOptions: [
-    {
-      ignoreRhs: false,
-    },
-  ],
-  create(context, [{ ignoreRhs }]) {
+  defaultOptions: [],
+  create(context) {
     const service = getParserServices(context);
     const checker = service.program.getTypeChecker();
 
@@ -108,6 +88,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.Node): 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 `any` or `unknown`
@@ -154,15 +139,11 @@ 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: ExpressionWithTest,
     ): void {
-      if (
-        node.test !== null &&
-        node.test.type !== AST_NODE_TYPES.LogicalExpression
-      ) {
+      if (node.test !== null) {
         checkNode(node.test);
       }
     }
@@ -173,10 +154,9 @@ export default createRule<Options, MessageId>({
     function checkLogicalExpressionForUnnecessaryConditionals(
       node: TSESTree.LogicalExpression,
     ): void {
+      // 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);
-      }
     }
 
     return {

diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts b/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)],
 });
@@ -79,13 +76,13 @@ function test(a: string) {
   return a === "a"
 }`,
 
-    // 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;`,
     },
   ],
   invalid: [
@@ -97,16 +94,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
@@ -119,6 +122,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: `
@@ -169,27 +188,5 @@ if (x === Foo.a) {}
 `,
       errors: [ruleError(8, 5, 'literalBooleanExpression')],
     },
-
-    // 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'),
-      ],
-    },
   ],
 });