Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rule: async callback should be paired with return #994

Closed
puzrin opened this issue Jun 19, 2014 · 46 comments

Comments

Projects
None yet
9 participants
@puzrin
Copy link
Contributor

commented Jun 19, 2014

In node.js typical error is writing something like this:

if (err) {
  callback(err);
}

Right cases are:

if (err) {
  callback(err);
  return;
}
if (err) return callback (err);

Imho, this can be automated. Preliminary conditions to detect callback

  • keywords callback, cb, next in function name
  • 0-2 params

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@nzakas

This comment has been minimized.

Copy link
Member

commented Jun 19, 2014

I like it. I think we probably need to make the callback names configurable in case we miss any.

@jrajav

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2014

It seems like checking the condition variable for a variation of 'err' or 'error' would be more reliable than checking the callback name. There are tons of different naming conventions for the callback name, but the condition is rarely something besides 'e', 'err', or 'error'.

@puzrin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2014

Good point, but condition with "error" does not guarantee, that function in it is async callback. That can be just sync code with error check and calling reportef fn, for example.

Probably, we could combine both methods: tuneable callback names + err name check.

@jrajav

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2014

Whether it's async or not isn't it still reasonable to enforce a return in that block?

@puzrin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2014

Yes. In node.js "async" code doing callback without return is a bug - your code logic will become completely broken. And it's very time-costy to find such mistake.

PS. There can be terminology confusion. This case is not about sync iterators (lambdas), which sometime also named "callbacks".

@neonstalwart

This comment has been minimized.

Copy link

commented Jun 19, 2014

should it also be right if there is an else?

if (err) {
  callback(err);
}
else {
  // no error...
}
@jrajav

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2014

Yes. In node.js "async" code doing callback without return is a bug - your code logic will become completely broken. And it's very time-costy to find such mistake.

Yeah I agree with that, that wasn't the point of my question. I was saying that if you have a block like this in a synchronous function:

if (err) {
    syncReportingFunction(err);
}

It's probably still a bug to not return at that point. You have an error condition that you're checking and it's true.

I think it might be better to generalize the rule to be this: If you are in a function and checking a conditional (with no else clause, good point @neonstalwart) that is just a truthiness check (maybe including things like != null) of a symbol with the format 'err' or 'error' ('e' might be too broad, though I have seen that used in many places), ensure that there is a return statement in the if-block. Perhaps it could also be a requirement that the conditional be the first non-declaration statement.

@puzrin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2014

@neonstalwart that will work, if there are no code to execute after callback.

But in real life, your snippet is example of horrible async code writing.

  • you will do mistake with very high probability, if use it everywhere.
  • it additionally increases code nesting level, that is already high because of nested callback lambdas functions.
@neonstalwart

This comment has been minimized.

Copy link

commented Jun 19, 2014

@jrajav if there is a truthiness check for the first argument of a function call that results in calling the 2nd argument of the function call then could this situation be implied without any concern for the names?

@nzakas

This comment has been minimized.

Copy link
Member

commented Jun 19, 2014

There is no safe way to discern that a given function call is a callback or
not without some type of hint. For example:

function doSomething(callback) {
    doSomethingElse(function(err, data) {
        if (err) {
            callback(err);
        }

        // more code
    });
}

You can't simply go by the last argument of the containing function because
a callback can be passed into a function expression and used there.

I can't think of any way other than looking for specific function names to
disambiguate.

@jrajav

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2014

if there is a truthiness check for the first argument of a function call that results in calling the 2nd argument of the function call then could this situation be implied without any concern for the names?

That seems like way too broad of a condition to me. You could have any number of functions that look like this that aren't checking error conditions.

I don't think we can avoid having to check for at least the 'err' or 'error' symbol. I also still don't see why we have any reason to care what the block is doing, callback function or no, aside from whether it has a return statement.

Also @puzrin it is true that relying on branching has a higher error rate, however some people do it because they prefer to never return before the last statement of a function. It does make some patterns like this one and try-catch blocks more complicated, but the argument is that the flow is easier to follow and you only have one place to look for what a function returns.

@puzrin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2014

It does make some patterns like this one and try-catch blocks more complicated, but the argument is that the flow is easier to follow and you only have one place to look for what a function returns.

BTW, about try-catch. Making callback-return from try-catch sections can bring you tons of lulz with unit tests if some called helper code will throw exception. After i've lost 2 days, trying to understand why "tests are called twice and return strange results", coding style of my projects strictly prohibits to do callbacks from try-catch.

@nzakas

This comment has been minimized.

Copy link
Member

commented Jun 19, 2014

Okay everyone, let's bring the conversation back to the point of the issue.

I'm fine with a rule requiring return after a callback is called so long as we are looking at specific function names. We'll have the rule off by default. If someone wants to put together a PR, we can take it from there.

@jrajav

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2014

I think it should be based on the conditional and the return statement, not the callback at all. There are lots of times you would be using callbacks that don't need a return after them (imagine a kind of mapper or other functional construct rather than an async 'next' or 'done' control yielder), which would make this rule hard to use if that's all it checked. I also think there's too much variance in callback names (versus very little in the names of error arguments), and ones as generic as 'callback' don't necessarily need to be the last thing called in a function.

