From fd5023e45c98ce369fbbb67db8045cd68ef04075 Mon Sep 17 00:00:00 2001 From: Gabriel R Sezefredo Date: Thu, 5 Dec 2019 15:58:40 -0300 Subject: [PATCH] Update: array-callback-return checks Array.forEach (fixes #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 --- docs/rules/array-callback-return.md | 36 +++++++++- lib/rules/array-callback-return.js | 87 +++++++++++++++++++++--- tests/lib/rules/array-callback-return.js | 30 ++++++++ 3 files changed, 141 insertions(+), 12 deletions(-) diff --git a/docs/rules/array-callback-return.md b/docs/rules/array-callback-return.md index 34ffeffcebd..9e4b97d2e71 100644 --- a/docs/rules/array-callback-return.md +++ b/docs/rules/array-callback-return.md @@ -74,7 +74,7 @@ var bar = foo.map(node => node.getAttribute("id")); ## Options -This rule has an object option: +This rule has two object options: * `"allowImplicit": false` (default) When set to true, allows implicitly returning `undefined` with a `return` statement containing no expression. @@ -87,6 +87,40 @@ var undefAllTheThings = myArray.map(function(item) { }); ``` +* `"checkForEach": false` (default) When set to true, rule will also report forEach callbacks which return a value. + +Examples of **incorrect** code for the `{ "checkForEach": 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", { checkForEach: true }]*/ +myArray.forEach(function(item) { + handleItem(item) +}); + +myArray.forEach(function(item) { + if (item < 0) { + return; + } + handleItem(item); +}); +``` + ## Known Limitations This rule checks callback functions of methods with the given names, *even if* the object which has the method is *not* an array. diff --git a/lib/rules/array-callback-return.js b/lib/rules/array-callback-return.js index d632a3f30c2..037ad3bb139 100644 --- 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. @@ -131,6 +131,33 @@ function isCallbackOfArrayMethod(node) { return false; } +/** + * Retrieves the array method name given a node inside of it. + * Method starts from a given node, and goes up in the parent chain until it finds the array method name. + * @param {ASTNode} node A node inside an array method. + * @returns {string} the method name or null if not available. + */ +function getArrayMethodName(node) { + + for (let currentNode = node; currentNode !== null; currentNode = currentNode.parent) { + + if (currentNode.type === "CallExpression" && currentNode.callee.type === "MemberExpression") { + + // for members being called as foo.every(...) + if (currentNode.callee.computed === false) { + return currentNode.callee.property.name; + } + + // for members being called as foo["every"](...) + if (currentNode.callee.computed === true) { + return currentNode.callee.property.value; + } + } + + } + return null; +} + //------------------------------------------------------------------------------ // Rule Definition //------------------------------------------------------------------------------ @@ -153,6 +180,10 @@ module.exports = { allowImplicit: { type: "boolean", default: false + }, + checkForEach: { + type: "boolean", + default: false } }, additionalProperties: false @@ -162,18 +193,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,15 +223,28 @@ module.exports = { * @returns {void} */ function checkLastSegment(node) { - if (funcInfo.shouldCheck && - funcInfo.codePath.currentSegments.some(isReachable) - ) { + + let messageId = null; + + if (funcInfo.shouldCheck) { + if (funcInfo.arrayMethodName === "forEach") { + if (options.checkForEach && + !funcInfo.codePath.currentSegments.some(isReachable) + ) { + messageId = "noReturnExpected"; + } + } else { + if (funcInfo.codePath.currentSegments.some(isReachable)) { + messageId = funcInfo.hasReturn ? "expectedAtEnd" : "expectedInside"; + } + } + } + + if (messageId) { context.report({ node, loc: getLocation(node, context.getSourceCode()).loc.start, - messageId: funcInfo.hasReturn - ? "expectedAtEnd" - : "expectedInside", + messageId, data: { name: astUtils.getFunctionNameWithKind(funcInfo.node) } @@ -220,6 +268,10 @@ module.exports = { !node.generator, node }; + + if (funcInfo.shouldCheck) { + funcInfo.arrayMethodName = getArrayMethodName(node); + } }, // Pops this function's information. @@ -229,14 +281,27 @@ module.exports = { // Checks the return statement is valid. ReturnStatement(node) { + if (funcInfo.shouldCheck) { funcInfo.hasReturn = true; + let messageId = null; + // if allowImplicit: false, should also check node.argument - if (!options.allowImplicit && !node.argument) { + if (!options.allowImplicit && !node.argument && funcInfo.arrayMethodName !== "forEach") { + messageId = "expectedReturnValue"; + } else { + + // if checkForEach: returning a value at any path inside a forEach is not allowed + if (options.checkForEach && funcInfo.arrayMethodName === "forEach" && node.argument) { + messageId = "expectedNoReturnValue"; + } + } + + if (messageId) { context.report({ node, - messageId: "expectedReturnValue", + messageId, data: { name: lodash.upperFirst(astUtils.getFunctionNameWithKind(funcInfo.node)) } diff --git a/tests/lib/rules/array-callback-return.js b/tests/lib/rules/array-callback-return.js index de39efcf6b9..e886eb21d3f 100644 --- a/tests/lib/rules/array-callback-return.js +++ b/tests/lib/rules/array-callback-return.js @@ -16,8 +16,29 @@ const ruleTester = new RuleTester(); const allowImplicitOptions = [{ allowImplicit: true }]; +const checkForEach = [{ checkForEach: true }]; + ruleTester.run("array-callback-return", rule, { valid: [ + "foo.forEach(bar || function(x) { var a=0; })", + "foo.forEach(bar || function(x) { return a; })", + "foo.forEach(function() {return function() { var a = 0;}}())", + "foo.forEach(function(x) { var a=0; })", + "foo.forEach(function(x) {return;})", + "foo.forEach(function(x) { if (a === b) { return;} var a=0; })", + "foo.forEach(function(x) { if (a === b) { return x;} var a=0; })", + "foo.forEach(function(x) { return x; })", + "foo.bar().forEach(function(x) { return; })", + "[\"foo\",\"bar\",\"baz\"].forEach(function(x) { return x; })", + { code: "foo.forEach((x) => { var a=0; })", parserOptions: { ecmaVersion: 6 } }, + { code: "foo.forEach((x) => { if (a === b) { return;} var a=0; })", parserOptions: { ecmaVersion: 6 } }, + + // options: { checkForEach: true } + { code: "foo.forEach(function() {return function() { if (a == b) { return; }}}())", options: checkForEach }, + { code: "foo.forEach(function(x) { var a=0; })", options: checkForEach }, + { code: "foo.forEach(function(x) { if (a === b) { return;} var a=0; })", options: checkForEach }, + { code: "foo.forEach((x) => { var a=0; })", options: checkForEach, parserOptions: { ecmaVersion: 6 } }, + { code: "foo.forEach((x) => { if (a === b) { return;} var a=0; })", options: checkForEach, parserOptions: { ecmaVersion: 6 } }, // options: { allowImplicit: false } "Array.from(x, function() { return true; })", @@ -82,6 +103,15 @@ ruleTester.run("array-callback-return", rule, { { code: "foo.map(function* () {})", parserOptions: { ecmaVersion: 6 } } ], invalid: [ + { code: "foo.forEach(function() {return function() { if (a == b) { return a; }}}())", options: checkForEach, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function" } }] }, + { code: "foo.forEach(bar || function(x) { return; })", options: checkForEach, errors: [{ messageId: "noReturnExpected", data: { name: "function" } }] }, + { code: "foo.forEach(function(x) { if (a == b) {return x;}})", options: checkForEach, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function" } }] }, + { code: "foo.forEach(function(x) { if (a == b) {return undefined;}})", options: checkForEach, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function" } }] }, + { code: "foo.forEach(function(x) { return;})", options: checkForEach, errors: [{ messageId: "noReturnExpected", data: { name: "function" } }] }, + { code: "foo.forEach(function bar(x) { return x;})", options: checkForEach, errors: [{ messageId: "noReturnExpected", data: { name: "function 'bar'" } }, { messageId: "expectedNoReturnValue", data: { name: "Function 'bar'" } }] }, + { code: "foo.bar().forEach(function bar(x) { return x;})", options: checkForEach, errors: [{ messageId: "noReturnExpected", data: { name: "function 'bar'" } }, { messageId: "expectedNoReturnValue", data: { name: "Function 'bar'" } }] }, + { code: "[\"foo\",\"bar\"].forEach(function bar(x) { return x;})", options: checkForEach, errors: [{ messageId: "noReturnExpected", data: { name: "function 'bar'" } }, { messageId: "expectedNoReturnValue", data: { name: "Function 'bar'" } }] }, + { code: "foo.forEach((x) => { return x;})", options: checkForEach, parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "noReturnExpected", data: { name: "arrow function" } }, { messageId: "expectedNoReturnValue", data: { name: "Arrow function" } }] }, { code: "Array.from(x, function() {})", errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, { code: "Array.from(x, function foo() {})", errors: [{ messageId: "expectedInside", data: { name: "function 'foo'" } }] }, { code: "Int32Array.from(x, function() {})", errors: [{ messageId: "expectedInside", data: { name: "function" } }] },