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 Jan 1, 2020
1 parent e707453 commit a8340bd
Show file tree
Hide file tree
Showing 3 changed files with 224 additions and 36 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
121 changes: 86 additions & 35 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,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 }
});
}
}
Expand All @@ -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
Expand All @@ -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.
Expand Down

0 comments on commit a8340bd

Please sign in to comment.