no-unused-vars reports warning at incorrect position for recursive functions

[sdegueldre]

environment

Node version: v14.4.0
npm version: v6.14.5
Local ESLint version: v7.12.1 (Currently used)
Global ESLint version: v7.8.1

parser

default

Configuration

{
    "env": {
        "es6": true,
    },
    "root": true,
    "rules": {
        "no-unused-vars": ["warn", {"args": "none"}],
    },
}

Code being linted:

function fact(num) {
	return num * fact(num - 1);
}

command used to lint the code

npx run eslint test.js

What did you expect to happen?

no-unused-var is triggered on the function declaration:

  1:10  warning  'fact' is defined but never used  no-unused-vars

✖ 1 problem (0 errors, 1 warning)

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

no-unused-var is triggered on the last internal self-reference of the function:

  2:15  warning  'fact' is defined but never used  no-unused-vars

✖ 1 problem (0 errors, 1 warning)

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

I have never touched eslint's code but if someone would like to point me in the right direction I'm willing to submit a PR for this.

I would also like to note that this is a regression as eslint 7.1 correctly reports the warning at 1:10, it starts reporting the warning at 2:15 in version 7.2

[btmills]

Demo link for anyone else who wants to try this.

Thanks for the tip on the v7.1-v7.2 regression - that made it easy to track down. In #13354 and #13181, we intentionally changed no-unused-vars to report the last reference to a variable that's not used. See #13181 (comment) for a case where that change makes much more sense than this.

I'm wondering if this is perhaps an unintended side effect of that change. Even in the primary motivating example (demo from the original issue), we report the last reference but not the last assignment. If I were to remove the assignment, the warning would disappear from the reference.

I think the fix here might be to report the last assignment reference instead of just the last reference of any kind. (If there are no assignments, we'd still report the declaration.) Thus in variable = await variable, we'd change the rule to report the left side rather than the right, and in this issue's description, we'd report the declaration rather than the recursive call.

Tagging @anikethsaha who made the original improvement. What do you think of my proposed change?

[anikethsaha]

I think we had a issue to reconsider the position again to declaration but not sure whether we reached a decision of not.

I think the fix here might be to report the last assignment reference instead of just the last reference of any kind. (If there are no assignments, we'd still report the declaration.) Thus in variable = await variable, we'd change the rule to report the left side rather than the right, and in this issue's description, we'd report the declaration rather than the recursive call.

This seems much better than the current implement. +1 for this

[sdegueldre]

I'm unfamiliar with the way eslint works so pardon me if I'm suggesting something wild here, but wouldn't it possible to warn on every assignment where the variable goes unused before the end of the scope or the next assignment? eg:

function test() {
  let a = 5;
  console.log(a);
  a = 7; // warn here
  a = 2;
  console.log(a);
  a = 9; // warn here too
}

This is could be achieved by converting to SSA form, as all of these assignments would then be treated as "new" variables and it becomes obvious which ones are actually used and which ones are not. Although if we're not already converting to SSA at some point in the pipeline I guess it would add quite a bit of overhead.

[mdjermanovic]

I'm unfamiliar with the way eslint works so pardon me if I'm suggesting something wild here, but wouldn't it possible to warn on every assignment where the variable goes unused before the end of the scope or the next assignment? eg:

function test() {
  let a = 5;
  console.log(a);
  a = 7; // warn here
  a = 2;
  console.log(a);
  a = 9; // warn here too
}

It's certainly doable, but I think out of scope for this rule. It could be a separate rule, like no-unused-assignments or no-useless-assignments. ESLint provides code path analysis which can be used for that purpose, but it would be still a fairly complex rule.

[mdjermanovic]

This also looks wrong:

/* eslint no-unused-vars: error */

let a;

reset();
a = foo(a);
a = foo(a);
reset();
a = foo(a);
a = foo(a); // should report here?

function reset() {  
    a = 0; // reports here
}

Demo link

The reported location should probably never be inside an inner function, since we can't know if it's really the last assignment.

[sdegueldre]

It's certainly doable, but I think out of scope for this rule. It could be a separate rule, like no-unused-assignments or no-useless-assignments. ESLint provides code path analysis which can be used for that purpose, but it would be still a fairly complex rule.

I'm not sure I understand why it would be out of scope. If what I described is not the expected behaviour, then what exactly is the expected behaviour? Clearly, in all these examples, the variables are used, they're just not used after being reassigned.

If we take the description of the rule from the documentation:

A variable foo is considered to be used if any of the following are true:

• It is called (foo()) or constructed (new foo())
• It is read (var bar = foo)
• It is passed into a function as an argument (doSomething(foo))
• It is read inside of a function that is passed to another function (doSomething(function() { foo(); }))

A variable is not considered to be used if it is only ever declared (var foo = 5) or assigned to (foo = 7)

Most of the examples in this thread and in the issues linked to the original fix should report no error at all.

[mdjermanovic]

This rule isn't intended to detect dead stores, but just variables that are never read.

We did add some enhancements like #6348, which is to not count reads used to update the variable. Maybe we shouldn't have done that as it makes the scope unclear and causes confusion.

Simply said, the purpose of this rule is to detect variables with zero read references. A few exceptions to this logic are documented as examples of incorrect code:

// A read for a modification of itself is not considered as used.
var z = 0;
z = z + 1;

// Unused recursive functions also cause warnings.
function fact(n) {
    if (n < 2) return 1;
    return n * fact(n - 1);
}

[btmills]

The reported location should probably never be inside an inner function, since we can't know if it's really the last assignment.

@mdjermanovic yes, I agree with your example there as well. So at least don't cross function boundaries. What about other scope boundaries? Example:

/* eslint no-unused-vars: error */

let a;

reset();
a = foo(a);
a = foo(a);
reset();
a = foo(a);
a = foo(a); // should report here?

for(;;) {
    a = 0;
}

if (foo()) {
    a = 0; // or here?
}

Demo link

I think in this case I'd be fine warning inside of loops/conditionals and other block-like scopes since flow still proceeds roughly linearly from declaration to their entrance. Functions don't behave like that, so we shouldn't warn across function scopes.

[sdegueldre]

We did add some enhancements like #6348, which is to not count reads used to update the variable. Maybe we shouldn't have done that as it makes the scope unclear and causes confusion.

Agreed. While this makes sense if the right-hand of the assignment has no side effect, it causes confusing results with function calls:

let a;
a = console.log(a); // Reports unused a

let b;
console.log(b); // Doesn't report unused b

This behaviour is what caused me to believe that the intended effect of the rule was to report dead-stores. Maybe the scope of this rule should be reduced to match its description more strictly (only report if the variable is only ever declared or assigned but never read, always report at declaration, consider that being used in own right-hand of assignment is used), and make a separate rule for dead-stores that would catch a lot of the other issues that are discussed in this thread. The original improvement that you linked (#6348) would also be covered by a no-dead-stores rule.

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