feat(eslint-plugin): [no-unnec-cond] array predicate callbacks (#1206)

Co-authored-by: Brad Zacher <brad.zacher@gmail.com>

packages/eslint-plugin/docs/rules/no-unnecessary-condition.md

@@ -74,6 +74,19 @@ for (; true; ) {}
 do {} while (true);
 ```
 
+- `checkArrayPredicates` (default: `false`) - if set checks that the return value from certain array method callbacks (`filter`, `find`, `some`, `every`) is necessarily conditional.
+
+```ts
+// Valid: numbers can be truthy or falsy.
+[0, 1, 2, 3].filter(t => t);
+
+// Invalid: arrays are always falsy.
+[
+  [1, 2],
+  [3, 4],
+].filter(t => t);
+```
+
 ## When Not To Use It
 
 The main downside to using this rule is the need for type information.

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

@@ -10,6 +10,7 @@ import {
   isFalsyType,
   isBooleanLiteralType,
   isLiteralType,
+  getCallSignaturesOfType,
 } from 'tsutils';
 import {
   createRule,
@@ -60,12 +61,15 @@ export type Options = [
   {
     allowConstantLoopConditions?: boolean;
     ignoreRhs?: boolean;
+    checkArrayPredicates?: boolean;
   },
 ];
 
 export type MessageId =
   | 'alwaysTruthy'
   | 'alwaysFalsy'
+  | 'alwaysTruthyFunc'
+  | 'alwaysFalsyFunc'
   | 'neverNullish'
   | 'alwaysNullish'
   | 'literalBooleanExpression'
@@ -92,6 +96,9 @@ export default createRule<Options, MessageId>({
           ignoreRhs: {
             type: 'boolean',
           },
+          checkArrayPredicates: {
+            type: 'boolean',
+          },
         },
         additionalProperties: false,
       },
@@ -100,6 +107,10 @@ export default createRule<Options, MessageId>({
     messages: {
       alwaysTruthy: 'Unnecessary conditional, value is always truthy.',
       alwaysFalsy: 'Unnecessary conditional, value is always falsy.',
+      alwaysTruthyFunc:
+        'This callback should return a conditional, but return is always truthy',
+      alwaysFalsyFunc:
+        'This callback should return a conditional, but return is always falsy',
       neverNullish:
         'Unnecessary conditional, expected left-hand side of `??` operator to be possibly null or undefined.',
       alwaysNullish:
@@ -114,9 +125,13 @@ export default createRule<Options, MessageId>({
     {
       allowConstantLoopConditions: false,
       ignoreRhs: false,
+      checkArrayPredicates: false,
     },
   ],
-  create(context, [{ allowConstantLoopConditions, ignoreRhs }]) {
+  create(
+    context,
+    [{ allowConstantLoopConditions, checkArrayPredicates, ignoreRhs }],
+  ) {
     const service = getParserServices(context);
     const checker = service.program.getTypeChecker();
     const sourceCode = context.getSourceCode();
@@ -126,6 +141,11 @@ export default createRule<Options, MessageId>({
       return getConstrainedTypeAtLocation(checker, tsNode);
     }
 
+    function nodeIsArrayType(node: TSESTree.Node): boolean {
+      const nodeType = getNodeType(node);
+      return checker.isArrayType(nodeType) || checker.isTupleType(nodeType);
+    }
+
     /**
      * Checks if a conditional node is necessary:
      * if the type of the node is always true or always false, it's not necessary.
@@ -270,6 +290,77 @@ export default createRule<Options, MessageId>({
       checkNode(node.test);
     }
 
+    const ARRAY_PREDICATE_FUNCTIONS = new Set([
+      'filter',
+      'find',
+      'some',
+      'every',
+    ]);
+    function shouldCheckCallback(node: TSESTree.CallExpression): boolean {
+      const { callee } = node;
+      return (
+        // option is on
+        !!checkArrayPredicates &&
+        // looks like `something.filter` or `something.find`
+        callee.type === AST_NODE_TYPES.MemberExpression &&
+        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)
+      );
+    }
+    function checkCallExpression(node: TSESTree.CallExpression): void {
+      const {
+        arguments: [callback],
+      } = node;
+      if (callback && shouldCheckCallback(node)) {
+        // Inline defined functions
+        if (
+          (callback.type === AST_NODE_TYPES.ArrowFunctionExpression ||
+            callback.type === AST_NODE_TYPES.FunctionExpression) &&
+          callback.body
+        ) {
+          // Two special cases, where we can directly check the node that's returned:
+          // () => something
+          if (callback.body.type !== AST_NODE_TYPES.BlockStatement) {
+            return checkNode(callback.body);
+          }
+          // () => { return something; }
+          const callbackBody = callback.body.body;
+          if (
+            callbackBody.length === 1 &&
+            callbackBody[0].type === AST_NODE_TYPES.ReturnStatement &&
+            callbackBody[0].argument
+          ) {
+            return checkNode(callbackBody[0].argument);
+          }
+          // Potential enhancement: could use code-path analysis to check
+          //   any function with a single return statement
+          // (Value to complexity ratio is dubious however)
+        }
+        // Otherwise just do type analysis on the function as a whole.
+        const returnTypes = getCallSignaturesOfType(
+          getNodeType(callback),
+        ).map(sig => sig.getReturnType());
+        /* istanbul ignore if */ if (returnTypes.length === 0) {
+          // Not a callable function
+          return;
+        }
+        if (!returnTypes.some(isPossiblyFalsy)) {
+          return context.report({
+            node: callback,
+            messageId: 'alwaysTruthyFunc',
+          });
+        }
+        if (!returnTypes.some(isPossiblyTruthy)) {
+          return context.report({
+            node: callback,
+            messageId: 'alwaysFalsyFunc',
+          });
+        }
+      }
+    }
+
     function checkOptionalChain(
       node: TSESTree.OptionalMemberExpression | TSESTree.OptionalCallExpression,
       beforeOperator: TSESTree.Node,
@@ -323,6 +414,7 @@ export default createRule<Options, MessageId>({
 
     return {
       BinaryExpression: checkIfBinaryExpressionIsNecessaryConditional,
+      CallExpression: checkCallExpression,
       ConditionalExpression: checkIfTestExpressionIsNecessaryConditional,
       DoWhileStatement: checkIfLoopIsNecessaryConditional,
       ForStatement: checkIfLoopIsNecessaryConditional,

packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts

@@ -88,6 +88,52 @@ function test<T>(t: T | []) {
 function test(a: string) {
   return a === "a"
 }`,
+
+    /**
+     * Predicate functions
+     **/
+    // valid, with the flag off
+    `
+[1,3,5].filter(() => true);
+[1,2,3].find(() => false);
+function truthy() {
+  return [];
+}
+function falsy() {}
+[1,3,5].filter(truthy);
+[1,2,3].find(falsy);
+`,
+    {
+      options: [{ checkArrayPredicates: true }],
+      code: `
+// with literal arrow function
+[0,1,2].filter(x => x);
+
+// filter with named function
+function length(x: string) {
+  return x.length;
+}
+["a", "b", ""].filter(length);
+
+// with non-literal array
+function nonEmptyStrings(x: string[]) {
+  return x.filter(length);
+}
+`,
+    },
+    // Ignores non-array methods of the same name
+    {
+      options: [{ checkArrayPredicates: true }],
+      code: `
+const notArray = {
+  filter: (func: () => boolean) => func(),
+  find: (func: () => boolean) => func(),
+};
+notArray.filter(() => true);
+notArray.find(() => true);
+`,
+    },
+
     // Nullish coalescing operator
     `
 function test(a: string | null) {
@@ -289,6 +335,63 @@ function test(a: never) {
       errors: [ruleError(3, 10, 'never')],
     },
 
+    // Predicate functions
+    {
+      options: [{ checkArrayPredicates: true }],
+      code: `
+[1,3,5].filter(() => true);
+[1,2,3].find(() => { return false; });
+
+// with non-literal array
+function nothing(x: string[]) {
+  return x.filter(() => false);
+}
+// with readonly array
+function nothing2(x: readonly string[]) {
+  return x.filter(() => false);
+}
+// with tuple
+function nothing3(x: [string, string]) {
+  return x.filter(() => false);
+}
+`,
+      errors: [
+        ruleError(2, 22, 'alwaysTruthy'),
+        ruleError(3, 29, 'alwaysFalsy'),
+        ruleError(7, 25, 'alwaysFalsy'),
+        ruleError(11, 25, 'alwaysFalsy'),
+        ruleError(15, 25, 'alwaysFalsy'),
+      ],
+    },
+    {
+      options: [{ checkArrayPredicates: true }],
+      code: `
+function truthy() {
+  return [];
+}
+function falsy() {}
+[1,3,5].filter(truthy);
+[1,2,3].find(falsy);
+`,
+      errors: [
+        ruleError(6, 16, 'alwaysTruthyFunc'),
+        ruleError(7, 14, 'alwaysFalsyFunc'),
+      ],
+    },
+    // Supports generics
+    // TODO: fix this
+    //     {
+    //       options: [{ checkArrayPredicates: true }],
+    //       code: `
+    // const isTruthy = <T>(t: T) => T;
+    // // Valid: numbers can be truthy or falsy (0).
+    // [0,1,2,3].filter(isTruthy);
+    // // Invalid: arrays are always falsy.
+    // [[1,2], [3,4]].filter(isTruthy);
+    // `,
+    //       errors: [ruleError(6, 23, 'alwaysTruthyFunc')],
+    //     },
+
     // Still errors on in the expected locations when ignoring RHS
     {
       options: [{ ignoreRhs: true }],

packages/eslint-plugin/typings/typescript.d.ts

@@ -0,0 +1,21 @@
+import { TypeChecker, Type } from 'typescript';
+
+declare module 'typescript' {
+  interface TypeChecker {
+    // internal TS APIs
+
+    /**
+     * @returns `true` if the given type is an array type:
+     * - Array<foo>
+     * - ReadonlyArray<foo>
+     * - foo[]
+     * - readonly foo[]
+     */
+    isArrayType(type: Type): boolean;
+    /**
+     * @returns `true` if the given type is a tuple type:
+     * - [foo]
+     */
+    isTupleType(type: Type): boolean;
+  }
+}