From 571548243a7ce2cc3c42134ae18c03c065b2d560 Mon Sep 17 00:00:00 2001 From: Retsam Date: Thu, 12 Sep 2019 04:42:09 -0400 Subject: [PATCH] feat(eslint-plugin): add no-unnecessary-condition rule (#699) --- packages/eslint-plugin/README.md | 1 + .../docs/rules/no-unnecessary-condition.md | 64 ++++++ .../docs/rules/strict-boolean-expressions.md | 2 + packages/eslint-plugin/src/configs/all.json | 1 + packages/eslint-plugin/src/rules/index.ts | 2 + .../src/rules/no-unnecessary-condition.ts | 192 +++++++++++++++++ .../rules/no-unnecessary-condition.test.ts | 195 ++++++++++++++++++ 7 files changed, 457 insertions(+) create mode 100644 packages/eslint-plugin/docs/rules/no-unnecessary-condition.md create mode 100644 packages/eslint-plugin/src/rules/no-unnecessary-condition.ts create mode 100644 packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 178ca903a9f..baec8f2426e 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -177,6 +177,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e | [`@typescript-eslint/no-require-imports`](./docs/rules/no-require-imports.md) | Disallows invocation of `require()` | | | | | [`@typescript-eslint/no-this-alias`](./docs/rules/no-this-alias.md) | Disallow aliasing `this` | :heavy_check_mark: | | | | [`@typescript-eslint/no-type-alias`](./docs/rules/no-type-alias.md) | Disallow the use of type aliases | | | | +| [`@typescript-eslint/no-unnecessary-condition`](./docs/rules/no-unnecessary-condition.md) | Prevents conditionals where the type is always truthy or always falsy | | | :thought_balloon: | | [`@typescript-eslint/no-unnecessary-qualifier`](./docs/rules/no-unnecessary-qualifier.md) | Warns when a namespace qualifier is unnecessary | | :wrench: | :thought_balloon: | | [`@typescript-eslint/no-unnecessary-type-arguments`](./docs/rules/no-unnecessary-type-arguments.md) | Warns if an explicitly specified type argument is the default for that type parameter | | :wrench: | :thought_balloon: | | [`@typescript-eslint/no-unnecessary-type-assertion`](./docs/rules/no-unnecessary-type-assertion.md) | Warns if a type assertion does not change the type of an expression | :heavy_check_mark: | :wrench: | :thought_balloon: | diff --git a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md new file mode 100644 index 00000000000..ac54dba64ca --- /dev/null +++ b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md @@ -0,0 +1,64 @@ +# Condition expressions must be necessary + +Any expression being used as a condition must be able to evaluate as truthy or falsy in order to be considered "necessary". Conversely, any expression that always evaluates to truthy or always evaluates to falsy, as determined by the type of the expression, is considered unnecessary and will be flagged by this rule. + +The following expressions are checked: + +- Arguments to the `&&`, `||` and `?:` (ternary) operators +- Conditions for `if`, `for`, `while`, and `do-while` statements. + +Examples of **incorrect** code for this rule: + +```ts +function head(items: T[]) { + // items can never be nullable, so this is unnecessary + if (items) { + return items[0].toUpperCase(); + } +} + +function foo(arg: 'bar' | 'baz') { + // arg is never nullable or empty string, so this is unnecessary + if (arg) { + } +} +``` + +Examples of **correct** code for this rule: + +```ts +function head(items: T[]) { + // Necessary, since items.length might be 0 + if (items.length) { + return items[0].toUpperCase(); + } +} + +function foo(arg: string) { + // Necessary, since foo might be ''. + if (arg) { + } +} +``` + +## Options + +Accepts an object with the following options: + +- `ignoreRhs` (default `false`) - doesn't check if the right-hand side of `&&` and `||` is a necessary condition. For example, the following code is valid with this option on: + +```ts +function head(items: T[]) { + return items.length && items[0].toUpperCase(); +} +``` + +## When Not To Use It + +The main downside to using this rule is the need for type information. + +## Related To + +- ESLint: [no-constant-condition](https://eslint.org/docs/rules/no-constant-condition) - this rule is essentially a stronger versison + +- [strict-boolean-expression](./strict-boolean-expressions.md) - a stricter alternative to this rule. diff --git a/packages/eslint-plugin/docs/rules/strict-boolean-expressions.md b/packages/eslint-plugin/docs/rules/strict-boolean-expressions.md index de27437a47c..6bed5b03fe3 100644 --- a/packages/eslint-plugin/docs/rules/strict-boolean-expressions.md +++ b/packages/eslint-plugin/docs/rules/strict-boolean-expressions.md @@ -61,3 +61,5 @@ Options may be provided as an object with: ## Related To - TSLint: [strict-boolean-expressions](https://palantir.github.io/tslint/rules/strict-boolean-expressions) + +- [no-unnecessary-condition](./no-unnecessary-condition.md) - a looser alternative to this rule. diff --git a/packages/eslint-plugin/src/configs/all.json b/packages/eslint-plugin/src/configs/all.json index 0974b7a7263..377f4b58f0e 100644 --- a/packages/eslint-plugin/src/configs/all.json +++ b/packages/eslint-plugin/src/configs/all.json @@ -46,6 +46,7 @@ "@typescript-eslint/no-require-imports": "error", "@typescript-eslint/no-this-alias": "error", "@typescript-eslint/no-type-alias": "error", + "@typescript-eslint/no-unnecessary-condition": "error", "@typescript-eslint/no-unnecessary-qualifier": "error", "@typescript-eslint/no-unnecessary-type-arguments": "error", "@typescript-eslint/no-unnecessary-type-assertion": "error", diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index 8ae9d698cca..5302abd05de 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -35,6 +35,7 @@ import noParameterProperties from './no-parameter-properties'; import noRequireImports from './no-require-imports'; import noThisAlias from './no-this-alias'; import noTypeAlias from './no-type-alias'; +import noUnnecessaryCondition from './no-unnecessary-condition'; import noUnnecessaryQualifier from './no-unnecessary-qualifier'; import noUnnecessaryTypeAssertion from './no-unnecessary-type-assertion'; import noUnusedVars from './no-unused-vars'; @@ -100,6 +101,7 @@ export default { 'no-require-imports': noRequireImports, 'no-this-alias': noThisAlias, 'no-type-alias': noTypeAlias, + 'no-unnecessary-condition': noUnnecessaryCondition, 'no-unnecessary-qualifier': noUnnecessaryQualifier, 'no-unnecessary-type-arguments': useDefaultTypeParameter, 'no-unnecessary-type-assertion': noUnnecessaryTypeAssertion, diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts new file mode 100644 index 00000000000..49c8f13401c --- /dev/null +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -0,0 +1,192 @@ +import { + TSESTree, + AST_NODE_TYPES, +} from '@typescript-eslint/experimental-utils'; +import ts, { TypeFlags } from 'typescript'; +import { + isTypeFlagSet, + unionTypeParts, + isFalsyType, + isBooleanLiteralType, + isLiteralType, +} from 'tsutils'; +import { + createRule, + getParserServices, + getConstrainedTypeAtLocation, +} from '../util'; + +// Truthiness utilities +// #region +const isTruthyLiteral = (type: ts.Type): boolean => + isBooleanLiteralType(type, true) || (isLiteralType(type) && !!type.value); + +const isPossiblyFalsy = (type: ts.Type): boolean => + unionTypeParts(type) + // PossiblyFalsy flag includes literal values, so exclude ones that + // are definitely truthy + .filter(t => !isTruthyLiteral(t)) + .some(type => isTypeFlagSet(type, ts.TypeFlags.PossiblyFalsy)); + +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 = + | TSESTree.ConditionalExpression + | TSESTree.DoWhileStatement + | TSESTree.ForStatement + | TSESTree.IfStatement + | TSESTree.WhileStatement; + +export type Options = [ + { + ignoreRhs?: boolean; + }, +]; + +export type MessageId = + | 'alwaysTruthy' + | 'alwaysFalsy' + | 'literalBooleanExpression' + | 'never'; +export default createRule({ + name: 'no-unnecessary-conditionals', + meta: { + type: 'suggestion', + docs: { + description: + 'Prevents conditionals where the type is always truthy or always falsy', + category: 'Best Practices', + recommended: false, + requiresTypeChecking: true, + }, + schema: [ + { + type: 'object', + properties: { + ignoreRhs: { + type: 'boolean', + }, + }, + additionalProperties: false, + }, + ], + 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`', + }, + }, + defaultOptions: [ + { + ignoreRhs: false, + }, + ], + create(context, [{ ignoreRhs }]) { + 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 type = getNodeType(node); + + // Conditional is always necessary if it involves `any` or `unknown` + if (isTypeFlagSet(type, TypeFlags.Any | TypeFlags.Unknown)) { + return; + } + if (isTypeFlagSet(type, TypeFlags.Never)) { + context.report({ node, messageId: 'never' }); + } else if (!isPossiblyTruthy(type)) { + context.report({ node, messageId: 'alwaysFalsy' }); + } else if (!isPossiblyFalsy(type)) { + context.report({ node, messageId: 'alwaysTruthy' }); + } + } + + /** + * Checks that a binary expression is necessarily conditional, 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 = new Set([ + '<', + '>', + '<=', + '>=', + '==', + '===', + '!=', + '!==', + ]); + function checkIfBinaryExpressionIsNecessaryConditional( + node: TSESTree.BinaryExpression, + ): void { + if ( + BOOL_OPERATORS.has(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. + */ + function checkIfTestExpressionIsNecessaryConditional( + node: ExpressionWithTest, + ): void { + if ( + node.test !== null && + node.test.type !== AST_NODE_TYPES.LogicalExpression + ) { + checkNode(node.test); + } + } + + /** + * Checks that a logical expression contains a boolean, reports otherwise. + */ + function checkLogicalExpressionForUnnecessaryConditionals( + node: TSESTree.LogicalExpression, + ): void { + checkNode(node.left); + if (!ignoreRhs) { + checkNode(node.right); + } + } + + return { + BinaryExpression: checkIfBinaryExpressionIsNecessaryConditional, + ConditionalExpression: checkIfTestExpressionIsNecessaryConditional, + DoWhileStatement: checkIfTestExpressionIsNecessaryConditional, + ForStatement: checkIfTestExpressionIsNecessaryConditional, + IfStatement: checkIfTestExpressionIsNecessaryConditional, + WhileStatement: checkIfTestExpressionIsNecessaryConditional, + LogicalExpression: checkLogicalExpressionForUnnecessaryConditionals, + }; + }, +}); diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts new file mode 100644 index 00000000000..f298495a26f --- /dev/null +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -0,0 +1,195 @@ +import path from 'path'; +import rule, { + Options, + MessageId, +} from '../../src/rules/no-unnecessary-condition'; +import { RuleTester } from '../RuleTester'; +import { + TestCaseError, + InvalidTestCase, +} from '@typescript-eslint/experimental-utils/dist/ts-eslint'; + +const rootPath = path.join(process.cwd(), 'tests/fixtures/'); + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + parserOptions: { + tsconfigRootDir: rootPath, + project: './tsconfig.json', + }, +}); + +const ruleError = ( + line: number, + column: number, + messageId: MessageId, +): TestCaseError => ({ + messageId, + line, + column, +}); + +const necessaryConditionTest = (condition: string): string => ` +declare const b1: ${condition}; +declare const b2: boolean; +const t1 = b1 && b2; +`; + +const unnecessaryConditionTest = ( + condition: string, + messageId: MessageId, +): InvalidTestCase => ({ + code: necessaryConditionTest(condition), + errors: [ruleError(4, 12, messageId)], +}); + +ruleTester.run('no-unnecessary-conditionals', rule, { + valid: [ + ` +declare const b1: boolean; +declare const b2: boolean; +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'`, + necessaryConditionTest('false | 5'), // Truthy literal and falsy literal + necessaryConditionTest('boolean | "foo"'), // boolean and truthy literal + necessaryConditionTest('0 | boolean'), // boolean and falsy literal + necessaryConditionTest('boolean | object'), // boolean and always-truthy type + necessaryConditionTest('false | object'), // always truthy type and falsy literal + // always falsy type and always truthy type + necessaryConditionTest('null | object'), + necessaryConditionTest('undefined | true'), + necessaryConditionTest('void | true'), + + necessaryConditionTest('any'), // any + necessaryConditionTest('unknown'), // unknown + + // Generic type params + ` +function test(t: T) { + return t ? 'yes' : 'no' +}`, + + // Boolean expressions + ` +function test(a: string) { + return a === "a" +}`, + + // Supports ignoring the RHS + { + code: ` +declare const b1: boolean; +declare const b2: true; +if(b1 && b2) {}`, + options: [{ ignoreRhs: true }], + }, + ], + invalid: [ + // Ensure that it's checking in all the right places + { + code: ` +const b1 = true; +declare const b2: boolean; +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'), + ], + }, + // Ensure that it's complaining about the right things + unnecessaryConditionTest('object', 'alwaysTruthy'), + unnecessaryConditionTest('object | true', 'alwaysTruthy'), + unnecessaryConditionTest('"" | false', 'alwaysFalsy'), // Two falsy literals + unnecessaryConditionTest('"always truthy"', 'alwaysTruthy'), + unnecessaryConditionTest(`undefined`, 'alwaysFalsy'), + unnecessaryConditionTest('null', 'alwaysFalsy'), + unnecessaryConditionTest('void', 'alwaysFalsy'), + unnecessaryConditionTest('never', 'never'), + + // Generic type params + { + code: ` +function test(t: T) { + return t ? 'yes' : 'no' +}`, + errors: [ruleError(3, 10, 'alwaysTruthy')], + }, + { + code: ` +function test(t: T) { + return t ? 'yes' : 'no' +}`, + errors: [ruleError(3, 10, 'alwaysFalsy')], + }, + { + code: ` +function test(t: T) { + return t ? 'yes' : 'no' +}`, + 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 }], + 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'), + ], + }, + ], +});