feat(eslint-plugin): [no-unnecessary-condition] remove `checkArrayPre…

…dicates` and always check it (#1579)
typescript-eslint · May 21, 2020 · bfd9b60 · bfd9b60
1 parent 7fa9060
commit bfd9b60
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 49 deletions.
diff --git a/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md b/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md
@@ -28,6 +28,12 @@ function bar<T>(arg: string) {
   // arg can never be nullish, so ?. is unnecessary
   return arg?.length;
 }
+
+// Checks array predicate return types, where possible
+[
+  [1, 2],
+  [3, 4],
+].filter(t => t); // number[] is always truthy
 ```
 
 Examples of **correct** code for this rule:
@@ -50,6 +56,8 @@ function bar(arg?: string | null) {
   // Necessary, since arg might be nullish
   return arg?.length;
 }
+
+[0, 1, 2, 3].filter(t => t); // number can be truthy or falsy
 ```
 
 ## Options
@@ -74,19 +82,6 @@ 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.

diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts
@@ -64,7 +64,6 @@ const isLiteral = (type: ts.Type): boolean =>
 export type Options = [
   {
     allowConstantLoopConditions?: boolean;
-    checkArrayPredicates?: boolean;
   },
 ];
 
@@ -97,9 +96,6 @@ export default createRule<Options, MessageId>({
           allowConstantLoopConditions: {
             type: 'boolean',
           },
-          checkArrayPredicates: {
-            type: 'boolean',
-          },
         },
         additionalProperties: false,
       },
@@ -127,10 +123,9 @@ export default createRule<Options, MessageId>({
   defaultOptions: [
     {
       allowConstantLoopConditions: false,
-      checkArrayPredicates: false,
     },
   ],
-  create(context, [{ allowConstantLoopConditions, checkArrayPredicates }]) {
+  create(context, [{ allowConstantLoopConditions }]) {
     const service = getParserServices(context);
     const checker = service.program.getTypeChecker();
     const sourceCode = context.getSourceCode();
@@ -337,11 +332,9 @@ export default createRule<Options, MessageId>({
       'some',
       'every',
     ]);
-    function shouldCheckCallback(node: TSESTree.CallExpression): boolean {
+    function isArrayPredicateFunction(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 &&
@@ -351,10 +344,9 @@ export default createRule<Options, MessageId>({
       );
     }
     function checkCallExpression(node: TSESTree.CallExpression): void {
-      const {
-        arguments: [callback],
-      } = node;
-      if (callback && shouldCheckCallback(node)) {
+      // If this is something like arr.filter(x => /*condition*/), check `condition`
+      if (isArrayPredicateFunction(node) && node.arguments.length) {
+        const callback = node.arguments[0]!;
         // Inline defined functions
         if (
           (callback.type === AST_NODE_TYPES.ArrowFunctionExpression ||

diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts
@@ -113,20 +113,7 @@ function test(a?: string) {
     /**
      * 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);
 
@@ -140,20 +127,16 @@ function length(x: string) {
 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
     `
@@ -470,7 +453,6 @@ function test(a: never) {
 
     // Predicate functions
     {
-      options: [{ checkArrayPredicates: true }],
       code: `
 [1, 3, 5].filter(() => true);
 [1, 2, 3].find(() => {
@@ -534,7 +516,6 @@ if (arr.filter) {
       errors: [ruleError(3, 5, 'alwaysTruthy')],
     },
     {
-      options: [{ checkArrayPredicates: true }],
       code: `
 function truthy() {
   return [];
@@ -551,7 +532,6 @@ function falsy() {}
     // Supports generics
     // TODO: fix this
     //     {
-    //       options: [{ checkArrayPredicates: true }],
     //       code: `
     // const isTruthy = <T>(t: T) => T;
     // // Valid: numbers can be truthy or falsy (0).