callback-return not working for calls immediately preceding returns

[revolter]

Tell us about your environment

Node version: v10.16.0
npm version: v6.13.7
Local ESLint version: v6.8.0 (Currently used)
Global ESLint version: Not found

What parser (default, Babel-ESLint, etc.) are you using?

default

Please show your full configuration:

1581359005.zip

Configuration

module.exports = {
    "extends": "eslint:all",
    "env": {
        "es6": true
    }
};

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

index_1.js:

(() => {
    "use strict";

    const object = {
        "test": (callback) => {
            if (callback) {
                callback();
            }
        }
    };

    object.test2 = 42;
})();

index_2.js:

(() => {
    "use strict";

    const object = {
        "test": (callback) => {
            if (callback) {
                callback();

                return;
            }
        }
    };

    object.test2 = 42;
})();

./node_modules/.bin/eslint index_1.js index_2.js

What did you expect to happen?

Only report error for index_1.js file.

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

/Users/revolt/1581359005/index_1.js
  11:17  error  Expected return with your callback function  callback-return

/Users/revolt/1581359005/index_2.js
  13:17  error  Unnecessary return statement  no-useless-return

✖ 2 problems (2 errors, 0 warnings)
  1 error and 0 warnings potentially fixable with the `--fix` option.

Are you willing to submit a pull request to fix this bug?

Maybe.

[kaicataldo]

Can you confirm that the error you are reporting as incorrect is actually coming from the no-useless-return rule?

If so, I actually think we should see if callback-return doesn't warn if there is no executable code afterward (as is the case here) so that it is compatible with no-useless-return.

[revolter]

How can I confirm that? If it helps, I already have attached the entire project in the original issue. It looks to me like those 2 rules are contradicting each other, and I expected for this case to be detected and correctly handled.

The documentation for callback-return says that it is:

aimed at ensuring that callbacks ... are always ... immediately preceding a return statement.

Hence why I expected for callback(); return; to work too, as return callback(); does.

[kaicataldo]

I just wanted to clarify, since the issue title is reporting callback-return, but the unexpected error (in index_2.js) is coming from the no-useless-return rule. Agreed that we should be able to use these two rules together.

I think no-useless-return is doing the correct thing here, and I would advocate for changing callback-return to not report the example in index_1.js (when there isn't any executable code after the callback) either by default or behind an option.

[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 issues failing to reach accepted status after 21 days tend to
never be accepted, 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.

[revolter]

@kaicataldo, is it complicated to implement?

[kaicataldo]

@revolter I'm still not clear on what the issue is. Do you mind clarifying what you're asking about?

[G-Rath]

@kaicataldo this might belong in another issue, but I'm going to put this here for now as it seems similar enough.

Overall I think callback-return has a bug, in that it's only checking if there is a return on the line that the callback is called on.

For example:

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

This should not be flagged by callback-return since the callback is being called in the main body of the function.

Additionally, putting a return after the callback does not satisfy the result, even if it is in a conditional:

const mutate = <TObject extends object, TKey extends keyof TObject>(
  obj: TObject,
  key: TKey,
  callback: (value: TObject[TKey]) => TObject[TKey]
): void => {
  if(process.env.ENABLE_CALLBACKS) {
    obj[key] = callback(obj[key]);

    return;
  }

  doSomething();
};

I don't think the rules interaction with no-useless-return is notable, as it's to be expected - the real bug is that callback-return only checks for the return on the same line.

---

Let me know if you'd like me to make a fresh issue to track this :)

[kaicataldo]

Making a new issue would be great. We've seen that discussions on closed/stale issues tend to not get much discussion.

[G-Rath]

@kaicataldo I've created #13064 - apologises for the title; it's been a long day and couldn't find of a more concise way to expression the problem (since its not actually only assignments).

[kaicataldo]

Much appreciated!

[revolter]

I think no-useless-return is doing the correct thing here

Why do you think this? If I have a function that returns void, and a callback that returns something else, I don't want to return it, but only to exit after the callback is called, and in this case, no-useless-return is incorrect, because that return is not useless, it is to satisfy the callback-return rule, hence, the issue title.

I would advocate for changing callback-return to not report the example in index_1.js (when there isn't any executable code after the callback) either by default or behind an option.

But it wouldn't work for:

(() => {
    "use strict";

    const object = {
        "test": (callback) => {
            if (callback) {
                callback();
            }

            alert("something else");
        }
    };

    object.test2 = 42;
})();

in which case, I would like to be able to have:

(() => {
    "use strict";

    const object = {
        "test": (callback) => {
            if (callback) {
                callback();

                return;
            }

            alert("something else");
        }
    };

    object.test2 = 42;
})();

and "extends": "eslint:all", without ESLint reporting any error.

[revolter]

If so, I actually think we should see if callback-return doesn't warn if there is no executable code afterward (as is the case here) so that it is compatible with no-useless-return.

Actually, yes. I tested it again, and adding some code after that return makes it work correctly.