From ee8dd8f8a9e6c25ac426ce9bb71c5f012c51f264 Mon Sep 17 00:00:00 2001 From: Retsam Date: Sat, 9 May 2020 19:27:48 -0400 Subject: [PATCH] feat(eslint-plugin): [no-unnecessary-condition] remove option `ignoreRHS` (#1163) --- .../src/rules/no-unnecessary-condition.ts | 45 ++++-------- .../rules/no-unnecessary-condition.test.ts | 73 ++++++++++--------- 2 files changed, 53 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 a64a3ddc9c9..919e182a749 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(); @@ -176,6 +168,12 @@ export default createRule({ return; } + // 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: @@ -259,20 +257,6 @@ 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; - } - - checkNode(node.test); - } - /** * Checks that a logical expression contains a boolean, reports otherwise. */ @@ -283,10 +267,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); - } } /** @@ -298,10 +281,8 @@ export default createRule({ | TSESTree.ForStatement | TSESTree.WhileStatement, ): void { - if ( - node.test === null || - node.test.type === AST_NODE_TYPES.LogicalExpression - ) { + if (node.test === null) { + // e.g. `for(;;)` return; } @@ -480,10 +461,10 @@ export default createRule({ 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, 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 42f9e288b23..913aca55565 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -56,6 +56,14 @@ for (let i = 0; b1 && b2; i++) { break; } const t1 = b1 && b2 ? 'yes' : 'no'; +if (b1 && b2) { +} +while (b1 && b2) {} +for (let i = 0; b1 && b2; i++) { + break; +} +const t1 = b1 && b2 ? 'yes' : 'no'; +for (;;) {} `, necessaryConditionTest('false | 5'), // Truthy literal and falsy literal necessaryConditionTest('boolean | "foo"'), // boolean and truthy literal @@ -203,15 +211,14 @@ returnsArr?.()[42]?.toUpperCase(); declare const arr: string[][]; arr[x] ?? []; `, - // 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) { -} +const x = b1 && b2; `, - options: [{ ignoreRhs: true }], }, { code: ` @@ -289,19 +296,26 @@ 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 = b2 && b1 ? 'yes' : 'no'; `, errors: [ ruleError(4, 12, 'alwaysTruthy'), ruleError(5, 12, 'alwaysTruthy'), ruleError(6, 5, 'alwaysTruthy'), - ruleError(8, 8, 'alwaysTruthy'), - ruleError(9, 17, 'alwaysTruthy'), - ruleError(12, 12, 'alwaysTruthy'), + ruleError(8, 11, 'alwaysTruthy'), + ruleError(10, 8, 'alwaysTruthy'), + ruleError(11, 14, 'alwaysTruthy'), + ruleError(12, 17, 'alwaysTruthy'), + ruleError(15, 12, 'alwaysTruthy'), + ruleError(16, 18, 'alwaysTruthy'), ], }, // Ensure that it's complaining about the right things @@ -314,6 +328,25 @@ 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, 5, 'alwaysTruthy'), + ruleError(6, 11, 'alwaysFalsy'), + ruleError(8, 17, 'alwaysTruthy'), + ], + }, + // Generic type params { code: ` @@ -497,32 +530,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, 5, 'alwaysTruthy'), - ruleError(8, 8, 'alwaysTruthy'), - ruleError(9, 17, 'alwaysTruthy'), - ruleError(12, 12, 'alwaysTruthy'), - ], - }, { code: ` while (true) {}