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 suggestion: error when iterators like map or reduce do not return #1128

Closed
spadgos opened this Issue Jul 28, 2014 · 27 comments

Comments

Projects
None yet
@spadgos
Copy link
Contributor

spadgos commented Jul 28, 2014

On quite a few occasions, I've been messed up by forgetting to return the memo object in a reduce call:

// example: convert ['a', 'b', 'c'] --> {a: 0, b: 1, c: 2}
myArray.reduce(function (memo, item, index) {
  memo[item] = index;
}, {}); // Err: cannot set property 'b' of undefined

I would argue that it never makes sense to have no return statement in:

  • map
  • reduce
  • reduceRight
  • filter
  • every
  • some

...since if you don't have a return statement, you could just use a forEach (except for every, but that would be a weird construct too).

Also, it never makes sense to have a return statement in forEach. I commonly see the misconception that forEach can be short-circuited with return false.

For the first list, I would not go so far to ensure that a return is guaranteed, since it sometimes is not necessary in each iteration (eg: for filter, every and some, no return value is equivalent to return false).

The following would be considered errors/warnings:

arr.map(function (item) {
  someStatements();
});
arr.reduce(function (memo, item) {
  someStatements(function () {
    return somethingInner;
  });
});
arr.forEach(function (item) {
  return true;
});

The following would not be considered warnings:

arr.reduceRight(function (memo, item) {
  return memo + item;
});

arr.filter(myFunction); // only function literals checked for simplicity

I do see one large potential problem with this, in determining when the iterator functions actually belong to Array.prototype and when they're just members on another arbitrary object.

var myMappingObject = {
  map: function (callback) {
    // do something, perhaps with the google maps api?
  }
};
myMappingObject.map(function () {
  someStatements(); // would be incorrectly marked as an error
});

What do you think?

@jrajav

This comment has been minimized.

Copy link
Contributor

jrajav commented Jul 29, 2014

I think this would be a good rule to have. Missing a .map or .every return usually results in nonworking code, but missing a .reduce return can sometimes be a latent bug. We've run into that at least twice on this very project!

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jul 29, 2014

Okay by me. I think even if we are not dealing with arrays, the expectation is still that these methods expect a callback with a return.

@jrajav

This comment has been minimized.

Copy link
Contributor

jrajav commented Jul 29, 2014

I think that's reasonable too, but maybe it should just be "if map/reduce/every/whatever, AND the parameter is a callback, THEN it must have a return", rather than also enforcing a callback?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jul 30, 2014

I'm sorry, I don't understand your comment. AFAIK each of these methods require a callback to be passed. Am I missing something? Are you maybe referring to using a function expression rather than a variable containing a reference to a function?

@jrajav

This comment has been minimized.

Copy link
Contributor

jrajav commented Jul 30, 2014

I'm saying that this shouldn't violate the rule:

numberObject.reduce(4);

But this should:

array.reduce(function (iter, el) {
    iter += transform(el);
});

That is, include "parameter is a function expression" in the rule heuristics. I guess that was already implied.

@spadgos

This comment has been minimized.

Copy link
Contributor Author

spadgos commented Jul 30, 2014

Small addendum for forEach. A return with no value could be allowable. Consider:

arr.forEach(function () {
  if (someCondition) {
    return;
  }
  someStatements();
});
@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jul 30, 2014

@jrajav yes, that was definitely the implication here.

@spadgos agreed.

@jrajav

This comment has been minimized.

Copy link
Contributor

jrajav commented Jul 30, 2014

The foreach thing only seems to be tangentially related. Should it be in the same rule?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jul 31, 2014

I agree, I'd leave forEach out of this rule.

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Aug 1, 2014

#566 brought up branch tracking for the consistent-return rule, but it was closed without a PR. Would this rule require branch tracking to ensure that all branches return? If not, what's the appropriate logic? Any return value at all?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 2, 2014

That's a good point - in an ideal world we'd have branch tracking checks for return. That also complicates this greatly.

@btmills

This comment has been minimized.

Copy link
Member

btmills commented Aug 2, 2014

There isn't yet an official way to share code between rules, is there? Presumably branch tracking is something to be implemented only once.

Actually, it doesn't need to be shared. This rule could check that there is at least one return value, and consistent-return, with branch tracking added, would verify that all branches return values.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 3, 2014

Rules need to be atomic and not have dependencies on other rules. I don't know if it's possible to share this functionality because it's directly related to the traversal. We would need someone to implement this functionality to evaluate.

@doberkofler

This comment has been minimized.

Copy link
Contributor

doberkofler commented Jan 24, 2015

+1

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Aug 30, 2015

Are we missing accepted tag here @nzakas ?

@IanVS

This comment has been minimized.

Copy link
Member

IanVS commented Aug 30, 2015

Looks like once #3530 lands, this could be turned into a very excellent rule. I know it would have saved my bacon a few times.

@nzakas nzakas added the accepted label Aug 31, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 31, 2015

Yeah, this was created before we used "accepted". 👍

@BYK

This comment has been minimized.

Copy link
Member

BYK commented Sep 1, 2015

@IanVS - why do we need #3530 for this? Can't we just check function declaration and expressions that are passed into certain function names?

@IanVS

This comment has been minimized.

Copy link
Member

IanVS commented Sep 1, 2015

in an ideal world we'd have branch tracking checks for return.

Consider:

[1,2,3,4,5].reduce(function (acc, val) {
    if (val % 2 === 0) {
        acc.push(val);
        return acc;
    }
}, []);

As far as I can tell, without branch checking, we might look at this and think that we are returning acc as needed. Or maybe I'm missing another solution?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Sep 1, 2015

Btw, your example is invalid:-) You should always return from inside reduce. And to determine if every code path returns you need #3530

@IanVS

This comment has been minimized.

Copy link
Member

IanVS commented Sep 1, 2015

Btw, your example is invalid

That was exactly the point, it is an example of code this rule should warn about.

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Dec 6, 2015

Now, #3530 was merged into master, so I'd like to work on this in next few days.

My plan is that the rule checks every code path in callbacks of array's functional methods returns a value, then the rule will say messages similar to consistent-return. I guess the detection way for callbacks of array's functional methods is similar to what no-invalid-this is using.

Well, I want to get suggestions of the rule name.
To think the name is too hard to me...

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 6, 2015

Something like loop-func-return? array-func-return?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 6, 2015

array-callback-return?

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Dec 7, 2015

@ilyavolodin @nzakas Thank you for suggestions!

I will go with array-callback-return.

@mysticatea mysticatea self-assigned this Dec 7, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Dec 7, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Dec 7, 2015

@IanVS

This comment has been minimized.

Copy link
Member

IanVS commented Dec 7, 2015

I'm really excited for this rule, @mysticatea. Thanks for working on it.

mysticatea added a commit to mysticatea/eslint that referenced this issue Dec 8, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Dec 9, 2015

@nzakas nzakas closed this in #4628 Dec 11, 2015

@joeytwiddle

This comment has been minimized.

Copy link

joeytwiddle commented Jun 22, 2017

Did the .forEach() rule ever get written?

My searches for no-array-callback-return and array-callback-no-return came up blank.

(One could argue that must-return-something and must-not-return-something are actually the same concern: must-return-appropriate-type)

@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.