Update: array-callback-return checks Array.forEach (fixes eslint#12551)

array-callback-return rule now checks if Array.forEach returns a value, and, if it does, reports an error. This feature is being added disabled by default, and can be enabled by setting the option "checkForEach" to true. Signed-off-by: Gabriel R Sezefredo <g@briel.dev>
gabrieldrs · Jan 2, 2020 · 4cfed56 · 4cfed56
1 parent e707453
commit 4cfed56
Show file tree

Hide file tree

Showing 3 changed files with 302 additions and 41 deletions.
diff --git a/docs/rules/array-callback-return.md b/docs/rules/array-callback-return.md
@@ -10,7 +10,8 @@ var indexMap = myArray.reduce(function(memo, item, index) {
 }, {}); // Error: cannot set property 'b' of undefined
 ```
 
-This rule enforces usage of `return` statement in callbacks of array's methods.
+This rule enforces usage of `return` statement in callbacks of array's methods.  
+Additionaly, it may also enforce the `forEach` array method callback to __not__ return a value by using the `checkForEach` option.
 
 ## Rule Details
 
@@ -26,8 +27,17 @@ This rule finds callback functions of the following methods, then checks usage o
 * [`Array.prototype.reduceRight`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.reduceright)
 * [`Array.prototype.some`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.some)
 * [`Array.prototype.sort`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.sort)
+* [`Array.prototype.forEach`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.foreach)
 * And above of typed arrays.
 
+## Options
+
+This rule accepts a configuration object with two options:
+
+* `"allowImplicit": false` (default) When set to `true`, allows implicitly returning `undefined` with a `return` statement containing no expression.
+* `"checkForEach": false` (default) When set to `true`, rule will also report `forEach` callbacks that return a value.
+
+
 Examples of **incorrect** code for this rule:
 
 ```js
@@ -70,21 +80,119 @@ var foo = Array.from(nodes, function(node) {
 });
 
 var bar = foo.map(node => node.getAttribute("id"));
+
+myArray.forEach(function(item) {
+    return;
+});
 ```
 
-## Options
+Examples of **correct** code for the `{ "allowImplicit": true }` option:
 
-This rule has an object option:
+```js
+/*eslint array-callback-return: ["error", { allowImplicit: true }]*/
+var undefAllTheThings = myArray.map(function(item) {
+    return;
+});
+```
 
-* `"allowImplicit": false` (default) When set to true, allows implicitly returning `undefined` with a `return` statement containing no expression.
+Examples of **incorrect** code for the `{ "checkForEach": true }` option:
 
-Examples of **correct** code for the `{ "allowImplicit": true }` option:
+```js
+/*eslint array-callback-return: ["error", { checkForEach: true }]*/
+
+myArray.forEach(function(item) {
+    return handleItem(item)
+});
+
+myArray.forEach(function(item) {
+    if (item < 0) {
+        return x;
+    }
+    handleItem(item);
+});
+```
+
+Examples of **correct** code for the `{ "checkForEach": true }` option:
 
 ```js
-/*eslint array-callback-return: ["error", { allowImplicit: true }]*/
+/*eslint array-callback-return: ["error", { checkForEach: true }]*/
+
+myArray.forEach(function(item) {
+    handleItem(item)
+});
+
+myArray.forEach(function(item) {
+    if (item < 0) {
+        return;
+    }
+    handleItem(item);
+});
+
+myArray.forEach(function(item) {
+    handleItem(item);
+    return;
+});
+```
+
+Examples of **incorrect** code for this rule with the `{ "allowImplicit": true, "checkForEach": true }` options:
+
+```js
+/*eslint array-callback-return: ["error", { "allowImplicit": true, "checkForEach": true }]*/
+
+var indexMap = myArray.reduce(function(memo, item, index) {
+    memo[item] = index;
+}, {});
+
+var foo = Array.from(nodes, function(node) {
+    if (node.tagName === "DIV") {
+        return true;
+    }
+});
+
+myArray.forEach(function(item) {
+    return handleItem(item)
+});
+
+myArray.forEach(function(item) {
+    if (item < 0) {
+        return x;
+    }
+    handleItem(item);
+});
+```
+
+Examples of **correct** code for this rule with the `{ "allowImplicit": true, "checkForEach": true }` options:
+
+```js
+/*eslint array-callback-return: ["error", { "allowImplicit": true, "checkForEach": true }]*/
+
 var undefAllTheThings = myArray.map(function(item) {
     return;
 });
+
+var bar = foo.filter(function(x) {
+    if (x) {
+        return true;
+    } else {
+        return;
+    }
+});
+
+myArray.forEach(function(item) {
+    handleItem(item)
+});
+
+myArray.forEach(function(item) {
+    if (item < 0) {
+        return;
+    }
+    handleItem(item);
+});
+
+myArray.forEach(function(item) {
+    handleItem(item);
+    return;
+});
 ```
 
 ## Known Limitations

diff --git a/lib/rules/array-callback-return.js b/lib/rules/array-callback-return.js
@@ -18,7 +18,7 @@ const astUtils = require("./utils/ast-utils");
 //------------------------------------------------------------------------------
 
 const TARGET_NODE_TYPE = /^(?:Arrow)?FunctionExpression$/u;
-const TARGET_METHODS = /^(?:every|filter|find(?:Index)?|map|reduce(?:Right)?|some|sort)$/u;
+const TARGET_METHODS = /^(?:every|filter|find(?:Index)?|map|reduce(?:Right)?|some|sort|forEach)$/u;
 
 /**
  * Checks a given code path segment is reachable.
@@ -61,12 +61,13 @@ function isTargetMethod(node) {
 
 /**
  * Checks whether or not a given node is a function expression which is the
- * callback of an array method.
+ * callback of an array method, returning the method name.
  * @param {ASTNode} node A node to check. This is one of
  *      FunctionExpression or ArrowFunctionExpression.
- * @returns {boolean} `true` if the node is the callback of an array method.
+ * @returns {string} The method name if the node is a callback method,
+ *      null otherwise.
  */
-function isCallbackOfArrayMethod(node) {
+function getArrayMethodName(node) {
     let currentNode = node;
 
     while (currentNode) {
@@ -95,7 +96,7 @@ function isCallbackOfArrayMethod(node) {
                 const func = astUtils.getUpperFunction(parent);
 
                 if (func === null || !astUtils.isCallee(func)) {
-                    return false;
+                    return null;
                 }
                 currentNode = func.parent;
                 break;
@@ -108,27 +109,31 @@ function isCallbackOfArrayMethod(node) {
              */
             case "CallExpression":
                 if (astUtils.isArrayFromMethod(parent.callee)) {
-                    return (
+                    if (
                         parent.arguments.length >= 2 &&
                         parent.arguments[1] === currentNode
-                    );
+                    ) {
+                        return astUtils.getStaticPropertyName(parent.callee);
+                    }
                 }
                 if (isTargetMethod(parent.callee)) {
-                    return (
+                    if (
                         parent.arguments.length >= 1 &&
                         parent.arguments[0] === currentNode
-                    );
+                    ) {
+                        return astUtils.getStaticPropertyName(parent.callee);
+                    }
                 }
-                return false;
+                return null;
 
             // Otherwise this node is not target.
             default:
-                return false;
+                return null;
         }
     }
 
     /* istanbul ignore next: unreachable */
-    return false;
+    return null;
 }
 
 //------------------------------------------------------------------------------
@@ -153,6 +158,10 @@ module.exports = {
                     allowImplicit: {
                         type: "boolean",
                         default: false
+                    },
+                    checkForEach: {
+                        type: "boolean",
+                        default: false
                     }
                 },
                 additionalProperties: false
@@ -162,18 +171,22 @@ module.exports = {
         messages: {
             expectedAtEnd: "Expected to return a value at the end of {{name}}.",
             expectedInside: "Expected to return a value in {{name}}.",
-            expectedReturnValue: "{{name}} expected a return value."
+            expectedReturnValue: "{{name}} expected a return value.",
+            expectedNoReturnValue: "{{name}} did not expect a return value.",
+            noReturnExpected: "Return not expected in {{name}}."
         }
     },
 
     create(context) {
 
-        const options = context.options[0] || { allowImplicit: false };
+        const options = context.options[0] || { allowImplicit: false, checkForEach: false };
 
         let funcInfo = {
+            arrayMethodName: null,
             upper: null,
             codePath: null,
             hasReturn: false,
+            hasReturnValue: false,
             shouldCheck: false,
             node: null
         };
@@ -188,18 +201,32 @@ module.exports = {
          * @returns {void}
          */
         function checkLastSegment(node) {
-            if (funcInfo.shouldCheck &&
-                funcInfo.codePath.currentSegments.some(isReachable)
-            ) {
+
+            if (!funcInfo.shouldCheck) {
+                return;
+            }
+
+            let messageId = null;
+
+            if (funcInfo.arrayMethodName === "forEach") {
+                if (options.checkForEach && node.type === "ArrowFunctionExpression" && node.expression) {
+                    messageId = "expectedNoReturnValue";
+                }
+            } else {
+                if (node.body.type === "BlockStatement" && funcInfo.codePath.currentSegments.some(isReachable)) {
+                    messageId = funcInfo.hasReturn ? "expectedAtEnd" : "expectedInside";
+                }
+            }
+
+            if (messageId) {
+                let name = astUtils.getFunctionNameWithKind(funcInfo.node);
+
+                name = messageId === "expectedNoReturnValue" ? lodash.upperFirst(name) : name;
                 context.report({
                     node,
                     loc: getLocation(node, context.getSourceCode()).loc.start,
-                    messageId: funcInfo.hasReturn
-                        ? "expectedAtEnd"
-                        : "expectedInside",
-                    data: {
-                        name: astUtils.getFunctionNameWithKind(funcInfo.node)
-                    }
+                    messageId,
+                    data: { name }
                 });
             }
         }
@@ -208,14 +235,20 @@ module.exports = {
 
             // Stacks this function's information.
             onCodePathStart(codePath, node) {
+
+                let methodName;
+
+                if (TARGET_NODE_TYPE.test(node.type)) {
+                    methodName = getArrayMethodName(node);
+                }
+
                 funcInfo = {
+                    arrayMethodName: methodName,
                     upper: funcInfo,
                     codePath,
                     hasReturn: false,
                     shouldCheck:
-                        TARGET_NODE_TYPE.test(node.type) &&
-                        node.body.type === "BlockStatement" &&
-                        isCallbackOfArrayMethod(node) &&
+                        methodName &&
                         !node.async &&
                         !node.generator,
                     node
@@ -229,20 +262,38 @@ module.exports = {
 
             // Checks the return statement is valid.
             ReturnStatement(node) {
-                if (funcInfo.shouldCheck) {
-                    funcInfo.hasReturn = true;
+
+                if (!funcInfo.shouldCheck) {
+                    return;
+                }
+
+                funcInfo.hasReturn = true;
+
+                let messageId = null;
+
+                if (funcInfo.arrayMethodName === "forEach") {
+
+                    // if checkForEach: true, returning a value at any path inside a forEach is not allowed
+                    if (options.checkForEach && node.argument) {
+                        messageId = "expectedNoReturnValue";
+                    }
+                } else {
 
                     // if allowImplicit: false, should also check node.argument
                     if (!options.allowImplicit && !node.argument) {
-                        context.report({
-                            node,
-                            messageId: "expectedReturnValue",
-                            data: {
-                                name: lodash.upperFirst(astUtils.getFunctionNameWithKind(funcInfo.node))
-                            }
-                        });
+                        messageId = "expectedReturnValue";
                     }
                 }
+
+                if (messageId) {
+                    context.report({
+                        node,
+                        messageId,
+                        data: {
+                            name: lodash.upperFirst(astUtils.getFunctionNameWithKind(funcInfo.node))
+                        }
+                    });
+                }
             },
 
             // Reports a given function if the last path is reachable.