Update: array-callback-return checks Array.forEach (fixes #12551)

[gabrieldrs]

What is the purpose of this pull request? (put an "X" next to item)
[x] Changes an existing rule

What rule do you want to change?
array-callback-return

Does this change cause the rule to produce more or fewer warnings?
more

How will the change be implemented? (New option, new default behavior, etc.)?
new option checkForEach created, defaulted to false. when true, rule will also check if Array.forEach callback has a return statement and warn if it does

Please provide some example code that this change will affect:

Examples of incorrect code for the { "checkForEach": true } option:

/*eslint array-callback-return: ["error", { checkForEach: true }]*/
myArray.forEach(function(item) {
    return handleItem(item)
});

Examples of correct code for the { "checkForEach": true } option:

/*eslint array-callback-return: ["error", { checkForEach: true }]*/
myArray.forEach(function(item) {
    handleItem(item)
});
myArray.forEach(function(item) {
    if (item < 0) {
        return;
    }
    handleItem(item);
});

What does the rule currently do for this code?

doesn't check

What will the rule do after it's changed?

check if Array.forEach callback has a return statement and warn if it does

What changes did you make? (Give an overview)

added checkForEach option which enables/disables the new rule check

Is there anything you'd like reviewers to focus on?

[gabrieldrs]

updated PR description with required information

[mdjermanovic]

@gabrieldrs thanks for the PR! Left a couple of notes where I believe we should change the approach.

In general, this option should report any return value from forEach. I believe it should also report implicit returns from arrow functions.

Fill free to ask if you have any questions!

[kaicataldo]

Can we document/test how the two options interact? Or should they be mutually exclusive? i.e. what happens when the rule is configured as { checkForEach: true, allowImplicit: false }?

[gabrieldrs]

I think they shouldn't be mutually exclusive, and, at the same time, one should not effect the other, in the sense that setting allowImplicit should not change the behaviour of checkForEach, and vice versa.

I will create tests with both options set to ensure this.

[kaicataldo]

@gabrieldrs That makes sense to me! I think we'll just need to update the documentation for allowImplicit to clarify which Array.prototype methods it applies to.

[mdjermanovic]

That makes sense to me, too.

I think that this rule shouldn't disallow return; in forEach callback. That kind of restriction might require the user to make a serious refactoring. It might be a separate generic rule to allow return only at the end of functions (though, I'm not sure how popular that style is).

Also, "allowImplicit": true shouldn't allow x => x arrow return in forEach (this also looks like something 'implicit'). This might be an additional option in the future.

[mdjermanovic]

Thanks for the changes, this looks good in general! I left some notes, mostly about the documentation and tests.

[aladdin-add]

IMHO, no need to add the option, just make it as default behavior -- a breaking change in ESLint v7. thoughts? @mdjermanovic @kaicataldo

[ljharb]

It's always better imo to add an option in a minor, and then flip the default in a major - that also gives users a super easy way to bypass the breakages and more easily upgrade.

[aladdin-add]

emm...yes, a tradeoff! the dark side is: it added complexity, but I didn't see how it's useful -- returning a value in Array.forEach is mostly a mistake.

[ljharb]

There are many people who intentionally omit the curly braces in forEach, knowing the return value is ignored, and would not want warnings there. I'm not one of those people, nor hopefully is anyone touching any code I have to work with, but these people do exist :-)

[mdjermanovic]

If someone doesn't want this for some reason (e.g., because of implicit arrow functions or something else may appear), the only choice for them would be to completely turn off the rule and thus lose the current default behavior.

So it might be safe to start with an option and see from the feedback is there a need for additional options to tweak this option (i.e. options of the option), and those options would stay when we eventually remove this one.

[mdjermanovic]

Looks great! Left just a couple of notes.

[mdjermanovic]

Just a couple of minor details, otherwise this looks really great!

[mdjermanovic]

LGTM! All details are covered well (and that includes details from the previous behavior since this change required a redesign of the existing code).

Thanks for the great work, @gabrieldrs!

[kaicataldo]

LGTM. Thanks for sticking with all the feedback!

[kaicataldo]

Thanks for contributing to ESLint!