diff --git a/docs/rules/array-callback-return.md b/docs/rules/array-callback-return.md index 34ffeffcebd2..9e4b97d2e71e 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 d632a3f30c28..bffb87cd7795 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. @@ -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. diff --git a/tests/lib/rules/array-callback-return.js b/tests/lib/rules/array-callback-return.js index de39efcf6b9a..459aafcab010 100644 --- a/tests/lib/rules/array-callback-return.js +++ b/tests/lib/rules/array-callback-return.js @@ -16,9 +16,47 @@ 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: { checkForEach: false } + "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) { 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 } }, + + // 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 }, + + // options: { allowImplicit: true, checkForEach: true } + { code: "foo.forEach(function(x) { return; })", options: allowImplicitCheckForEach }, + { code: "foo.forEach(function(x) { var a=0; })", options: allowImplicitCheckForEach }, + { code: "foo.forEach(function(x) { if (a === b) { return;} var a=0; })", options: allowImplicitCheckForEach }, + { code: "foo.forEach(function() {return function() { if (a == b) { return; }}}())", options: allowImplicitCheckForEach }, + { code: "foo.forEach(x => { var a=0; })", options: allowImplicitCheckForEach, parserOptions: { ecmaVersion: 6 } }, + { code: "foo.forEach(x => { if (a === b) { return;} var a=0; })", options: allowImplicitCheckForEach, parserOptions: { ecmaVersion: 6 } }, + { code: "foo.forEach(x => { x })", options: allowImplicitCheckForEach, parserOptions: { ecmaVersion: 6 } }, + { code: "foo.forEach(bar || function(x) { return; })", options: allowImplicitCheckForEach }, + // options: { allowImplicit: false } "Array.from(x, function() { return true; })", "Int32Array.from(x, function() { return true; })", @@ -29,6 +67,10 @@ ruleTester.run("array-callback-return", rule, { { code: "Array.from(x, function() { return; })", options: allowImplicitOptions }, { code: "Int32Array.from(x, function() { return; })", options: allowImplicitOptions }, + // options: { allowImplicit: true, checkForEach: true } + { code: "Array.from(x, function() { return; })", options: allowImplicitCheckForEach }, + { code: "Int32Array.from(x, function() { return; })", options: allowImplicitCheckForEach }, + "Arrow.from(x, function() {})", // options: { allowImplicit: false } @@ -53,6 +95,17 @@ ruleTester.run("array-callback-return", rule, { { code: "foo.some(function() { return; })", options: allowImplicitOptions }, { code: "foo.sort(function() { return; })", options: allowImplicitOptions }, + // options: { allowImplicit: true, checkForEach: true } + { code: "foo.every(function() { return; })", options: allowImplicitCheckForEach }, + { code: "foo.filter(function() { return; })", options: allowImplicitCheckForEach }, + { code: "foo.find(function() { return; })", options: allowImplicitCheckForEach }, + { code: "foo.findIndex(function() { return; })", options: allowImplicitCheckForEach }, + { code: "foo.map(function() { return; })", options: allowImplicitCheckForEach }, + { code: "foo.reduce(function() { return; })", options: allowImplicitCheckForEach }, + { code: "foo.reduceRight(function() { return; })", options: allowImplicitCheckForEach }, + { code: "foo.some(function() { return; })", options: allowImplicitCheckForEach }, + { code: "foo.sort(function() { return; })", options: allowImplicitCheckForEach }, + "foo.abc(function() {})", "every(function() {})", "foo[every](function() {})", @@ -74,6 +127,13 @@ ruleTester.run("array-callback-return", rule, { { code: "foo.every(function() { try { bar(); return; } catch (err) { return; } })", options: allowImplicitOptions }, { code: "foo.every(function() { try { bar(); } finally { return; } })", options: allowImplicitOptions }, + // options: { allowImplicit: true, checkForEach: true } + { code: "foo.every(() => { return; })", options: allowImplicitCheckForEach, parserOptions: { ecmaVersion: 6 } }, + { code: "foo.every(function() { if (a) return; else return a; })", options: allowImplicitCheckForEach }, + { code: "foo.every(function() { switch (a) { case 0: bar(); default: return; } })", options: allowImplicitCheckForEach }, + { code: "foo.every(function() { try { bar(); return; } catch (err) { return; } })", options: allowImplicitCheckForEach }, + { code: "foo.every(function() { try { bar(); } finally { return; } })", options: allowImplicitCheckForEach }, + "foo.every(function(){}())", "foo.every(function(){ return function() { return true; }; }())", "foo.every(function(){ return function() { return; }; })", @@ -82,6 +142,49 @@ ruleTester.run("array-callback-return", rule, { { code: "foo.map(function* () {})", parserOptions: { ecmaVersion: 6 } } ], invalid: [ + + // options: { allowImplicit: false, checkForEach: true } + { code: "foo.forEach(x => x)", 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.find(function foo() {})", options: checkForEachOptions, errors: [{ messageId: "expectedInside", data: { name: "function 'foo'" } }] }, + { code: "foo.map(function() {})", options: checkForEachOptions, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.reduce(function() {})", options: checkForEachOptions, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.reduceRight(function() {})", options: checkForEachOptions, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.bar.baz.every(function foo() {})", options: checkForEachOptions, errors: [{ messageId: "expectedInside", data: { name: "function 'foo'" } }] }, + { code: "foo.every(cb || function() {})", options: checkForEachOptions, errors: ["Expected to return a value in function."] }, + + // options: { allowImplicit: true, checkForEach: true } + { code: "foo.forEach(x => x)", options: allowImplicitCheckForEach, parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Arrow function" } }] }, + { code: "foo.forEach(function() {return function() { if (a == b) { return a; }}}())", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function" } }] }, + { code: "foo.forEach(function(x) { if (a == b) {return x;}})", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function" } }] }, + { code: "foo.forEach(function(x) { if (a == b) {return undefined;}})", 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'" } }] }, + { code: "foo.bar().forEach(function bar(x) { return x;})", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function 'bar'" } }] }, + { code: "[\"foo\",\"bar\"].forEach(function bar(x) { return x;})", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Function 'bar'" } }] }, + { code: "foo.forEach((x) => { return x;})", options: allowImplicitCheckForEach, parserOptions: { ecmaVersion: 6 }, errors: [{ messageId: "expectedNoReturnValue", data: { name: "Arrow function" } }] }, + { code: "Array.from(x, function() {})", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.every(function() {})", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.filter(function foo() {})", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedInside", data: { name: "function 'foo'" } }] }, + { code: "foo.find(function foo() {})", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedInside", data: { name: "function 'foo'" } }] }, + { code: "foo.map(function() {})", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.reduce(function() {})", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.reduceRight(function() {})", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedInside", data: { name: "function" } }] }, + { code: "foo.bar.baz.every(function foo() {})", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedInside", data: { name: "function 'foo'" } }] }, + { code: "foo.every(cb || function() {})", options: allowImplicitCheckForEach, errors: ["Expected to return a value in function."] }, + { code: "[\"foo\",\"bar\"].sort(function foo() {})", options: allowImplicitCheckForEach, errors: [{ messageId: "expectedInside", data: { name: "function 'foo'" } }] }, + + // options: { allowImplicit: false, checkForEach: false } { 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" } }] },