no-unused-vars is incorrectly raised when function name matches parameter name

[sokra]

The code snippet:

function a(a) { return a; }
a();

incorrectly complain about the parameter a being unused. It seem to prefer the reference to the funcion name instead of the parameter.

Propably caused by the order in block-scoped-var.js.

[nzakas]

Thanks, we'll take a look.

[jrvidal]

I think the a reference in the return a statement is being incorrectly classified as the function name by isSelfReference. It looks like it doesn't account for shadowing.

Would it make sense to extract the shadowing checks as a common tool?

[nzakas]

Perhaps. We do have a lot of repeating logic across rules, not just related to scoping. I'm definitely open to suggestions to eliminate duplicate code.

[jrvidal]

I've been digging a little bit about this one. Some previous discussions can be found in #2095, df9b254.

Here are a few test cases related to no-unused-vars and recursive functions:

1. The original report:

   "use strict";
   
   function a(a) {
   // Incorrectly reports the parameter 'a' as unused
       return a;
   }
   
   a(1); // 1

2. An analogous case with an inner function:

   "use strict";
   
   function a() {
       // Incorrectly reports the 'a' inner function as unused
       function a() {
           return 1;
       }
   
       return a();
   }
   
   a(); // 1

3. If a function expression is used, it only gets reported if it's named:

   "use strict";
   
   var a = function () {
       // OK, not reported
       var a = 1;
   
       return a;
   };
   
   var b = function b() {
       // Incorrectly reports 'b' as unused
       var b = 1;
   
       return b;
   };
   
   var c = function d() {
       // Incorrectly reports 'd' as unused
       var d = 1;
   
       return d;
   };
   
   a(); // 1
   b(); // 1
   c(); // 1

4. On the other hand, function expressions are not checked for self-reference:

   "use strict";
   
   // Incorrectly? considers 'a' as used
   var a = function () {
       return a();
   };

5. Recursive function declarations are not always detected either:

   "use strict";
   
   // Incorrectly? considers 'foo' as used
   function foo() {
       return function inner() {
               return foo();
       };
   }

For now, I'm trying to find a fix for the "false positives" in 1, 2 and 3. I am not sure whether detecting all "missed negatives" (undetected unused vars) statically is even feasible/desirable.

[jrvidal]

Actually there are tests that consider case 5 above as valid. Should we consider changing this behavior?

[nzakas]

The recursive stuff is hard to track down, especially in the case of anonymous functions. I'd say start with the first three cases and focus on getting those fixed.