From 2b9603d868c57556d8cd6087685e798d74cb6f26 Mon Sep 17 00:00:00 2001 From: Retsam Date: Thu, 19 Mar 2020 23:47:48 -0400 Subject: [PATCH] feat(eslint-plugin): [no-unnecessary-condition] ignore basic array indexing false positives (#1534) --- .../src/rules/no-unnecessary-condition.ts | 69 ++++++++++++++++++- .../rules/no-unnecessary-condition.test.ts | 64 +++++++++++++++++ 2 files changed, 131 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 45af1e80706..07bb3d56c5c 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -143,7 +143,25 @@ export default createRule({ function nodeIsArrayType(node: TSESTree.Expression): boolean { const nodeType = getNodeType(node); - return checker.isArrayType(nodeType) || checker.isTupleType(nodeType); + return checker.isArrayType(nodeType); + } + function nodeIsTupleType(node: TSESTree.Expression): boolean { + const nodeType = getNodeType(node); + return checker.isTupleType(nodeType); + } + + function isArrayIndexExpression(node: TSESTree.Expression): boolean { + return ( + // Is an index signature + node.type === AST_NODE_TYPES.MemberExpression && + node.computed && + // ...into an array type + (nodeIsArrayType(node.object) || + // ... or a tuple type + (nodeIsTupleType(node.object) && + // Exception: literal index into a tuple - will have a sound type + node.property.type !== AST_NODE_TYPES.Literal)) + ); } /** @@ -151,6 +169,13 @@ export default createRule({ * if the type of the node is always true or always false, it's not necessary. */ function checkNode(node: TSESTree.Expression): void { + // Since typescript array index signature types don't represent the + // possibility of out-of-bounds access, if we're indexing into an array + // just skip the check, to avoid false positives + if (isArrayIndexExpression(node)) { + return; + } + const type = getNodeType(node); // Conditional is always necessary if it involves: @@ -181,6 +206,12 @@ export default createRule({ } function checkNodeForNullish(node: TSESTree.Expression): void { + // Since typescript array index signature types don't represent the + // possibility of out-of-bounds access, if we're indexing into an array + // just skip the check, to avoid false positives + if (isArrayIndexExpression(node)) { + return; + } const type = getNodeType(node); // Conditional is always necessary if it involves `any` or `unknown` if (isTypeFlagSet(type, ts.TypeFlags.Any | ts.TypeFlags.Unknown)) { @@ -306,7 +337,7 @@ export default createRule({ callee.property.type === AST_NODE_TYPES.Identifier && ARRAY_PREDICATE_FUNCTIONS.has(callee.property.name) && // and the left-hand side is an array, according to the types - nodeIsArrayType(callee.object) + (nodeIsArrayType(callee.object) || nodeIsTupleType(callee.object)) ); } function checkCallExpression(node: TSESTree.CallExpression): void { @@ -361,6 +392,33 @@ export default createRule({ } } + // Recursively searches an optional chain for an array index expression + // Has to search the entire chain, because an array index will "infect" the rest of the types + // Example: + // ``` + // [{x: {y: "z"} }][n] // type is {x: {y: "z"}} + // ?.x // type is {y: "z"} + // ?.y // This access is considered "unnecessary" according to the types + // ``` + function optionChainContainsArrayIndex( + node: TSESTree.OptionalMemberExpression | TSESTree.OptionalCallExpression, + ): boolean { + const lhsNode = + node.type === AST_NODE_TYPES.OptionalCallExpression + ? node.callee + : node.object; + if (isArrayIndexExpression(lhsNode)) { + return true; + } + if ( + lhsNode.type === AST_NODE_TYPES.OptionalMemberExpression || + lhsNode.type === AST_NODE_TYPES.OptionalCallExpression + ) { + return optionChainContainsArrayIndex(lhsNode); + } + return false; + } + function checkOptionalChain( node: TSESTree.OptionalMemberExpression | TSESTree.OptionalCallExpression, beforeOperator: TSESTree.Node, @@ -372,6 +430,13 @@ export default createRule({ return; } + // Since typescript array index signature types don't represent the + // possibility of out-of-bounds access, if we're indexing into an array + // just skip the check, to avoid false positives + if (optionChainContainsArrayIndex(node)) { + return; + } + const type = getNodeType(node); if ( isTypeFlagSet(type, ts.TypeFlags.Any) || 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 5af22f00ca0..f3adbc3a849 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -150,6 +150,39 @@ function test(a: string | null | undefined) { function test(a: unknown) { return a ?? "default"; }`, + // Indexing cases + ` +declare const arr: object[]; +if(arr[42]) {} // looks unnecessary from the types, but isn't + +const tuple = [{}] as [object]; +declare const n: number; +if(tuple[n]) {} +`, + // Optional-chaining indexing + ` +declare const arr: Array<{value: string} & (() => void)>; +if(arr[42]?.value) {} +arr[41]?.(); + +// An array access can "infect" deeper into the chain +declare const arr2: Array<{x: {y: {z: object}}}>; +arr2[42]?.x?.y?.z; + +const tuple = ["foo"] as const; +declare const n: number; +tuple[n]?.toUpperCase(); + `, + `if(arr?.[42]) {}`, + ` +declare const returnsArr: undefined | (() => string[]); +if(returnsArr?.()[42]) {} +returnsArr?.()[42]?.toUpperCase()`, + // nullish + array index + ` +declare const arr: string[][]; +arr[x] ?? []; +`, // Supports ignoring the RHS { code: ` @@ -363,6 +396,37 @@ function nothing3(x: [string, string]) { ruleError(15, 25, 'alwaysFalsy'), ], }, + // Indexing cases + { + // This is an error because 'dict' doesn't represent + // the potential for undefined in its types + code: ` +declare const dict: Record; +if(dict["mightNotExist"]) {} +`, + errors: [ruleError(3, 4, 'alwaysTruthy')], + }, + { + // Should still check tuples when accessed with literal numbers, since they don't have + // unsound index signatures + code: ` +const x = [{}] as [{foo: string}]; +if(x[0]) {} +if(x[0]?.foo) {} +`, + errors: [ + ruleError(3, 4, 'alwaysTruthy'), + ruleError(4, 8, 'neverOptionalChain'), + ], + }, + { + // Shouldn't mistake this for an array indexing case + code: ` +declare const arr: object[]; +if(arr.filter) {} +`, + errors: [ruleError(3, 4, 'alwaysTruthy')], + }, { options: [{ checkArrayPredicates: true }], code: `