diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 38f9204d5e9..2acc0266a53 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -30,6 +30,15 @@ const isPossiblyFalsy = (type: ts.Type): boolean => const isPossiblyTruthy = (type: ts.Type): boolean => unionTypeParts(type).some(type => !isFalsyType(type)); + +// isLiteralType only covers numbers and strings, this is a more exhaustive check. +const isLiteral = (type: ts.Type): boolean => + isBooleanLiteralType(type, true) || + isBooleanLiteralType(type, false) || + type.flags === ts.TypeFlags.Undefined || + type.flags === ts.TypeFlags.Null || + type.flags === ts.TypeFlags.Void || + isLiteralType(type); // #endregion type ExpressionWithTest = @@ -45,7 +54,12 @@ export type Options = [ }, ]; -export default createRule({ +export type MessageId = + | 'alwaysTruthy' + | 'alwaysFalsy' + | 'literalBooleanExpression' + | 'never'; +export default createRule({ name: 'no-unnecessary-conditionals', meta: { type: 'suggestion', @@ -70,6 +84,8 @@ export default createRule({ messages: { alwaysTruthy: 'Unnecessary conditional, value is always truthy.', alwaysFalsy: 'Unnecessary conditional, value is always falsy.', + literalBooleanExpression: + 'Unnecessary conditional, both sides of the expression are literal values', never: 'Unnecessary conditional, value is `never`', }, }, @@ -82,15 +98,17 @@ export default createRule({ const service = getParserServices(context); const checker = service.program.getTypeChecker(); + function getNodeType(node: TSESTree.Node): ts.Type { + const tsNode = service.esTreeNodeToTSNodeMap.get(node); + return getConstrainedTypeAtLocation(checker, tsNode); + } + /** * Checks if a conditional node is necessary: * if the type of the node is always true or always false, it's not necessary. */ function checkNode(node: TSESTree.Node): void { - const tsNode = service.esTreeNodeToTSNodeMap.get( - node, - ); - const type = getConstrainedTypeAtLocation(checker, tsNode); + const type = getNodeType(node); // Conditional is always necessary if it involves `any` or `unknown` if (isTypeFlagSet(type, TypeFlags.Any | TypeFlags.Unknown)) { @@ -105,6 +123,26 @@ export default createRule({ } } + /** + * Checks that a binary expression is necessarily conditoinal, reports otherwise. + * If both sides of the binary expression are literal values, it's not a necessary condition. + * + * NOTE: It's also unnecessary if the types that don't overlap at all + * but that case is handled by the Typescript compiler itself. + */ + const BOOL_OPERATORS = ['<', '>', '<=', '>=', '==', '===', '!=', '!==']; + function checkIfBinaryExpressionIsNecessaryConditional( + node: TSESTree.BinaryExpression, + ): void { + if ( + BOOL_OPERATORS.includes(node.operator) && + isLiteral(getNodeType(node.left)) && + isLiteral(getNodeType(node.right)) + ) { + context.report({ node, messageId: 'literalBooleanExpression' }); + } + } + /** * Checks that a testable expression is necessarily conditional, reports otherwise. * Filters all LogicalExpressions to prevent some duplicate reports. @@ -133,6 +171,7 @@ export default createRule({ } return { + BinaryExpression: checkIfBinaryExpressionIsNecessaryConditional, ConditionalExpression: checkIfTestExpressionIsNecessaryConditional, DoWhileStatement: checkIfTestExpressionIsNecessaryConditional, ForStatement: checkIfTestExpressionIsNecessaryConditional, 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 4ce31715106..f298495a26f 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -1,5 +1,8 @@ import path from 'path'; -import rule, { Options } from '../../src/rules/no-unnecessary-condition'; +import rule, { + Options, + MessageId, +} from '../../src/rules/no-unnecessary-condition'; import { RuleTester } from '../RuleTester'; import { TestCaseError, @@ -16,7 +19,6 @@ const ruleTester = new RuleTester({ }, }); -type MessageId = 'alwaysTruthy' | 'alwaysFalsy' | 'never'; const ruleError = ( line: number, column: number, @@ -71,6 +73,12 @@ function test(t: T) { return t ? 'yes' : 'no' }`, + // Boolean expressions + ` +function test(a: string) { + return a === "a" +}`, + // Supports ignoring the RHS { code: ` @@ -134,6 +142,34 @@ function test(t: T) { errors: [ruleError(3, 10, 'alwaysTruthy')], }, + // Boolean expressions + { + code: ` +function test(a: "a") { + return a === "a" +}`, + errors: [ruleError(3, 10, 'literalBooleanExpression')], + }, + { + code: ` +const y = 1; +if (y === 0) {} +`, + errors: [ruleError(3, 5, 'literalBooleanExpression')], + }, + { + code: ` +enum Foo { + a = 1, + b = 2 +} + +const x = Foo.a; +if (x === Foo.a) {} +`, + errors: [ruleError(8, 5, 'literalBooleanExpression')], + }, + // Still errors on in the expected locations when ignoring RHS { options: [{ ignoreRhs: true }],