Skip to content

Commit

Permalink
feat(eslint-plugin): add checks for unnecessary optional chains
Browse files Browse the repository at this point in the history
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?.();
```
  • Loading branch information
jridgewell committed Dec 10, 2019
1 parent 9f76095 commit 382164b
Show file tree
Hide file tree
Showing 2 changed files with 260 additions and 1 deletion.
63 changes: 62 additions & 1 deletion 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 {
Expand All @@ -14,6 +15,9 @@ import {
createRule,
getParserServices,
getConstrainedTypeAtLocation,
isNullableType,
nullThrows,
NullThrowsReasons,
} from '../util';

// Truthiness utilities
Expand Down Expand Up @@ -65,7 +69,8 @@ export type MessageId =
| 'neverNullish'
| 'alwaysNullish'
| 'literalBooleanExpression'
| 'never';
| 'never'
| 'neverOptionalChain';
export default createRule<Options, MessageId>({
name: 'no-unnecessary-conditionals',
meta: {
Expand All @@ -91,6 +96,7 @@ export default createRule<Options, MessageId>({
additionalProperties: false,
},
],
fixable: 'code',
messages: {
alwaysTruthy: 'Unnecessary conditional, value is always truthy.',
alwaysFalsy: 'Unnecessary conditional, value is always falsy.',
Expand All @@ -101,6 +107,7 @@ export default createRule<Options, MessageId>({
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: [
Expand All @@ -112,6 +119,7 @@ export default createRule<Options, MessageId>({
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);
Expand Down Expand Up @@ -260,6 +268,57 @@ export default createRule<Options, MessageId>({
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,
Expand All @@ -268,6 +327,8 @@ export default createRule<Options, MessageId>({
IfStatement: checkIfTestExpressionIsNecessaryConditional,
LogicalExpression: checkLogicalExpressionForUnnecessaryConditionals,
WhileStatement: checkIfLoopIsNecessaryConditional,
OptionalMemberExpression: checkOptionalMemberExpression,
OptionalCallExpression: checkOptionalCallExpression,
};
},
});
198 changes: 198 additions & 0 deletions packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts
Expand Up @@ -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
Expand Down Expand Up @@ -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,
},
],
},
],
});

0 comments on commit 382164b

Please sign in to comment.