feat(eslint-plugin): [no-unnec-conditions] check array predicates option

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

@@ -63,6 +63,16 @@ 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

@@ -9,6 +9,7 @@ import {
   isFalsyType,
   isBooleanLiteralType,
   isLiteralType,
+  getCallSignaturesOfType,
 } from 'tsutils';
 import {
   createRule,
@@ -45,12 +46,15 @@ export type Options = [
   {
     allowConstantLoopConditions?: boolean;
     ignoreRhs?: boolean;
+    checkArrayPredicates?: boolean;
   },
 ];
 
 export type MessageId =
   | 'alwaysTruthy'
   | 'alwaysFalsy'
+  | 'alwaysTruthyFunc'
+  | 'alwaysFalsyFunc'
   | 'literalBooleanExpression'
   | 'never';
 export default createRule<Options, MessageId>({
@@ -74,13 +78,20 @@ export default createRule<Options, MessageId>({
           ignoreRhs: {
             type: 'boolean',
           },
+          checkArrayPredicates: {
+            type: 'boolean',
+          },
         },
         additionalProperties: false,
       },
     ],
     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',
       literalBooleanExpression:
         'Unnecessary conditional, both sides of the expression are literal values',
       never: 'Unnecessary conditional, value is `never`',
@@ -90,9 +101,10 @@ 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();
 
@@ -101,6 +113,13 @@ export default createRule<Options, MessageId>({
       return getConstrainedTypeAtLocation(checker, tsNode);
     }
 
+    function nodeIsArrayType(node: TSESTree.Node): boolean {
+      const type = getNodeType(node);
+      // TODO: is there a better way to determine if this is an array type?
+      //    Currently just checking for a `lastIndexOf` method.
+      return !!type.getProperty('lastIndexOf');
+    }
+
     /**
      * Checks if a conditional node is necessary:
      * if the type of the node is always true or always false, it's not necessary.
@@ -216,8 +235,80 @@ 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',
+          });
+        }
+      }
+    }
+
     return {
       BinaryExpression: checkIfBinaryExpressionIsNecessaryConditional,
+      CallExpression: checkCallExpression,
       ConditionalExpression: checkIfTestExpressionIsNecessaryConditional,
       DoWhileStatement: checkIfLoopIsNecessaryConditional,
       ForStatement: checkIfLoopIsNecessaryConditional,

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

@@ -89,6 +89,51 @@ 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);
+`,
+    },
+
     // Supports ignoring the RHS
     {
       code: `
@@ -188,6 +233,58 @@ if (x === Foo.a) {}
       errors: [ruleError(8, 5, 'literalBooleanExpression')],
     },
 
+    // Predicate functions
+    {
+      options: [{ checkArrayPredicates: true }],
+      code: `
+[1,3,5].filter(() => true);
+[1,2,3].find(() => 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);
+}
+`,
+      errors: [
+        ruleError(2, 22, 'alwaysTruthy'),
+        ruleError(3, 20, 'alwaysFalsy'),
+        ruleError(7, 25, 'alwaysFalsy'),
+        ruleError(11, 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 }],