no-unused-vars incorrectly raised when variable is used with for…in

[RubenVerborgh]

The no-unused-vars error is incorrectly triggered when a variable is only used in a for in statement.

The following snippet gives the error no-unused-vars:

(function () {
  "use strict";
  var key;
  for (key in this) return;
})();

3:6  error  key is defined but never used  no-unused-vars

However, the variable is clearly used in for (key in this).
Furthermore, omitting the declaration also results in an error:

(function () {
  "use strict";
  for (key in this) return;
})();

3:7  error  "key" is not defined  no-undef

The expected behavior is that the first snippet does not yield any error; the second error is correct.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[ilyavolodin]

In the code you provided, key is actually not used for anything. This rule considers a variable to be used when it's value is assigned to something, or used as part of the call. In your case, neither is done.

[RubenVerborgh]

It seems that “usage” is broader than just assignments or calls. Here is a meaningful example:

function countNumberOfKeys(object) {
  var key, i = 0;
  for (key in object)
    i++;
  return i;
}

It would not seem right to have the above example linted by setting an exception for that line only.

[nzakas]

We probably need to add this exception for both for-in and for-of. These loops are impossible without a variable, so that should count as used.

[RubenVerborgh]

And thinking about this some more: for…in actually does an assignment if you think about it. It assigns values to the key variable. So that qualifies for the above definition of “used”.

[nzakas]

Assignment to the variable is not considered usage. So this is not usage:

var x;
x=1;   // unused

That's what the for-in loop is doing.

Assigning the variable to another variable is usage:

var x;
x=1;
var y=x;  // now x is used but y isn't

[RubenVerborgh]

Indeed; I misread the definition above ("when its value is assigned to something"). I guess for in/off is the one case where “assignment” is mandatory, so we really need to have the variable.

[gyandeeps]

@nzakas I think we can close this as looking at the comments there is no issue here.

[RubenVerborgh]

I don't think closing is the appropriate action at this time; @nzakas said
above we probably need to add an exception.

On Sunday, May 17, 2015, Gyandeep Singh notifications@github.com wrote:

@nzakas https://github.com/nzakas I think we can close this as looking
at the comments there are no issue here.

—
Reply to this email directly or view it on GitHub
#2342 (comment).

[nzakas]

Yeah, we need to add the exception for these loops.

[michaelficarra]

I don't think we should make an exception for these. They are unused. You can disable the rule for this line. I believe a previous issue also proposed that particular names be exempt from this rule: "no-unused-vars": [2, ["unused", "ignored", "_"]].

[RubenVerborgh]

It all depends on your exact definition of used. The variable is not read, no. But it is written to. And there is no way to write this code without the variable, nor is it necessary to read it. The variable is used in the sense that it is necessary for the code to have the variable.

[RubenVerborgh]

Also note that there is no way to write a functionally identical piece of code without switching off at least one warning: either no-unused-vars or no-undef is raised. I think it should at least be possible to write this code without needing to ignore anything. This is regardless of style, because no alternative exists.

[michaelficarra]

We've already defined usage as a read. I'm claiming that this is not a common or important pattern. I would use Object.keys(...).forEach(...) for this.

[RubenVerborgh]

I would then suggest clarifying on http://eslint.org/docs/rules/no-unused-vars.html that use means read. Note that Object.keys(...).forEach(...) is not equivalent, as this takes O(n) whereas the code I list above takes O(1).

[ilyavolodin]

Somewhat unrelated, @RubenVerborgh unless I completely missed something, code you posted above is also O(n) and is an exact equivalent of what @michaelficarra suggested.

[RubenVerborgh]

I don't want to start a code-complexity war here 😉, but:

  for (key in this) return;

only needs to assign one value to key and then return, hence it can be implemented in O(1) (regardless of how concrete JavaScript engines do it).

The code I posted was a simplified example from a real-word function, which needs to return true when there is at least one key, and false when there is none. At the time of implementation, for in was faster for this purpose than Object.keys across engines. So in that sense, Object.keys is not an equivalent.

I keep finding it strange that var key; for (key in this) return; would throw an error for a variable being unused, whereas without the var declaration, you would also get an error. There's no stylistic alternative in this case that does not throw the error. Object.keys() has a different performance, and other problems (can be overridden etc).

[nzakas]

The problem I see here is that turning off the rule around an area is the nuclear option, you could be missing other unused variables. I think we need an exception of some sort, and am open to suggestions.

[RubenVerborgh]

Could we pass an option to the no-unused-vars rule? But not for specific names, as suggested above, but rather for for-constructs?

[nzakas]

Passing an option is an option (ha!) but the problem with options is always how to do them in such a way that they make sense and aren't overly broad or narrow. Like, do you really want to always to exempt all for-in loops? What about this:

for (var foo in bar) {
    doSomething(bar);
}

In this case, it's more likely than not that you meant to include foo somewhere in there and just forgot, so I'd want that to be flagged.

If we then say only your specific case for (var foo in bar) return; doesn't cause a warning, then we've now made it overly narrow such that everyone will want their own pattern as an opt-out as well.

I'm very much against adding options that could lead to you missing an error - we always try to add options that always do the right thing so that there's no way an end user can choose incorrectly and end up missing something.

[RubenVerborgh]

There seems to be no way in general to guard against the doSomething(bar) scenario. I'm afraid that exempting either all for in loops or none is the only realistic option. Perhaps adjusting the error message to foo is assigned to but never read might also bring clarity.

BTW For a couple of scenarios where this case occurs, see this code, in particular 46–49 and 70–74.

[nzakas]

We'd need a more generic message, as declarations aren't always assignments and I really don't want to special-case messaging for assignments.

We could maybe do foo is declared but never read.

I'm a bit concerned that using "read" instead of "used" is introducing more ambiguity, but I'd like to hear opinions on that.

[michaelficarra]

I prefer the message use "used" as it does now. If someone doesn't understand that, the documentation should describe in detail what actually constitutes a "use" with examples.

[nzakas]

Yeah, I thought we had updated the docs at some point, but apparently not. We should do that.

So, circling back to the original point, I don't think it's safe to have a flag that blocks warnings for all for-in loops, and it doesn't appear that there's a simple pattern to flag, so I'm going to change this over to a documentation task.

[michaelficarra]

for (var key in object) return;

[Zarel]

Sorry, I misread "is the" as "is in the".

[Zarel]

In case it's relevant, I don't remember either JSHint or JSCS complaining about my code, so they may have done something like this.

[nzakas]

Pretty sure JSCS doesn't check variable usage, and JSHint has only very basic usage checking that tends to miss things.

I still don't see any good solution to this. We can choose to ignore for (x in y) return all the time, but I think anything more than that gets us into a really fuzzy area that's not helpful for anyone.

I'm definitely against the break/return proposal.

@ilyavolodin who else brought this problem up?

[ilyavolodin]

It was brought up a few times in gitter chat room, I believe. I also don't see a good solution for the problem.

[nzakas]

Here's my proposal: we just always allow patterns like for (x in y) return. That's the extent to which I think we should go to address this.

If there's disagreement on that, then I'd say this is best left to a custom rule.

[ilyavolodin]

I think I can buy exception for for in.

[ilyavolodin]

Marked this as accepted and removed documentation as it seems the decision was reached to make an implicit exception for for (x in y) return; pattern only (and same pattern for for..of).