From 50143904e657efd20cefa391046c554ff322544b 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 | 44 ++++--------- .../rules/no-unnecessary-condition.test.ts | 63 +++++++++---------- 2 files changed, 42 insertions(+), 65 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 49c8f13401c8..5f5fb10066a8 100644 --- 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({ +export default createRule<[], MessageId>({ name: 'no-unnecessary-conditionals', meta: { type: 'suggestion', @@ -70,17 +64,7 @@ export default createRule({ 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({ 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({ * 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({ /** * 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({ 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 index f298495a26f8..f74dfba13e7c 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)], }); @@ -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'), - ], - }, ], });