Skip to content

Commit

Permalink
Update: array-callback-return checks Array.forEach (fixes #12551) (#1…
Browse files Browse the repository at this point in the history 
…2646)

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 Feb 4, 2020
1 parent 33efd71 commit 439c833
Show file tree
Hide file tree
Showing 3 changed files with 241 additions and 70 deletions.
65 changes: 61 additions & 4 deletions docs/rules/array-callback-return.md
View file
Expand Up @@ -10,10 +10,11 @@ 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.

## Rule Details

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.

This rule finds callback functions of the following methods, then checks usage of `return` statement.

* [`Array.from`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.from)
Expand All @@ -22,6 +23,7 @@ This rule finds callback functions of the following methods, then checks usage o
* [`Array.prototype.find`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.find)
* [`Array.prototype.findIndex`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.findindex)
* [`Array.prototype.flatMap`](https://www.ecma-international.org/ecma-262/10.0/#sec-array.prototype.flatmap)
* [`Array.prototype.forEach`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.foreach) (optional, based on `checkForEach` parameter)
* [`Array.prototype.map`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.map)
* [`Array.prototype.reduce`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.reduce)
* [`Array.prototype.reduceRight`](https://www.ecma-international.org/ecma-262/6.0/#sec-array.prototype.reduceright)
Expand Down Expand Up @@ -75,9 +77,12 @@ var bar = foo.map(node => node.getAttribute("id"));

## Options

This rule has an object option:
This rule accepts a configuration object with two options:

* `"allowImplicit": false` (default) When set to `true`, allows callbacks of methods that require a return value to implicitly return `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.

* `"allowImplicit": false` (default) When set to true, allows implicitly returning `undefined` with a `return` statement containing no expression.
### allowImplicit

Examples of **correct** code for the `{ "allowImplicit": true }` option:

Expand All @@ -88,6 +93,58 @@ var undefAllTheThings = myArray.map(function(item) {
});
```

### checkForEach

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);
});

myArray.forEach(item => handleItem(item));

myArray.forEach(item => {
return 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);
});

myArray.forEach(function(item) {
handleItem(item);
return;
});

myArray.forEach(item => {
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
119 changes: 84 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)?|flatMap|map|reduce(?:Right)?|some|sort)$/u;
const TARGET_METHODS = /^(?:every|filter|find(?:Index)?|flatMap|forEach|map|reduce(?:Right)?|some|sort)$/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 "from";
}
}
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,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,
Expand All @@ -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 }
});
}
}
Expand All @@ -208,14 +233,20 @@ module.exports = {

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

let methodName = null;

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 +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.
Expand Down

0 comments on commit 439c833

Please sign in to comment.