From 5e59e648e2e6e097a047fce8d4e6741307d0ce58 Mon Sep 17 00:00:00 2001 From: Retsam Date: Wed, 30 Oct 2019 00:31:36 -0400 Subject: [PATCH] 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 && `. 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. --- .../src/rules/no-unnecessary-condition.ts | 32 ++++------ .../rules/no-unnecessary-condition.test.ts | 63 +++++++++---------- 2 files changed, 41 insertions(+), 54 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 22a9fed5ba65..2ff65a93e937 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/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({ allowConstantLoopConditions: { type: 'boolean', }, - ignoreRhs: { - type: 'boolean', - }, checkArrayPredicates: { type: 'boolean', }, @@ -124,14 +120,10 @@ export default createRule({ 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({ * 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({ /** * 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({ 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({ | TSESTree.ForStatement | TSESTree.WhileStatement, ): void { - if ( - node.test === null || - node.test.type === AST_NODE_TYPES.LogicalExpression - ) { + if (node.test === null) { 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 index 1a7abc30f465..50e7d3158e54 100644 --- 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 => ({ +): InvalidTestCase => ({ 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) {}