feat(eslint-plugin): [no-unnecessary-condition] ignore basic array in…

…dexing false positives (#1534)

packages/eslint-plugin/src/rules/no-unnecessary-condition.ts

@@ -143,14 +143,39 @@ export default createRule<Options, MessageId>({
 
     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))
+      );
     }
 
     /**
      * 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 {
+      // 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<Options, MessageId>({
     }
 
     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<Options, MessageId>({
         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<Options, MessageId>({
       }
     }
 
+    // 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<Options, MessageId>({
         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) ||

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<string, object>;
+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: `