diff --git a/docs/rules/array-callback-return.md b/docs/rules/array-callback-return.md index 44dccdefff8..c0a5ad8ab35 100644 --- a/docs/rules/array-callback-return.md +++ b/docs/rules/array-callback-return.md @@ -10,10 +10,11 @@ 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. - ## Rule Details +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. + This rule finds callback functions of the following methods, then checks usage of `return` statement. * [`Array.from`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.from) @@ -22,6 +23,7 @@ This rule finds callback functions of the following methods, then checks usage o * [`Array.prototype.find`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.find) * [`Array.prototype.findIndex`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.findindex) * [`Array.prototype.flatMap`](https://www.ecma-international.org/ecma-262/10.0/#sec-array.prototype.flatmap) +* [`Array.prototype.forEach`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.foreach) (optional, based on `checkForEach` parameter) * [`Array.prototype.map`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.map) * [`Array.prototype.reduce`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.reduce) * [`Array.prototype.reduceRight`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.reduceright) @@ -75,9 +77,12 @@ var bar = foo.map(node => node.getAttribute("id")); ## Options -This rule has an object option: +This rule accepts a configuration object with two options: + +* `"allowImplicit": false` (default) When set to `true`, allows callbacks of methods that require a return value to implicitly return `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. -* `"allowImplicit": false` (default) When set to true, allows implicitly returning `undefined` with a `return` statement containing no expression. +### allowImplicit Examples of **correct** code for the `{ "allowImplicit": true }` option: @@ -88,6 +93,58 @@ var undefAllTheThings = myArray.map(function(item) { }); ``` +### checkForEach + +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); +}); + +myArray.forEach(item => handleItem(item)); + +myArray.forEach(item => { + return 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); +}); + +myArray.forEach(function(item) { + handleItem(item); + return; +}); + +myArray.forEach(item => { + 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 6e425dcdcb8..eb38965024f 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)?|flatMap|map|reduce(?:Right)?|some|sort)$/u; +const TARGET_METHODS = /^(?:every|filter|find(?:Index)?|flatMap|forEach|map|reduce(?:Right)?|some|sort)$/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 "from"; + } } 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,15 +171,17 @@ 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." } }, 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, @@ -188,18 +199,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 +233,20 @@ module.exports = { // Stacks this function's information. onCodePathStart(codePath, node) { + + let methodName = null; + + 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 +260,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. diff --git a/tests/lib/rules/array-callback-return.js b/tests/lib/rules/array-callback-return.js index 2816d796246..71731de5879 100644 --- a/tests/lib/rules/array-callback-return.js +++ b/tests/lib/rules/array-callback-return.js @@ -16,74 +16,105 @@ const ruleTester = new RuleTester(); const allowImplicitOptions = [{ allowImplicit: true }]; +const checkForEachOptions = [{ checkForEach: true }]; + +const allowImplicitCheckForEach = [{ allowImplicit: true, checkForEach: true }]; + ruleTester.run("array-callback-return", rule, { valid: [ - // options: { allowImplicit: false } - "Array.from(x, function() { return true; })", - "Int32Array.from(x, function() { return true; })", - { code: "Array.from(x, function() { return true; })", options: [{ allowImplicit: false }] }, - { code: "Int32Array.from(x, function() { return true; })", options: [{ allowImplicit: false }] }, + "foo.every(function(){}())", + "foo.every(function(){ return function() { return true; }; }())", + "foo.every(function(){ return function() { return; }; })", - // options: { allowImplicit: true } - { code: "Array.from(x, function() { return; })", options: allowImplicitOptions }, - { code: "Int32Array.from(x, function() { return; })", options: allowImplicitOptions }, + "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 a;})", + "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.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 } }, + { code: "foo.forEach(x => x)", parserOptions: { ecmaVersion: 6 } }, + { code: "foo.forEach(val => y += val)", parserOptions: { ecmaVersion: 6 } }, - "Arrow.from(x, function() {})", + { code: "foo.map(async function(){})", parserOptions: { ecmaVersion: 8 } }, + { code: "foo.map(async () => {})", parserOptions: { ecmaVersion: 8 } }, + { code: "foo.map(function* () {})", parserOptions: { ecmaVersion: 6 } }, // options: { allowImplicit: false } + { code: "Array.from(x, function() { return true; })", options: [{ allowImplicit: false }] }, + { code: "Int32Array.from(x, function() { return true; })", options: [{ allowImplicit: false }] }, "foo.every(function() { return true; })", "foo.filter(function() { return true; })", "foo.find(function() { return true; })", "foo.findIndex(function() { return true; })", "foo.flatMap(function() { return true; })", + "foo.forEach(function() { return; })", "foo.map(function() { return true; })", "foo.reduce(function() { return true; })", "foo.reduceRight(function() { return true; })", "foo.some(function() { return true; })", "foo.sort(function() { return 0; })", + { code: "foo.every(() => { return true; })", parserOptions: { ecmaVersion: 6 } }, + "foo.every(function() { if (a) return true; else return false; })", + "foo.every(function() { switch (a) { case 0: bar(); default: return true; } })", + "foo.every(function() { try { bar(); return true; } catch (err) { return false; } })", + "foo.every(function() { try { bar(); } finally { return true; } })", // options: { allowImplicit: true } + { code: "Array.from(x, function() { return; })", options: allowImplicitOptions }, + { code: "Int32Array.from(x, function() { return; })", options: allowImplicitOptions }, { code: "foo.every(function() { return; })", options: allowImplicitOptions }, { code: "foo.filter(function() { return; })", options: allowImplicitOptions }, { code: "foo.find(function() { return; })", options: allowImplicitOptions }, { code: "foo.findIndex(function() { return; })", options: allowImplicitOptions }, { code: "foo.flatMap(function() { return; })", options: allowImplicitOptions }, + { code: "foo.forEach(function() { return; })", options: allowImplicitOptions }, { code: "foo.map(function() { return; })", options: allowImplicitOptions }, { code: "foo.reduce(function() { return; })", options: allowImplicitOptions }, { code: "foo.reduceRight(function() { return; })", options: allowImplicitOptions }, { code: "foo.some(function() { return; })", options: allowImplicitOptions }, { code: "foo.sort(function() { return; })", options: allowImplicitOptions }, + { code: "foo.every(() => { return; })", options: allowImplicitOptions, parserOptions: { ecmaVersion: 6 } }, + { code: "foo.every(function() { if (a) return; else return a; })", options: allowImplicitOptions }, + { code: "foo.every(function() { switch (a) { case 0: bar(); default: return; } })", options: allowImplicitOptions }, + { code: "foo.every(function() { try { bar(); return; } catch (err) { return; } })", options: allowImplicitOptions }, + { code: "foo.every(function() { try { bar(); } finally { return; } })", options: allowImplicitOptions }, + + // options: { checkForEach: true } + { code: "foo.forEach(function(x) { return; })", options: checkForEachOptions }, + { code: "foo.forEach(function(x) { var a=0; })", options: checkForEachOptions }, + { code: "foo.forEach(function(x) { if (a === b) { return;} var a=0; })", options: checkForEachOptions }, + { code: "foo.forEach(function() {return function() { if (a == b) { return; }}}())", options: checkForEachOptions }, + { code: "foo.forEach(x => { var a=0; })", options: checkForEachOptions, parserOptions: { ecmaVersion: 6 } }, + { code: "foo.forEach(x => { if (a === b) { return;} var a=0; })", options: checkForEachOptions, parserOptions: { ecmaVersion: 6 } }, + { code: "foo.forEach(x => { x })", options: checkForEachOptions, parserOptions: { ecmaVersion: 6 } }, + { code: "foo.forEach(bar || function(x) { return; })", options: checkForEachOptions }, + { code: "Array.from(x, function() { return true; })", options: checkForEachOptions }, + { code: "Int32Array.from(x, function() { return true; })", options: checkForEachOptions }, + { code: "foo.every(() => { return true; })", options: checkForEachOptions, parserOptions: { ecmaVersion: 6 } }, + { code: "foo.every(function() { if (a) return 1; else return a; })", options: checkForEachOptions }, + { code: "foo.every(function() { switch (a) { case 0: return bar(); default: return a; } })", options: checkForEachOptions }, + { code: "foo.every(function() { try { bar(); return 1; } catch (err) { return err; } })", options: checkForEachOptions }, + { code: "foo.every(function() { try { bar(); } finally { return 1; } })", options: checkForEachOptions }, + { code: "foo.every(function() { return; })", options: allowImplicitCheckForEach }, + "Arrow.from(x, function() {})", "foo.abc(function() {})", "every(function() {})", "foo[every](function() {})", "var every = function() {}", { code: "foo[`${every}`](function() {})", parserOptions: { ecmaVersion: 6 } }, - { code: "foo.every(() => true)", parserOptions: { ecmaVersion: 6 } }, - - // options: { allowImplicit: false } - { code: "foo.every(() => { return true; })", parserOptions: { ecmaVersion: 6 } }, - "foo.every(function() { if (a) return true; else return false; })", - "foo.every(function() { switch (a) { case 0: bar(); default: return true; } })", - "foo.every(function() { try { bar(); return true; } catch (err) { return false; } })", - "foo.every(function() { try { bar(); } finally { return true; } })", + { code: "foo.every(() => true)", parserOptions: { ecmaVersion: 6 } } - // options: { allowImplicit: true } - { code: "foo.every(() => { return; })", options: allowImplicitOptions, parserOptions: { ecmaVersion: 6 } }, - { code: "foo.every(function() { if (a) return; else return a; })", options: allowImplicitOptions }, - { code: "foo.every(function() { switch (a) { case 0: bar(); default: return; } })", options: allowImplicitOptions }, - { code: "foo.every(function() { try { bar(); return; } catch (err) { return; } })", options: allowImplicitOptions }, - { code: "foo.every(function() { try { bar(); } finally { return; } })", options: allowImplicitOptions }, - - "foo.every(function(){}())", - "foo.every(function(){ return function() { return true; }; }())", - "foo.every(function(){ return function() { return; }; })", - { code: "foo.map(async function(){})", parserOptions: { ecmaVersion: 8 } }, - { code: "foo.map(async () => {})", parserOptions: { ecmaVersion: 8 } }, - { code: "foo.map(function* () {})", parserOptions: { ecmaVersion: 6 } } ], invalid: [ + { 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" } }] }, @@ -134,6 +165,40 @@ ruleTester.run("array-callback-return", rule, { { code: "foo.every(function(){ return function() {}; }())", errors: [{ message: "Expected to return a value in function.", column: 30 }] }, { code: "foo.every(function(){ return function foo() {}; }())", errors: [{ message: "Expected to return a value in function 'foo'.", column: 39 }] }, { code: "foo.every(() => {})", options: [{ allowImplicit: false }], parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Expected to return a value in arrow function." }] }, - { code: "foo.every(() => {})", options: [{ allowImplicit: true }], parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Expected to return a value in arrow function." }] } + { code: "foo.every(() => {})", options: [{ allowImplicit: true }], parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Expected to return a value in arrow function." }] }, + + // options: { allowImplicit: true } + { code: "Array.from(x, function() {})", options: allowImplicitOptions, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.every(function() {})", options: allowImplicitOptions, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.filter(function foo() {})", options: allowImplicitOptions, errors: [{ messageId: "expectedInside", data: { name: "function 'foo'" } }] }, + { code: "foo.find(function foo() {})", options: allowImplicitOptions, errors: [{ messageId: "expectedInside", data: { name: "function 'foo'" } }] }, + { code: "foo.map(function() {})", options: allowImplicitOptions, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.reduce(function() {})", options: allowImplicitOptions, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.reduceRight(function() {})", options: allowImplicitOptions, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.bar.baz.every(function foo() {})", options: allowImplicitOptions, errors: [{ messageId: "expectedInside", data: { name: "function 'foo'" } }] }, + { code: "foo.every(cb || function() {})", options: allowImplicitOptions, errors: ["Expected to return a value in function."] }, + { code: "[\"foo\",\"bar\"].sort(function foo() {})", options: allowImplicitOptions, errors: [{ messageId: "expectedInside", data: { name: "function 'foo'" } }] }, + { code: "foo.forEach(x => x)", options: allowImplicitCheckForEach, parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Arrow function" } }] }, + { code: "foo.forEach(function(x) { if (a == b) {return x;}})", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function" } }] }, + { code: "foo.forEach(function bar(x) { return x;})", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function 'bar'" } }] }, + + // // options: { checkForEach: true } + { code: "foo.forEach(x => x)", options: checkForEachOptions, parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Arrow function" } }] }, + { code: "foo.forEach(val => y += val)", options: checkForEachOptions, parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Arrow function" } }] }, + { code: "[\"foo\",\"bar\"].forEach(x => ++x)", options: checkForEachOptions, parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Arrow function" } }] }, + { code: "foo.bar().forEach(x => x === y)", options: checkForEachOptions, parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Arrow function" } }] }, + { code: "foo.forEach(function() {return function() { if (a == b) { return a; }}}())", options: checkForEachOptions, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function" } }] }, + { code: "foo.forEach(function(x) { if (a == b) {return x;}})", options: checkForEachOptions, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function" } }] }, + { code: "foo.forEach(function(x) { if (a == b) {return undefined;}})", options: checkForEachOptions, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function" } }] }, + { code: "foo.forEach(function bar(x) { return x;})", options: checkForEachOptions, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function 'bar'" } }] }, + { code: "foo.bar().forEach(function bar(x) { return x;})", options: checkForEachOptions, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function 'bar'" } }] }, + { code: "[\"foo\",\"bar\"].forEach(function bar(x) { return x;})", options: checkForEachOptions, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function 'bar'" } }] }, + { code: "foo.forEach((x) => { return x;})", options: checkForEachOptions, parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Arrow function" } }] }, + { code: "Array.from(x, function() {})", options: checkForEachOptions, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.every(function() {})", options: checkForEachOptions, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.filter(function foo() {})", options: checkForEachOptions, errors: [{ messageId: "expectedInside", data: { name: "function 'foo'" } }] }, + { code: "foo.filter(function foo() { return; })", options: checkForEachOptions, errors: [{ messageId: "expectedReturnValue", data: { name: "Function 'foo'" } }] }, + { code: "foo.every(cb || function() {})", options: checkForEachOptions, errors: ["Expected to return a value in function."] } + ] });