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 20, 2019
1 parent e707453 commit 7e6c585
Show file tree
Hide file tree
Showing 3 changed files with 138 additions and 27 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
99 changes: 73 additions & 26 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 @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}

//------------------------------------------------------------------------------
Expand All @@ -153,6 +158,10 @@ module.exports = {
allowImplicit: {
type: "boolean",
default: false
},
checkForEach: {
type: "boolean",
default: false
}
},
additionalProperties: false
Expand All @@ -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
};
Expand All @@ -188,15 +201,27 @@ 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 @@ -208,14 +233,20 @@ module.exports = {

// Stacks this function's information.
onCodePathStart(codePath, node) {

let methodName;

if (TARGET_NODE_TYPE.test(node.type) && node.body.type === "BlockStatement") {
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
Expand All @@ -229,14 +260,30 @@ module.exports = {

// Checks the return statement is valid.
ReturnStatement(node) {

if (funcInfo.shouldCheck) {
funcInfo.hasReturn = true;

// if allowImplicit: false, should also check node.argument
if (!options.allowImplicit && !node.argument) {
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) {
messageId = "expectedReturnValue";
}
}

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 7e6c585

Please sign in to comment.