Fix: make no-unused-vars ignore for-in loops (fixes #2342)

[markelog]

Also clean jsdocs for internal helpers

[eslintbot]

LGTM

[mention-bot]

By analyzing the blame information on this pull request, we identified @vitorbal, @nzakas and @mysticatea to be potential reviewers

[markelog]

I assume phantom issue is known problem

[ilyavolodin]

Yes, phantom issue has been screwing our builds for a while now...

[alberto]

@markelog I think the agreed-upon change was to ignore only for (x in y) return;, not any for..in/of loop

[markelog]

@ilyavolodin

Is there ticket about this? Downloading phantom binary is known bottleneck and that's syntactic environment, since alternative would be to actually make ci run tests in real browsers, i would propose to do that instead.

@alberto

mm, i was under impression for (x in y) return; example was to illustrate the point.

Otherwise we would need to inspect body of the loop, which would introduce it seems unnecessary complexity. I'm not sure what kind of disadvantages we would try to avoid by doing that, i could see though breaking a use-case of object getters on which that loop could be iterated upon if we would be specific with return keyword.

[nzakas]

Agree with @alberto - the resolution of #2342 is to explicitly exempt for-in when it's body is just return (see #2342 (comment)). We definitely don't want to exempt all loops.

@markelog can you update this PR accordingly? Also, can you update the commit message to begin with "Update" instead of "Fix"? (The issue is labeled as an enhancement, so we use " Update" for that.)

[markelog]

ok-ok, so we targeting only loops with one statement which should be return, correct?

[nzakas]

Only for-in loops with return, yes.

[markelog]

Sorry to keep asking about this, but i want to be clear about it - only one statement with return is not violation? Like this -

for (var key in obj) {
 i++;
 return false;
}

Is violation or not? Also your last comment was #2342 (comment) yeah, but then there was #2342 (comment)

which includes

and same pattern for for..of

Those statements confuse a bit

[nzakas]

Just for-in and only when the loop body just has a return statement. So your example would still be flagged as a problem.

[markelog]

ok, thank you for clarification

[platinumazure]

@markelog @nzakas Hard to tell if this is ready for merge- if not, can we put "do not merge" on here?

[markelog]

Done, thank you

[eslintbot]

LGTM

[eslintbot]

Thanks for the pull request, @markelog! I took a look to make sure it's ready for merging and found some changes are needed:

• The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

[eslintbot]

LGTM

[nzakas]

Why did you remove @private everywhere?

[markelog]

There is a lot of rules that do use this keyword and there is a lot of rules that don't, some of the helpers of this particular rule also do and do not.

So it seems we have to choose with or without it, since in my view it is already obvious they are private, i choice the latter.

Trying to make it consistent, what way seems better to you?

[markelog]

So what should i do?

[nzakas]

I'd prefer to have @private, as it will prevent JSDoc from generating docs for them if we ever get that working again. :)

[markelog]

ok-ok, also, i think it would the right move to do this everywhere else, how about creating a ticket about this?

[markelog]

Will make a fix for other remarks after clarification, so it could be done in one move

[markelog]

Done, sorry for the delay

[ilyavolodin]

@markelog It looks like there's a still merge conflict in this PR. Could you rebase please?

[eslintbot]

LGTM

[markelog]

Donzo, there is a lot of interesting code in there.

[mysticatea]

I guess this may cause null dereference error at empty block body.

[markelog]

mm, could you elaborate with some code?

[mysticatea]

(function(obj) { var name; for ( name in obj ) { } })({});

[markelog]

Fixed thanks, btw, was about to cc you here and there you are :)

[eslintbot]

LGTM

[mysticatea]

I'd like to see a test for the following code:

for (const key in obj) return true;

[markelog]

You mean for const? I guess it also makes sense to add similar test for let too?

[mysticatea]

I'm seeing that this is checking whether the parent node of an identifier is a ForInStatement or not.
My example is that the parent node is a VariableDeclarator.

[eslintbot]

LGTM

[markelog]

@mysticatea done

[mysticatea]

Thank you very much!
Looks good to me.

[markelog]

Okay, so it seems i have addressed all concerns, so if no one objects, i will land this today

[nzakas]

@markelog why was the commit message changed from "Fix" to "Update" here?

Note that as soon as it becomes "Update", a TSC member needs to merge the code (because it causes a minor semver bump).

[nzakas]

FYI: We changed the commit message back to "Fix" as it seems like that is what is appropriate given the semver policy.

[alberto]

Thanks @markelog for keeping up!

[markelog]

@nzakas Probably is, on #2342 it has label enhancement that is why i change commit tag, should we change label on that issue?

Note that as soon as it becomes "Update", a TSC member needs to merge the code (because it causes a minor semver bump).

Ok-ok, i understand correlation now