From 5ec23e1f6fe98d689e32541854343d8e86b10c2a Mon Sep 17 00:00:00 2001 From: Retsam Date: Fri, 12 Jul 2019 13:08:06 -0400 Subject: [PATCH 01/10] feat(eslint-plugin): [no-unnecessary-condition] add rule Adds a no-unnecessary-condition, a rule that uses type information to determine if a condition is necessary. Any condition that's indicated by types to always be truthy or falsy is considered to be unnecessary. Essentially a stronger version of no-constant-condition from core eslint. For example: ```ts const items = ['foo', 'bar']; if(items) { } // Error: items is always truthy ``` --- packages/eslint-plugin/README.md | 1 + .../docs/rules/no-unnecessary-condition.md | 57 +++++++ packages/eslint-plugin/src/configs/all.json | 1 + packages/eslint-plugin/src/rules/index.ts | 2 + .../src/rules/no-unnecessary-condition.ts | 143 ++++++++++++++++++ .../rules/no-unnecessary-condition.test.ts | 119 +++++++++++++++ 6 files changed, 323 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 a3ce816616b..81d94361f8c 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -176,6 +176,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..a842907a4fd --- /dev/null +++ b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md @@ -0,0 +1,57 @@ +# 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[]) { + if (items) { + return items[0].toUpperCase(); + } +} + +const foo = 'foo'; +if (foo) { +} +``` + +Examples of **correct** code for this rule: + +```ts +function head(items: T[]) { + if (items.length) { + return items[0].toUpperCase(); + } +} + +declare const foo: string; +// Necessary, since foo might be ''. (If undesired, consider using `strict-boolean-expressions` rule) +if (foo) { +} +``` + +## 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 diff --git a/packages/eslint-plugin/src/configs/all.json b/packages/eslint-plugin/src/configs/all.json index cce896718db..36feb22f65f 100644 --- a/packages/eslint-plugin/src/configs/all.json +++ b/packages/eslint-plugin/src/configs/all.json @@ -44,6 +44,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 bd39c7867e3..6fb7383b518 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -34,6 +34,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'; @@ -98,6 +99,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..e274a5a8934 --- /dev/null +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -0,0 +1,143 @@ +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) => + isBooleanLiteralType(type, true) || (isLiteralType(type) && type.value); + +const isPossiblyFalsy = (type: ts.Type) => + 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) => + unionTypeParts(type).some(type => !isFalsyType(type)); +// #endregion + +type ExpressionWithTest = + | TSESTree.ConditionalExpression + | TSESTree.DoWhileStatement + | TSESTree.ForStatement + | TSESTree.IfStatement + | TSESTree.WhileStatement; + +type Options = [ + { + ignoreRhs?: boolean; + }, +]; + +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, + }, + schema: [ + { + type: 'object', + properties: { + ignoreRhs: { + type: 'boolean', + }, + }, + additionalProperties: false, + }, + ], + messages: { + alwaysTruthy: 'Unnecessary conditional, value is always truthy.', + alwaysFalsy: 'Unnecessary conditional, value is always falsy.', + never: 'Unnecessary conditional, value is `never`', + }, + }, + defaultOptions: [ + { + ignoreRhs: false, + }, + ], + create(context, [{ ignoreRhs }]) { + const service = getParserServices(context); + const checker = service.program.getTypeChecker(); + + /** + * 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); + + // 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 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 { + 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..842e5b715a9 --- /dev/null +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -0,0 +1,119 @@ +import path from 'path'; +import rule from '../../src/rules/no-unnecessary-condition'; +import { RuleTester } from '../RuleTester'; + +const rootPath = path.join(process.cwd(), 'tests/fixtures/'); + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + parserOptions: { + tsconfigRootDir: rootPath, + project: './tsconfig.json', + }, +}); + +type MessageId = 'alwaysTruthy' | 'alwaysFalsy' | 'never'; +const ruleError = (line: number, column: number, messageId: MessageId) => ({ + messageId, + line, + column, +}); + +const necessaryConditionTest = (condition: string) => ` +declare const b1: ${condition}; +declare const b2: boolean; +const t1 = b1 && b2; +`; + +const unnecessaryConditionTest = (condition: string, messageId: MessageId) => ({ + 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 + + // 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'), + + // 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'), + ], + }, + ], +}); From 0719da8faafde098d7167ad9d0439f53a2661af6 Mon Sep 17 00:00:00 2001 From: Retsam Date: Thu, 18 Jul 2019 11:14:17 -0400 Subject: [PATCH 02/10] docs(eslint-plugin): document as related to strict-boolean-expressions Since no-unnecessary-conditions and strict-boolean-expressions play similar roles but with different levels of strictness, mark them as related to each other in their docs --- packages/eslint-plugin/docs/rules/no-unnecessary-condition.md | 2 ++ packages/eslint-plugin/docs/rules/strict-boolean-expressions.md | 2 ++ 2 files changed, 4 insertions(+) diff --git a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md index a842907a4fd..68df6fc9e40 100644 --- a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md +++ b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md @@ -55,3 +55,5 @@ 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. From 9cbe4d8ed044f1f21a82c7c6906df2f76e99a258 Mon Sep 17 00:00:00 2001 From: Retsam Date: Mon, 19 Aug 2019 16:20:40 -0400 Subject: [PATCH 03/10] test(eslint-plugin): add tests for generic type params --- .../tests/rules/no-unnecessary-condition.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) 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 842e5b715a9..75cb093e2dc 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -54,6 +54,12 @@ const t1 = (b1 && b2) ? 'yes' : 'no'`, necessaryConditionTest('any'), // any necessaryConditionTest('unknown'), // unknown + // Generic type params + ` +function test(t: T) { + return t ? 'yes' : 'no' +}`, + // Supports ignoring the RHS { code: ` @@ -94,6 +100,15 @@ const t1 = (b1 && b2) ? 'yes' : 'no'`, unnecessaryConditionTest('void', 'alwaysFalsy'), unnecessaryConditionTest('never', 'never'), + // Generic type params + { + code: ` +function test(t: T) { + return t ? 'yes' : 'no' +}`, + errors: [ruleError(3, 10, 'alwaysTruthy')], + }, + // Still errors on in the expected locations when ignoring RHS { options: [{ ignoreRhs: true }], From c2a82171ef3697335a70e49b23306a8b9569d560 Mon Sep 17 00:00:00 2001 From: Retsam Date: Wed, 4 Sep 2019 13:59:49 -0400 Subject: [PATCH 04/10] chore(eslint-plugin): fix new lint errors --- .../src/rules/no-unnecessary-condition.ts | 10 +++++----- .../rules/no-unnecessary-condition.test.ts | 19 +++++++++++++++---- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index e274a5a8934..a6b76ec99ea 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -18,17 +18,17 @@ import { // Truthiness utilities // #region -const isTruthyLiteral = (type: ts.Type) => - isBooleanLiteralType(type, true) || (isLiteralType(type) && type.value); +const isTruthyLiteral = (type: ts.Type): boolean => + isBooleanLiteralType(type, true) || (isLiteralType(type) && !!type.value); -const isPossiblyFalsy = (type: ts.Type) => +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) => +const isPossiblyTruthy = (type: ts.Type): boolean => unionTypeParts(type).some(type => !isFalsyType(type)); // #endregion @@ -39,7 +39,7 @@ type ExpressionWithTest = | TSESTree.IfStatement | TSESTree.WhileStatement; -type Options = [ +export type Options = [ { ignoreRhs?: boolean; }, 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 75cb093e2dc..03095c3435b 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -1,6 +1,10 @@ import path from 'path'; -import rule from '../../src/rules/no-unnecessary-condition'; +import rule, { Options } 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/'); @@ -13,19 +17,26 @@ const ruleTester = new RuleTester({ }); type MessageId = 'alwaysTruthy' | 'alwaysFalsy' | 'never'; -const ruleError = (line: number, column: number, messageId: MessageId) => ({ +const ruleError = ( + line: number, + column: number, + messageId: MessageId, +): TestCaseError => ({ messageId, line, column, }); -const necessaryConditionTest = (condition: string) => ` +const necessaryConditionTest = (condition: string): string => ` declare const b1: ${condition}; declare const b2: boolean; const t1 = b1 && b2; `; -const unnecessaryConditionTest = (condition: string, messageId: MessageId) => ({ +const unnecessaryConditionTest = ( + condition: string, + messageId: MessageId, +): InvalidTestCase => ({ code: necessaryConditionTest(condition), errors: [ruleError(4, 12, messageId)], }); From 2d6e759b344d90bcb8e08e5e84c2dd328826ae9b Mon Sep 17 00:00:00 2001 From: Retsam Date: Wed, 4 Sep 2019 17:25:57 -0400 Subject: [PATCH 05/10] chore(eslint-plugin): add requiresTypeChecking --- packages/eslint-plugin/src/rules/no-unnecessary-condition.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index a6b76ec99ea..38f9204d5e9 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -54,6 +54,7 @@ export default createRule({ 'Prevents conditionals where the type is always truthy or always falsy', category: 'Best Practices', recommended: false, + requiresTypeChecking: true, }, schema: [ { From 36d5f5c5dda65b2c5a8482c5c12a81ab69d700be Mon Sep 17 00:00:00 2001 From: Retsam Date: Fri, 6 Sep 2019 15:46:05 -0400 Subject: [PATCH 06/10] chore(eslint-plugin): add more comments to example code --- .../docs/rules/no-unnecessary-condition.md | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md index 68df6fc9e40..c7c6af09f7b 100644 --- a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md +++ b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md @@ -11,13 +11,16 @@ 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(); } } -const foo = 'foo'; -if (foo) { +function foo(arg: 'bar' | 'baz') { + // arg is never nullable or empty string, so this is unnecessary + if (arg) { + } } ``` @@ -25,14 +28,16 @@ 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(); } } -declare const foo: string; -// Necessary, since foo might be ''. (If undesired, consider using `strict-boolean-expressions` rule) -if (foo) { +function foo(arg: 'bar' | 'baz') { + // Necessary, since foo might be ''. + if (arg) { + } } ``` From 5df478840a14926951c8dec54e750c8a2f722646 Mon Sep 17 00:00:00 2001 From: Retsam Date: Fri, 6 Sep 2019 15:49:38 -0400 Subject: [PATCH 07/10] test(eslint-plugin): additional cases for generic params --- .../tests/rules/no-unnecessary-condition.test.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) 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 03095c3435b..4ce31715106 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -119,6 +119,20 @@ function test(t: T) { }`, 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')], + }, // Still errors on in the expected locations when ignoring RHS { From be3dd52f4dd833d096616bee7db4f0932bcfcfa7 Mon Sep 17 00:00:00 2001 From: Retsam Date: Fri, 6 Sep 2019 15:21:39 -0400 Subject: [PATCH 08/10] feat(estlint-plugin): check BooleanExpressions in no-constant-condition If both sides of a boolean expression are literals, the condition is unnecessary --- .../src/rules/no-unnecessary-condition.ts | 49 +++++++++++++++++-- .../rules/no-unnecessary-condition.test.ts | 40 ++++++++++++++- 2 files changed, 82 insertions(+), 7 deletions(-) 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 }], From 5183fb61b89b47ead744729ddb70de84898a9fc4 Mon Sep 17 00:00:00 2001 From: Retsam Date: Sat, 7 Sep 2019 11:47:10 -0400 Subject: [PATCH 09/10] Fixes from code review Co-Authored-By: Brad Zacher --- .../eslint-plugin/docs/rules/no-unnecessary-condition.md | 2 +- .../eslint-plugin/src/rules/no-unnecessary-condition.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md index c7c6af09f7b..ac54dba64ca 100644 --- a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md +++ b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md @@ -34,7 +34,7 @@ function head(items: T[]) { } } -function foo(arg: 'bar' | 'baz') { +function foo(arg: string) { // Necessary, since foo might be ''. if (arg) { } diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 2acc0266a53..59a599a6dcf 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -124,18 +124,18 @@ export default createRule({ } /** - * Checks that a binary expression is necessarily conditoinal, reports otherwise. + * 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 = ['<', '>', '<=', '>=', '==', '===', '!=', '!==']; + const BOOL_OPERATORS = new Set(['<', '>', '<=', '>=', '==', '===', '!=', '!==']); function checkIfBinaryExpressionIsNecessaryConditional( node: TSESTree.BinaryExpression, ): void { if ( - BOOL_OPERATORS.includes(node.operator) && + BOOL_OPERATORS.has(node.operator) && isLiteral(getNodeType(node.left)) && isLiteral(getNodeType(node.right)) ) { From 17f0e2535aff86bdea80ee274f3f33c7d37dbdcf Mon Sep 17 00:00:00 2001 From: Brad Zacher Date: Sat, 7 Sep 2019 17:01:05 -0700 Subject: [PATCH 10/10] fix formatting --- .../src/rules/no-unnecessary-condition.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 59a599a6dcf..49c8f13401c 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -130,7 +130,16 @@ export default createRule({ * 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(['<', '>', '<=', '>=', '==', '===', '!=', '!==']); + const BOOL_OPERATORS = new Set([ + '<', + '>', + '<=', + '>=', + '==', + '===', + '!=', + '!==', + ]); function checkIfBinaryExpressionIsNecessaryConditional( node: TSESTree.BinaryExpression, ): void {