@puzrin

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2014

@jrajav error is not mandatory attribute. You can just write linear logic with multiple if, checking different conditions and terminating with callback if satisfied.

If your project has good coding style, why to limit checks with errors only?

@nzakas

This comment has been minimized.

Copy link
Member

commented Jun 19, 2014

Let's hold off on more feedback until a prototype is built.​

@jrajav

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2014

I can write a prototype for this rule, but I just want to make sure I'm not gonna waste my time here. I thought about it a little more and just checking for an if (err) conditional might not be enough - there might be cases where you want to log an error but still continue. I also still think checking for a callback name is too brittle, those vary much more than err or error. So these are the steps that I'm proposing, which I'll make a rule for if they seem alright (at least as a starting point):

  1. If an if conditional is encountered whose condition contains an identifier matching 'e', 'err', or 'error', that isn't modified by a ! operator...
  2. Check if that identifier is a parameter of the first containing function, and if so...
  3. Check if the conditional's body contains a function call on an identifier that is also a parameter of the first containing function, and if so...
  4. Assert that the conditional's body also contains a return or a throw as the last statement.
@puzrin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2014

That will not check this type o "linear" style:

if (cond1) {
  callback(null, result1);
  return;
}

if (cond2) {
  callback(null, result2);
  return;
}
...

I still think, that check of configurable callback name will be more simple and good enougth.

@jrajav

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2014

@puzrin To me that seems way outside the scope of what we can reliably check. You're talking about generic conditions now and not just errors? Many different code patterns will throw false positives for that, even if you specify a callback name (again, you can have functions that use 'callback' but aren't a simple continuation).

Also, I missed a part of the algorithm I was considering above and might make more sense: Only check the conditional if it's the first non-declaration statement in the function.

@puzrin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 6, 2014

Yes, i was speaking abouc generic condition. IMHO, situation depends on coding style. For example, my one says, that async callbacks should have name callback, cb or next, and iterators should never have such names. I'm interested to cover those names with check as max as possible.

I think, it's impossible to make generic algorythm, that will be good for all. And we finally will fall into custom rules like callback-return-jrajav, callback-return-puzrin, callback-return-nzakas and so on :)

@jrajav

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2014

Maybe it makes sense to split these off so that our discussion can be more fruitful. I think there are two potential rules here:

  1. A rule that covers generic (and configurable) callback names, enforcing return statements if they are called as functions inside of a block.
  2. The error-condition-checking rule I've proposed just above.

I personally don't agree that 1. should even exist, but that's probably closer to the intent of this issue as you opened it @puzrin. I will open a new issue for the error callback.

@nzakas

This comment has been minimized.

Copy link
Member

commented Jul 6, 2014

I think you're unnecessarily conflating error checking with the real purpose of this rule, which is simply to ensure that a return statement follows a callback inside of a conditional branch. We already have a rule to ensure errors are handled (http://eslint.org/docs/rules/handle-callback-err.html).

The algorithm for this rule is simple:

  1. Look for a function call
  2. if the function name is a specified callback name then
  3. Make sure it's not the last child of its parent (unless the parent is a function)
@jrajav

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2014

I understand that, I'm just disagreeing with that rule - I think that it's too generic to be useful. I also think it's brittle and complex to have a configurable list of callback names.

Also, the rule I'm proposing is not the same as handle-callback-err. It's saying that if you're already handling an error, ensure that it results in exiting the function. Maybe that's not very useful either, I just wanted to split that discussion off.

@nzakas

This comment has been minimized.

Copy link
Member

commented Jul 6, 2014

You can feel free to disagree, I think the rule as I described it is very useful and that's the one I want to pursue. Coupling that behavior with error-checking unnecessarily limits a useful check to just one part of a function. You seem to be assuming that callbacks are only ever called two times in a function, (once fir error, once for success). Callbacks can be called at any point in time in a function, any number of times, and every time it should be followed with a return unless the function exits naturally. This rule will catch all occurrences (including the error case).

@jrajav

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2014

Callbacks can be called at any point in time in a function, any number of times, and every time it should be followed with a return unless the function exits naturally.

Just to be clear, this is the reason I'm saying I think it's too generic - you can have functional patterns that do not necessitate a return after a callback. Two examples: A leaf-last walking pattern where a callback is passed in to be called on each node:

function walk(callback, ...) {
    ...
    if (node !== null) {
        callback(node);
    }
    ...
    return walk(node.child);
}

Or a function that is passed in but has nothing to do with continuation:

function (logger) {
    ...
    if (instance) {
        logger(instance);
    }
    ...
}

Assigning names to the callbacks would mitigate the latter case but that seems brittle to me.

That's just to clarify the concern I am expressing - I won't argue anymore since it's clear it's just a disagreement.

@nzakas

This comment has been minimized.

Copy link
Member

commented Jul 7, 2014

I think we give people some good outs if they want to use one of those patterns: just use a different function name (just like your second example).

