From ed33f8e476ad509c2bf8b1ba0fdcedd0064dcd58 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 | 120 +++++++++++++++++++++-- lib/rules/array-callback-return.js | 119 +++++++++++++++------- tests/lib/rules/array-callback-return.js | 102 +++++++++++++++++++ 3 files changed, 300 insertions(+), 41 deletions(-) diff --git a/docs/rules/array-callback-return.md b/docs/rules/array-callback-return.md index 34ffeffcebd2..0ab63f1b4c6e 100644 --- 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 index d632a3f30c28..bbc4c08321fd 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,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; + + 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 de39efcf6b9a..00659ea66d80 100644 --- a/tests/lib/rules/array-callback-return.js +++ b/tests/lib/rules/array-callback-return.js @@ -16,8 +16,45 @@ 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: [ + "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 } }, + + // 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; })", @@ -29,6 +66,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 +94,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 +126,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 +141,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" } }] },