Skip to content

Commit

Permalink
Update: callback-return allows for object methods (fixes #4711) (#6277)
Browse files Browse the repository at this point in the history
  • Loading branch information
@kaicataldo @nzakas
kaicataldo authored and nzakas committed Jun 3, 2016
1 parent 89580a4 commit 72c2ea5
Show file tree
Hide file tree
Showing 3 changed files with 224 additions and 11 deletions.
56 changes: 50 additions & 6 deletions docs/rules/callback-return.md
View file
Expand Up @@ -24,14 +24,16 @@ preceding a `return` statement. This rule decides what is a callback based on th

## Options

The rule takes a single option, which is an array of possible callback names. The default callback names are `callback`, `cb`, `next`.
The rule takes a single option - an array of possible callback names - which may include object methods. The default callback names are `callback`, `cb`, `next`.

### Default callback names

Examples of **incorrect** code for this rule with the default `["callback", "cb", "next"]` option:

```js
/*eslint callback-return: "error"*/

function foo() {
function foo(err, callback) {
if (err) {
callback(err);
}
Expand All @@ -44,14 +46,56 @@ Examples of **correct** code for this rule with the default `["callback", "cb",
```js
/*eslint callback-return: "error"*/

function foo() {
function foo(err, callback) {
if (err) {
return callback(err);
}
callback();
}
```

### Supplied callback names

Examples of **incorrect** code for this rule with the option `["done", "send.error", "send.success"]`:

```js
/*eslint callback-return: ["error", ["done", "send.error", "send.success"]]*/

function foo(err, done) {
if (err) {
done(err);
}
done();
}

function bar(err, send) {
if (err) {
send.error(err);
}
send.success();
}
```

Examples of **correct** code for this rule with the the option `["done", "send.error", "send.success"]`:

```js
/*eslint callback-return: ["error", ["done", "send.error", "send.success"]]*/

function foo(err, done) {
if (err) {
return done(err);
}
done();
}

function bar(err, send) {
if (err) {
return send.error(err);
}
send.success();
}
```

## Known Limitations

Because it is difficult to understand the meaning of a program through static analysis, this rule has limitations:
Expand All @@ -68,7 +112,7 @@ Example of a *false negative* when this rule reports correct code:
```js
/*eslint callback-return: "error"*/

function foo(callback) {
function foo(err, callback) {
if (err) {
setTimeout(callback, 0); // this is bad, but WILL NOT warn
}
Expand All @@ -85,7 +129,7 @@ Example of a *false negative* when this rule reports correct code:
```js
/*eslint callback-return: "error"*/

function foo(callback) {
function foo(err, callback) {
if (err) {
process.nextTick(function() {
return callback(); // this is bad, but WILL NOT warn
Expand All @@ -104,7 +148,7 @@ Example of a *false positive* when this rule reports incorrect code:
```js
/*eslint callback-return: "error"*/

function foo(callback) {
function foo(err, callback) {
if (err) {
callback(err); // this is fine, but WILL warn
} else {
Expand Down
28 changes: 25 additions & 3 deletions lib/rules/callback-return.js
View file
Expand Up @@ -24,7 +24,8 @@ module.exports = {

create: function(context) {

var callbacks = context.options[0] || ["callback", "cb", "next"];
var callbacks = context.options[0] || ["callback", "cb", "next"],
sourceCode = context.getSourceCode();

//--------------------------------------------------------------------------
// Helpers
Expand All @@ -46,13 +47,34 @@ module.exports = {
return node.parent;
}

/**
* Check to see if a node contains only identifers
* @param {ASTNode} node The node to check
* @returns {Boolean} Whether or not the node contains only identifers
*/
function containsOnlyIdentifiers(node) {
if (node.type === "Identifier") {
return true;
}

if (node.type === "MemberExpression") {
if (node.object.type === "Identifier") {
return true;
} else if (node.object.type === "MemberExpression") {
return containsOnlyIdentifiers(node.object);
}
}

return false;
}

/**
* Check to see if a CallExpression is in our callback list.
* @param {ASTNode} node The node to check against our callback names list.
* @returns {Boolean} Whether or not this function matches our callback name.
*/
function isCallback(node) {
return node.callee.type === "Identifier" && callbacks.indexOf(node.callee.name) > -1;
return containsOnlyIdentifiers(node.callee) && callbacks.indexOf(sourceCode.getText(node.callee)) > -1;
}

/**
Expand Down Expand Up @@ -90,7 +112,7 @@ module.exports = {
return {
CallExpression: function(node) {

// if we"re not a callback we can return
// if we're not a callback we can return
if (!isCallback(node)) {
return;
}
Expand Down
151 changes: 149 additions & 2 deletions tests/lib/rules/callback-return.js
View file
Expand Up @@ -21,7 +21,7 @@ var ruleTester = new RuleTester();
ruleTester.run("callback-return", rule, {
valid: [

// callbacks inside of functions should return
// callbacks inside of functions should return
"function a(err) { if (err) return callback (err); }",
"function a(err) { if (err) return callback (err); callback(); }",
"function a(err) { if (err) { return callback (err); } callback(); }",
Expand All @@ -38,6 +38,7 @@ ruleTester.run("callback-return", rule, {
"function x() { switch(x) { case 'a': return next(); } }",
"function x() { for(x = 0; x < 10; x++) { return next(); } }",
"function x() { while(x) { return next(); } }",
"function a(err) { if (err) { obj.method (err); } }",

// callback() all you want outside of a function
"callback()",
Expand Down Expand Up @@ -73,7 +74,7 @@ ruleTester.run("callback-return", rule, {

// options (only warns with the correct callback name)
{
code: "if (err) { callback(err) }",
code: "function a(err) { if (err) { callback(err) } }",
options: [["cb"]]
},
{
Expand All @@ -85,6 +86,68 @@ ruleTester.run("callback-return", rule, {
options: [["cb", "next"]]
},

// allow object methods (https://github.com/eslint/eslint/issues/4711)
{
code: "function a(err) { if (err) { return obj.method(err); } }",
options: [["obj.method"]]
},
{
code: "function a(err) { if (err) { return obj.prop.method(err); } }",
options: [["obj.prop.method"]]
},
{
code: "function a(err) { if (err) { return obj.prop.method(err); } otherObj.prop.method() }",
options: [["obj.prop.method", "otherObj.prop.method"]]
},
{
code: "function a(err) { if (err) { callback(err); } }",
options: [["obj.method"]]
},
{
code: "function a(err) { if (err) { otherObj.method(err); } }",
options: [["obj.method"]]
},
{
code: "function a(err) { if (err) { //comment\nreturn obj.method(err); } }",
options: [["obj.method"]]
},
{
code: "function a(err) { if (err) { /*comment*/return obj.method(err); } }",
options: [["obj.method"]]
},
{
code: "function a(err) { if (err) { return obj.method(err); //comment\n } }",
options: [["obj.method"]]
},
{
code: "function a(err) { if (err) { return obj.method(err); /*comment*/ } }",
options: [["obj.method"]]
},

// only warns if object of MemberExpression is an Identifier
{
code: "function a(err) { if (err) { obj().method(err); } }",
options: [["obj().method"]]
},
{
code: "function a(err) { if (err) { obj.prop().method(err); } }",
options: [["obj.prop().method"]]
},
{
code: "function a(err) { if (err) { obj().prop.method(err); } }",
options: [["obj().prop.method"]]
},

// does not warn if object of MemberExpression is invoked
{
code: "function a(err) { if (err) { obj().method(err); } }",
options: [["obj.method"]]
},
{
code: "function a(err) { if (err) { obj().method(err); } obj.method(); }",
options: [["obj.method"]]
},

// known bad examples that we know we are ignoring
"function x(err) { if (err) { setTimeout(callback, 0); } callback(); }", // callback() called twice
"function x(err) { if (err) { process.nextTick(function(err) { callback(); }); } callback(); }" // callback() called twice
Expand Down Expand Up @@ -332,6 +395,90 @@ ruleTester.run("callback-return", rule, {
nodeType: "CallExpression"
}
]
},
{
code: "function a(err) { if (err) { obj.method(err); } }",
options: [["obj.method"]],
errors: [
{
message: "Expected return with your callback function.",
line: 1,
column: 30,
nodeType: "CallExpression"
}
]
},
{
code: "function a(err) { if (err) { obj.prop.method(err); } }",
options: [["obj.prop.method"]],
errors: [
{
message: "Expected return with your callback function.",
line: 1,
column: 30,
nodeType: "CallExpression"
}
]
},
{
code: "function a(err) { if (err) { obj.prop.method(err); } otherObj.prop.method() }",
options: [["obj.prop.method", "otherObj.prop.method"]],
errors: [
{
message: "Expected return with your callback function.",
line: 1,
column: 30,
nodeType: "CallExpression"
}
]
},
{
code: "function a(err) { if (err) { /*comment*/obj.method(err); } }",
options: [["obj.method"]],
errors: [
{
message: "Expected return with your callback function.",
line: 1,
column: 41,
nodeType: "CallExpression"
}
]
},
{
code: "function a(err) { if (err) { //comment\nobj.method(err); } }",
options: [["obj.method"]],
errors: [
{
message: "Expected return with your callback function.",
line: 2,
column: 1,
nodeType: "CallExpression"
}
]
},
{
code: "function a(err) { if (err) { obj.method(err); /*comment*/ } }",
options: [["obj.method"]],
errors: [
{
message: "Expected return with your callback function.",
line: 1,
column: 30,
nodeType: "CallExpression"
}
]
},
{
code: "function a(err) { if (err) { obj.method(err); //comment\n } }",
options: [["obj.method"]],
errors: [
{
message: "Expected return with your callback function.",
line: 1,
column: 30,
nodeType: "CallExpression"
}
]
}
]
});

0 comments on commit 72c2ea5

Please sign in to comment.