diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 22a9fed5ba6..4860226beb7 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: @@ -228,20 +225,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. */ @@ -252,10 +235,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 +249,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; } @@ -415,10 +395,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 0728b9202b9..d369e5986d8 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -52,7 +52,8 @@ const t2 = b1 || b2; if(b1 && b2) {} while(b1 && b2) {} for (let i = 0; (b1 && b2); i++) { break; } -const t1 = (b1 && b2) ? 'yes' : 'no'`, +const t1 = (b1 && b2) ? 'yes' : 'no' +for(;;) {}`, necessaryConditionTest('false | 5'), // Truthy literal and falsy literal necessaryConditionTest('boolean | "foo"'), // boolean and truthy literal necessaryConditionTest('0 | boolean'), // boolean and falsy literal @@ -150,13 +151,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: ` @@ -232,16 +233,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 @@ -254,6 +261,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: ` @@ -390,28 +413,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) {}