@jrajav

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2014

It fits in the second example, but in the first one 'callback' or 'cb' is the most sensible name (something like "walkCallback" would be redundant). To me it doesn't make sense to have to change it just to not violate a rule that's intended to guard against an entirely different pattern.

Of course you could just disable the rule if this happened in practice.

@xjamundx

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2015

I've gotten to the place where these are all correctly handled (per my understanding)

-"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(); }",
- X "function a(err) { if (err) { callback (err); } }"

Let's talk through several more examples to make sure I understand the expected behavior:

// correct and good behavior
-"function x(err) { if (err) { callback(); return; } }"
-"function x(err) { if (err) { callback(); return; } return callback(); }"
-"function x(err) { if (err) { return callback(); } else { return callback(); } }"
-"function x(err) { if (err) return callback(); else return callback(); }"

// known bad behavior which we warn about
- X "function x(err) { if (err) { return callback(); }  else if (abc) { callback(); } else { return callback(); } }"

// known bad behavior which we ignore to simplify the rule
-"function x(err) { if (err) { setTimeout(callback, 0); } callback(); }" // callback() called twice
-"function x(err) { if (err) { callback(); callback(); return;  }" // callback() called twice with `return`

// good behavior that we warn against to simplify the rule
- X "function x(err) { if (err) { callback() } else { callback() } }"
- X "function x(err) { if (err) return callback(); else callback(); }"
@nzakas

This comment has been minimized.

Copy link
Member

commented Jun 19, 2015

Looks good!

@gcochard

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2015

👍 I strongly support this rule!

What about the following:

I foresee this when we want to force the callback to be async so we don't release zalgo. We are pairing the callback with return, but the first is inside of process.nextTick so it will be called twice, and should be handled.

function x(err) {
  if(err){
    process.nextTick(function(){
      return callback(err);
    });
  }
  return callback();
}
@nzakas

This comment has been minimized.

Copy link
Member

commented Jun 20, 2015

Are you asking if the rule would warn about this case? I'm not really sure from your description.

@gcochard

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2015

Yes, I would like the rule to warn about this case. Should have been more clear about that.

@nzakas

This comment has been minimized.

Copy link
Member

commented Jun 22, 2015

What is it warning about, exactly? Each callback has a return.

@gcochard

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2015

It violates (IMHO) the spirit of the rule, which is to keep a callback from being called twice. Putting the callback call inside an anonymous function causes the return to be in the wrong place. It should paired with process.nextTick so that each code path which calls a callback is a terminal path.

@nzakas

This comment has been minimized.

Copy link
Member

commented Jun 22, 2015

I think it's a stretch to include that case in this rule. I'd vote for keeping to the original description.

@gcochard

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2015

I'm okay with that. Just make a note that deferred callbacks may not be flagged by this rule.

@xjamundx

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2015

"callback-return": [0, ["callback", "cb", "next"]] seem like a good name and reasonable defaults?

@xjamundx

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2015

I thought about turning this on by default in the node environment and ran into this...

Validating Makefile.js

./Makefile.js
  721:12  error  Expected return with your async function  callback-return

✖ 1 problem (1 error, 0 warnings)
lib/config-initializer.js
  32:8  error  Expected return with your async function  callback-return

lib/util/traverse.js
  61:16  error  Expected return with your async function  callback-return
  81:8   error  Expected return with your async function  callback-return

xjamundx pushed a commit to xjamundx/eslint that referenced this issue Jun 23, 2015

@xjamundx

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2015

I guess I'll just remove that, because we're getting rules automatically being turned on anyway....

xjamundx pushed a commit to xjamundx/eslint that referenced this issue Jun 23, 2015

xjamundx pushed a commit to xjamundx/eslint that referenced this issue Jun 23, 2015

xjamundx pushed a commit to xjamundx/eslint that referenced this issue Jun 23, 2015

xjamundx pushed a commit to xjamundx/eslint that referenced this issue Jun 23, 2015

@lo1tuma

This comment has been minimized.

Copy link
Member

commented Jun 23, 2015

"callback-return": [0, ["callback", "cb", "next"]] seem like a good name and reasonable defaults?

What about done, which is often used in asynchronous tests (e.g. with mocha)?

xjamundx pushed a commit to xjamundx/eslint that referenced this issue Jun 24, 2015

xjamundx pushed a commit to xjamundx/eslint that referenced this issue Jun 24, 2015

xjamundx pushed a commit to xjamundx/eslint that referenced this issue Jun 24, 2015

xjamundx pushed a commit to xjamundx/eslint that referenced this issue Jun 24, 2015

xjamundx pushed a commit to xjamundx/eslint that referenced this issue Jun 25, 2015

xjamundx pushed a commit to xjamundx/eslint that referenced this issue Jun 25, 2015

xjamundx pushed a commit to xjamundx/eslint that referenced this issue Jun 26, 2015

ilyavolodin added a commit that referenced this issue Jul 1, 2015

Merge pull request #2828 from xjamundx/callback-return
New: add callback-return rule (fixes #994)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.