Skip to content

Commit

Permalink
Update: array-callback-return checks Array.forEach (fixes eslint#12551)
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@gabrieldrs
gabrieldrs committed Dec 9, 2019
1 parent e707453 commit fd5023e
Show file tree
Hide file tree
Showing 3 changed files with 141 additions and 12 deletions.
36 changes: 35 additions & 1 deletion docs/rules/array-callback-return.md
View file
Expand Up @@ -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.

Expand All @@ -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.
Expand Down
87 changes: 76 additions & 11 deletions lib/rules/array-callback-return.js
View file
Expand Up @@ -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.
Expand Down Expand Up @@ -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
//------------------------------------------------------------------------------
Expand All @@ -153,6 +180,10 @@ module.exports = {
allowImplicit: {
type: "boolean",
default: false
},
checkForEach: {
type: "boolean",
default: false
}
},
additionalProperties: false
Expand All @@ -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
};
Expand All @@ -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)
}
Expand All @@ -220,6 +268,10 @@ module.exports = {
!node.generator,
node
};

if (funcInfo.shouldCheck) {
funcInfo.arrayMethodName = getArrayMethodName(node);
}
},

// Pops this function's information.
Expand All @@ -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))
}
Expand Down
30 changes: 30 additions & 0 deletions tests/lib/rules/array-callback-return.js
View file
Expand Up @@ -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; })",
Expand Down Expand Up @@ -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" } }] },
Expand Down

0 comments on commit fd5023e

Please sign in to comment.