From e74bea471cea6bf1f5bc0d9f9f52933249d27a90 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Sat, 7 Dec 2019 03:50:51 -0500 Subject: [PATCH] feat(eslint-plugin): add checks for unnecessary optional chains This adds type checking around the the new optional-chaining feature. With this, it becomes invalid to start an optional chain when the base value is known to be not-nullish. ```js // Good declare const foo: string | null; declare const bar: null : (() => {}); foo?.length; foo?.slice(); bar?.(); // Bad declare const baz: string; declare const qux: (() => {}); baz?.length; baz?.slice(); baz.slice?.(); qux?.(); ``` --- .../src/rules/no-unnecessary-condition.ts | 63 +++++- .../rules/no-unnecessary-condition.test.ts | 198 ++++++++++++++++++ 2 files changed, 260 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 1ee2c2629806..5c884f4506ca 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 0fe1326811c0..84ba5db340ea 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,54 @@ 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: 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: null; +foo?.(); +`, + ` +let anyValue: any; +anyValue?.(); +`, + ` +let unknownValue: unknown; +unknownValue?.(); +`, ], invalid: [ // Ensure that it's checking in all the right places @@ -267,5 +315,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, + }, + ], + }, ], });