From 42f46c81686a4b9f5f145fd80c47f31e520ffc8e Mon Sep 17 00:00:00 2001 From: Retsam Date: Tue, 28 Jan 2020 16:13:17 -0500 Subject: [PATCH 1/6] fix(eslint-plugin): [no-unnecessary-cond] ignore basic array index case Adds a special case to suppress "unnecessary condition" errors on code like: ```ts function example(arr: number[]) { // type is number, but could be undefined if(arr[0]) { //... } } ``` --- .../src/rules/no-unnecessary-condition.ts | 16 ++++++++++++- .../rules/no-unnecessary-condition.test.ts | 23 +++++++++++++++++++ 2 files changed, 38 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 22a9fed5ba6..b2bc4e91d62 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -142,6 +142,10 @@ export default createRule({ } function nodeIsArrayType(node: TSESTree.Expression): boolean { + const nodeType = getNodeType(node); + return checker.isArrayType(nodeType); + } + function nodeIsArrayOrTupleType(node: TSESTree.Expression): boolean { const nodeType = getNodeType(node); return checker.isArrayType(nodeType) || checker.isTupleType(nodeType); } @@ -151,6 +155,16 @@ 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 { + // Index signature + if (node.type === AST_NODE_TYPES.MemberExpression && node.computed) { + // 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 (nodeIsArrayType(node.object)) { + return; + } + } + const type = getNodeType(node); // Conditional is always necessary if it involves: @@ -306,7 +320,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) + nodeIsArrayOrTupleType(callee.object) ); } function checkCallExpression(node: TSESTree.CallExpression): void { 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 0728b9202b9..99d4d64d4d9 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,11 @@ 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 +`, // Supports ignoring the RHS { code: ` @@ -362,6 +367,24 @@ 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')], + }, + { + // 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: ` From c4d65276f70dbbceab60b4825c5036f4307d47ef Mon Sep 17 00:00:00 2001 From: Retsam Date: Tue, 28 Jan 2020 18:13:17 -0500 Subject: [PATCH 2/6] fix(eslint-plugin): [no-unnecessary-condition] tuple edge case --- .../src/rules/no-unnecessary-condition.ts | 13 +++++++++---- .../tests/rules/no-unnecessary-condition.test.ts | 12 ++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index b2bc4e91d62..a19e87483ee 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -145,9 +145,9 @@ export default createRule({ const nodeType = getNodeType(node); return checker.isArrayType(nodeType); } - function nodeIsArrayOrTupleType(node: TSESTree.Expression): boolean { + function nodeIsTupleType(node: TSESTree.Expression): boolean { const nodeType = getNodeType(node); - return checker.isArrayType(nodeType) || checker.isTupleType(nodeType); + return checker.isTupleType(nodeType); } /** @@ -160,7 +160,12 @@ export default createRule({ // 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 (nodeIsArrayType(node.object)) { + if ( + nodeIsArrayType(node.object) || + (nodeIsTupleType(node.object) && + // Exception: literal index into a tuple - will have a sound type + node.property.type !== AST_NODE_TYPES.Literal) + ) { return; } } @@ -320,7 +325,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 - nodeIsArrayOrTupleType(callee.object) + (nodeIsArrayType(callee.object) || nodeIsTupleType(callee.object)) ); } function checkCallExpression(node: TSESTree.CallExpression): void { 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 99d4d64d4d9..5177117c385 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -154,6 +154,10 @@ function test(a: unknown) { ` 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]) {} `, // Supports ignoring the RHS { @@ -374,6 +378,14 @@ function nothing3(x: [string, string]) { 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 + code: ` +const x = [{}] as [object]; +if(x[0]) {} `, errors: [ruleError(3, 4, 'alwaysTruthy')], }, From 125229dd55fcb1e134e6e7287e5f2d38a3656a2b Mon Sep 17 00:00:00 2001 From: Retsam Date: Thu, 30 Jan 2020 17:56:16 -0500 Subject: [PATCH 3/6] fix(eslint-plugin): fix array-indexing optional chain case --- .../src/rules/no-unnecessary-condition.ts | 66 +++++++++++++++---- .../rules/no-unnecessary-condition.test.ts | 22 ++++++- 2 files changed, 73 insertions(+), 15 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index a19e87483ee..4caef7320a9 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -150,24 +150,30 @@ export default createRule({ 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)) + ); + } + /** * 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.Expression): void { - // Index signature - if (node.type === AST_NODE_TYPES.MemberExpression && node.computed) { - // 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 ( - nodeIsArrayType(node.object) || - (nodeIsTupleType(node.object) && - // Exception: literal index into a tuple - will have a sound type - node.property.type !== AST_NODE_TYPES.Literal) - ) { - 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 (isArrayIndexExpression(node)) { + return; } const type = getNodeType(node); @@ -380,6 +386,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, @@ -391,6 +424,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 5177117c385..a2b0ad78417 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -159,6 +159,20 @@ 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(); + `, // Supports ignoring the RHS { code: ` @@ -384,10 +398,14 @@ if(dict["mightNotExist"]) {} { // Should still check tuples when accessed with literal numbers, since they don't have code: ` -const x = [{}] as [object]; +const x = [{}] as [{foo: string}]; if(x[0]) {} +if(x[0]?.foo) {} `, - errors: [ruleError(3, 4, 'alwaysTruthy')], + errors: [ + ruleError(3, 4, 'alwaysTruthy'), + ruleError(4, 8, 'neverOptionalChain'), + ], }, { // Shouldn't mistake this for an array indexing case From 23eee5a8a49bd4db804f21b42838c4b9b2a228ee Mon Sep 17 00:00:00 2001 From: Retsam Date: Fri, 31 Jan 2020 19:48:09 -0500 Subject: [PATCH 4/6] fix(eslint-plugin): handle false positive with array index and ?? --- .../eslint-plugin/src/rules/no-unnecessary-condition.ts | 6 ++++++ .../tests/rules/no-unnecessary-condition.test.ts | 5 +++++ 2 files changed, 11 insertions(+) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 4caef7320a9..a1f365911da 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -206,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)) { 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 a2b0ad78417..4e14a301dc6 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -173,6 +173,11 @@ const tuple = ["foo"] as const; declare const n: number; tuple[n]?.toUpperCase(); `, + // nullish + array index + ` +declare const arr: string[][]; +arr[x] ?? []; +`, // Supports ignoring the RHS { code: ` From 25f318c648a21100f2f7fa38d574fca32a1a355c Mon Sep 17 00:00:00 2001 From: Retsam Date: Mon, 2 Mar 2020 12:48:03 -0500 Subject: [PATCH 5/6] chore(eslint-plugin): fix truncated comma --- .../eslint-plugin/tests/rules/no-unnecessary-condition.test.ts | 1 + 1 file changed, 1 insertion(+) 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 4e14a301dc6..dd7e65b8b6d 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -402,6 +402,7 @@ if(dict["mightNotExist"]) {} }, { // 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]) {} From 185be853ef3f95d8420279feea2c3565415eec8e Mon Sep 17 00:00:00 2001 From: Retsam Date: Mon, 2 Mar 2020 12:50:34 -0500 Subject: [PATCH 6/6] chore(eslint-plugin): add suggested test cases --- .../tests/rules/no-unnecessary-condition.test.ts | 5 +++++ 1 file changed, 5 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 dd7e65b8b6d..eb4eb79d90e 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -173,6 +173,11 @@ 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[][];