[callback-return] only accepts callbacks that are by themselves or prefixed with a return

[G-Rath]

callback-return only accepts callbacks that are by themselves on a line, or prefixed with return.

Tell us about your environment

• ESLint Version: 6.8.0
• Node Version: 12.14.1
• npm Version: 6.14.2

What parser (default, Babel-ESLint, etc.) are you using?
@typescript-eslint/parser

Please show your full configuration:

Configuration

module.exports = {
  root: true,
  parser: '@typescript-eslint/parser',
  parserOptions: {
    project: 'tsconfig.eslint.json',
    ecmaVersion: 2019,
    sourceType: 'module'
  },
  env: { node: true },
  rules: {
    'callback-return': 'warn'
  }
};

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

const mutate = <TObject extends object, TKey extends keyof TObject>(
  obj: TObject,
  key: TKey,
  callback: (value: TObject[TKey]) => TObject[TKey]
): void => {
  obj[key] = callback(obj[key]);
};

eslint . --ext ts

What did you expect to happen?

callback not to be flagged by callback-return.

What actually happened? Please include the actual, raw output from ESLint.

callback is flagged by callback-return:

16:14 warning Expected return with your callback function callback-return

Are you willing to submit a pull request to fix this bug?
Maybe - looking at the source code & documentation, it might be tough to implement a sound fix.

---

Demo reproduction.

[kaicataldo]

This does indeed look like a bug. Verified with the demo link above.

[anikethsaha]

@G-Rath mind if I take this ?

here is the fix which I think is better fit

15c0f4f

@kaicataldo if the callback is assigned to some variable without the call like this

function fn(err, cb){
  var cb2 = cb
}

now, it should check and add cb2 to callbacks array as well right ?

cause,

function fn(err, cb){
  var cb2 = cb
  if (err) {
        cb2(err); // throw error here 
    }
    cb2();
}

right ?

[kaicataldo]

I think the bug here is what @G-Rath describes above. We'll need to figure out the best heuristic to catch this. I think we'll want to make this check more robust.

[anikethsaha]

We'll need to figure out the best heuristic to catch this. I think we'll want to make this check more robust.

isCallback is just a function to check whether the call expression is about callbacks or not!

I think, to fix this issue we need to check where that callExpression (getting check from isCallback) is being called ! if its called as assigned statement or as variable declaration, it should not report.

Let me know if I am wrong about it !

@kaicataldo if the callback is assigned to some variable without the call like this

function fn(err, cb) {
  var cb2 = cb;
}

now, it should check and add cb2 to callbacks array as well right ?

cause,

function fn(err, cb) {
  var cb2 = cb;
  if (err) {
    cb2(err); // throw error here
  }
  cb2();
}

Even I don't want to make this rule this much complicated to analyze the variable like how it's being used. (not in the scope of this rule ) !

@G-Rath mind if I take this ?

I can submit a PR for this and we can discuss with the implementation once getting clarification from @G-Rath about this

[kaicataldo]

I think, to fix this issue we need to check where that callExpression (getting check from isCallback) is being called ! if its called as assigned statement or as variable declaration, it should not report.

I don't think this will work, because the following wouldn't be caught:

function fn(val, err, cb) {
  let value = val;

  if (err) {
    value = cb(err);
  }

  console.log(value);

  cb(val);
}

This is a really contrived example, but I think it should still be caught. Feels to me like we should be using code path analysis to ensure that anytime the callback is being called that it cannot be called again.

[anikethsaha]

I don't think this will work, because the following wouldn't be caught:

function fn(val, err, cb) {
  let value = val;

  if (err) {
    value = cb(err);
  }

  console.log(value);

  cb(val);
}

yes, this seems like a not complete fix. I will try to implement something else then . !

[anikethsaha]

Feels to me like we should be using code path analysis to ensure that anytime the callback is being called that it cannot be called again.

If I understand this correctly, it can't be called more than once for a single block right ?

function(cb){
 cb()
  ...
cb() // not allowed
}

But it should be allowed for this

function(cb){
 if(foo) cb()
 cb(foo) // no error
}

right ?

[kaicataldo]

function(cb){
 if(foo) cb()
 cb(foo) // no error
}

This should be an error. From the rule's docs:

To prevent calling the callback multiple times it is important to return anytime the callback is triggered outside of the main function body. Neglecting this technique often leads to issues where you do something more than once.

The intention of the rule is to prevent a callback from being called multiple times, and in the example above, it would be called twice if foo evaluates to truthy.

[G-Rath]

Sorry for the late response:

@anikethsaha

@G-Rath mind if I take this ?

Go for gold!

[anikethsaha]

OH , sorry I forgot to add return before the cb.

I meant this

function(cb){
 if(foo) return cb()
 return cb(foo) // no error
}

[anikethsaha]

@G-Rath I anticipated this rule a bit, I guess it would take time for me to fix it. As my solution is not covering all cases

If anyone else can submit before that. Go for it!

[aladdin-add]

is it a Node.js specific rule? it might be better to be in eslint-plugin-node, as we had planned to deprecate Node.js & CommonJS rules.

refs: #12898

[anikethsaha]

I dont think its node-specific, as callbacks are pretty common in the browser with a maximum browser supporting it 👍

[kaicataldo]

is it a Node.js specific rule? it might be better to be in eslint-plugin-node, as we had planned to deprecate Node.js & CommonJS rules.

This is an interesting point. I've definitely encountered this pattern is Node-land more than in the browser.

[anikethsaha]

@kaicataldo I think this rule should not throw error for those callbacks which are in the function body irrespective of how many times they are being called.

the following should be lint free (they are currently error in master)

function a(err) { callback (err); callback(); }
var a = (err) => { callback (err); callback(); }

And the following example should be of incorrect (currently lint-free in master)

if (x) { callback(); }

Cause as per rule's docs

This rule is aimed at ensuring that callbacks used outside of the main function block are always part-of or immediately preceding a return statement

If callback is in the function body (parent.type should be of "FunctionDeclaration","FunctionExpression","ArrowFunctionExpression")` it should be lint free

WDYT ?

[kaicataldo]

@anikethsaha I respectfully disagree. The practical purpose of the rule is to prevent callbacks from being invoked multiple times. From the docs (emphasis my own):

To prevent calling the callback multiple times it is important to return anytime the callback is triggered outside of the main function body. Neglecting this technique often leads to issues where you do something more than once.

If the goal is prevent callbacks from being invoked multiple times, the following should be disallowed since it will always invoke the callback twice.

function a(err) { callback (err); callback(); }
var a = (err) => { callback (err); callback(); }

I actually would like to explore @aladdin-add's point about this being a pattern that's specifically used in Node's core modules and more often in Node-land than in the browser. @mysticatea What are your thoughts on this? Would it make sense to add this to eslint-plugin-node as well?

[kaicataldo]

I'm clearly a bit scattered, because this is one of the rules we just ported over to eslint-plugin-node. Since we are deprecating this rule, I would suggest closing this and moving this discussion to a new issue over in that repository. Sorry for the confusion, all!

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.