diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index 503c06e6894..08030874d99 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -179,7 +179,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-condition`](./docs/rules/no-unnecessary-condition.md) | Prevents conditionals where the type is always truthy or always falsy | | :wrench: | :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 index 4bc160e68cc..cf6c277dfb5 100644 --- a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md +++ b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md @@ -5,7 +5,8 @@ Any expression being used as a condition must be able to evaluate as truthy or f The following expressions are checked: - Arguments to the `&&`, `||` and `?:` (ternary) operators -- Conditions for `if`, `for`, `while`, and `do-while` statements. +- Conditions for `if`, `for`, `while`, and `do-while` statements +- Base values of optional chain expressions Examples of **incorrect** code for this rule: @@ -22,6 +23,11 @@ function foo(arg: 'bar' | 'baz') { if (arg) { } } + +function bar(arg: string) { + // arg can never be nullish, so ?. is unnecessary + return arg?.length; +} ``` Examples of **correct** code for this rule: @@ -39,6 +45,11 @@ function foo(arg: string) { if (arg) { } } + +function bar(arg?: string | null) { + // Necessary, since arg might be nullish + return arg?.length; +} ``` ## Options diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 1ee2c262980..5c884f4506c 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -1,6 +1,7 @@ import { TSESTree, AST_NODE_TYPES, + AST_TOKEN_TYPES, } from '@typescript-eslint/experimental-utils'; import ts, { TypeFlags } from 'typescript'; import { @@ -14,6 +15,9 @@ import { createRule, getParserServices, getConstrainedTypeAtLocation, + isNullableType, + nullThrows, + NullThrowsReasons, } from '../util'; // Truthiness utilities @@ -65,7 +69,8 @@ export type MessageId = | 'neverNullish' | 'alwaysNullish' | 'literalBooleanExpression' - | 'never'; + | 'never' + | 'neverOptionalChain'; export default createRule({ name: 'no-unnecessary-conditionals', meta: { @@ -91,6 +96,7 @@ export default createRule({ additionalProperties: false, }, ], + fixable: 'code', messages: { alwaysTruthy: 'Unnecessary conditional, value is always truthy.', alwaysFalsy: 'Unnecessary conditional, value is always falsy.', @@ -101,6 +107,7 @@ export default createRule({ literalBooleanExpression: 'Unnecessary conditional, both sides of the expression are literal values', never: 'Unnecessary conditional, value is `never`', + neverOptionalChain: 'Unnecessary optional chain on a non-nullish value', }, }, defaultOptions: [ @@ -112,6 +119,7 @@ export default createRule({ create(context, [{ allowConstantLoopConditions, ignoreRhs }]) { const service = getParserServices(context); const checker = service.program.getTypeChecker(); + const sourceCode = context.getSourceCode(); function getNodeType(node: TSESTree.Node): ts.Type { const tsNode = service.esTreeNodeToTSNodeMap.get(node); @@ -260,6 +268,57 @@ export default createRule({ checkNode(node.test); } + function checkOptionalChain( + node: TSESTree.OptionalMemberExpression | TSESTree.OptionalCallExpression, + beforeOperator: TSESTree.Node, + fix: '' | '.', + ): void { + // We only care if this step in the chain is optional. If just descend + // from an optional chain, then that's fine. + if (!node.optional) { + return; + } + + const type = getNodeType(node); + if ( + isTypeFlagSet(type, ts.TypeFlags.Any) || + isTypeFlagSet(type, ts.TypeFlags.Unknown) || + isNullableType(type, { allowUndefined: true }) + ) { + return; + } + + const questionDotOperator = nullThrows( + sourceCode.getTokenAfter( + beforeOperator, + token => + token.type === AST_TOKEN_TYPES.Punctuator && token.value === '?.', + ), + NullThrowsReasons.MissingToken('operator', node.type), + ); + + context.report({ + node, + loc: questionDotOperator.loc, + messageId: 'neverOptionalChain', + fix(fixer) { + return fixer.replaceText(questionDotOperator, fix); + }, + }); + } + + function checkOptionalMemberExpression( + node: TSESTree.OptionalMemberExpression, + ): void { + checkOptionalChain(node, node.object, '.'); + } + + function checkOptionalCallExpression( + node: TSESTree.OptionalCallExpression, + ): void { + checkOptionalChain(node, node.callee, ''); + } + return { BinaryExpression: checkIfBinaryExpressionIsNecessaryConditional, ConditionalExpression: checkIfTestExpressionIsNecessaryConditional, @@ -268,6 +327,8 @@ export default createRule({ IfStatement: checkIfTestExpressionIsNecessaryConditional, LogicalExpression: checkLogicalExpressionForUnnecessaryConditionals, WhileStatement: checkIfLoopIsNecessaryConditional, + OptionalMemberExpression: checkOptionalMemberExpression, + OptionalCallExpression: checkOptionalCallExpression, }; }, }); 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 0fe1326811c..e2e46696224 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -121,6 +121,62 @@ do {} while(true) `, options: [{ allowConstantLoopConditions: true }], }, + ` +let foo: undefined | { bar: true }; +foo?.bar; +`, + ` +let foo: null | { bar: true }; +foo?.bar; +`, + ` +let foo: undefined; +foo?.bar; +`, + ` +let foo: undefined; +foo?.bar.baz; +`, + ` +let foo: null; +foo?.bar; +`, + ` +let anyValue: any; +anyValue?.foo; +`, + ` +let unknownValue: unknown; +unknownValue?.foo; +`, + ` +let foo: undefined | (() => {}); +foo?.(); +`, + ` +let foo: null | (() => {}); +foo?.(); +`, + ` +let foo: undefined; +foo?.(); +`, + ` +let foo: undefined; +foo?.().bar; +`, + ` +let foo: null; +foo?.(); +`, + ` +let anyValue: any; +anyValue?.(); +`, + ` +let unknownValue: unknown; +unknownValue?.(); +`, ], invalid: [ // Ensure that it's checking in all the right places @@ -267,5 +323,155 @@ do {} while(true) ruleError(4, 13, 'alwaysTruthy'), ], }, + { + code: ` +let foo = { bar: true }; +foo?.bar; +foo ?. bar; +foo ?. + bar; +foo + ?. bar; +`, + output: ` +let foo = { bar: true }; +foo.bar; +foo . bar; +foo . + bar; +foo + . bar; +`, + errors: [ + { + messageId: 'neverOptionalChain', + line: 3, + column: 4, + endLine: 3, + endColumn: 6, + }, + { + messageId: 'neverOptionalChain', + line: 4, + column: 5, + endLine: 4, + endColumn: 7, + }, + { + messageId: 'neverOptionalChain', + line: 5, + column: 5, + endLine: 5, + endColumn: 7, + }, + { + messageId: 'neverOptionalChain', + line: 8, + column: 3, + endLine: 8, + endColumn: 5, + }, + ], + }, + { + code: ` +let foo = () => {}; +foo?.(); +foo ?. (); +foo ?. + (); +foo + ?. (); +`, + output: ` +let foo = () => {}; +foo(); +foo (); +foo${' '} + (); +foo + (); +`, + errors: [ + { + messageId: 'neverOptionalChain', + line: 3, + column: 4, + endLine: 3, + endColumn: 6, + }, + { + messageId: 'neverOptionalChain', + line: 4, + column: 5, + endLine: 4, + endColumn: 7, + }, + { + messageId: 'neverOptionalChain', + line: 5, + column: 5, + endLine: 5, + endColumn: 7, + }, + { + messageId: 'neverOptionalChain', + line: 8, + column: 3, + endLine: 8, + endColumn: 5, + }, + ], + }, + { + code: ` +let foo = () => {}; +foo?.(bar); +foo ?. (bar); +foo ?. + (bar); +foo + ?. (bar); +`, + output: ` +let foo = () => {}; +foo(bar); +foo (bar); +foo${' '} + (bar); +foo + (bar); +`, + errors: [ + { + messageId: 'neverOptionalChain', + line: 3, + column: 4, + endLine: 3, + endColumn: 6, + }, + { + messageId: 'neverOptionalChain', + line: 4, + column: 5, + endLine: 4, + endColumn: 7, + }, + { + messageId: 'neverOptionalChain', + line: 5, + column: 5, + endLine: 5, + endColumn: 7, + }, + { + messageId: 'neverOptionalChain', + line: 8, + column: 3, + endLine: 8, + endColumn: 5, + }, + ], + }, ], });