Repository: eslint/eslint (js)

Issue:
no-unused-vars should report all unused function parameters that match the configured setting
I'm creating this as an attempt to refocus the discussion in #9750 into a specific proposal I have (although I think @g-plane would agree with me as well).

**What rule do you want to change?**

no-unused-vars

**Does this change cause the rule to produce more or fewer warnings?**

More

**How will the change be implemented? (New option, new default behavior, etc.)?**

New default behavior for `args: "after-used"` option: Report all unused parameters which occur after the last used parameter. This would be done as 1 lint message per unused parameter.

Note that this is not really a breaking change:
* When the user has no unused parameters after the last used parameter, the rule will continue not to report any violations.
* When the user has at least one unused parameter after this last used parameter, we currently report 1 parameter at a time.
    * When the user removes the unused parameter, we report the next unused parameter. Net effect for the user is to (eventually) see lint messages for all unused parameters after the last used parameter.
    * This proposal is just to report them all at once.

**Please provide some example code that this change will affect:**

```js
/* eslint no-unused-vars: ["error", { "args": "after-used" }] */
function (a, b, c, d, e) {
    c();
}
```

For more examples, see [demo](https://eslint.org/demo/#eyJ0ZXh0IjoiLyogZXNsaW50IG5vLXVudXNlZC12YXJzOltlcnJvciwgeyBhcmdzOiBhZnRlci11c2VkIH1dICovXHJcblxyXG5mdW5jdGlvbiBvbmUoYSwgYiwgYykge1xyXG4gICAgLy8gbm9uZSB1c2VkOiBTaG91bGQgcmVwb3J0IGEsIGIsIGFuZCBjIChldmVuIHdpdGggdGhlIFwiYWZ0ZXItdXNlZFwiIHNldHRpbmcpXHJcbn1cclxuXHJcbmZ1bmN0aW9uIHR3byhhLCBiLCBjLCBkLCBlKSB7XHJcbiAgICBkb1NvbWV0aGluZyhjKTtcclxuICAgIC8vIGEvYi9kL2UgdW51c2VkOiBTaG91bGQgcmVwb3J0IGEvYi9kL2UgaW4gXCJhbGxcIiBzZXR0aW5nLCBhbmQgZC9lIGluIFwiYWZ0ZXItdXNlZFwiIHNldHRpbmcpXHJcbn1cclxuXHJcbmZ1bmN0aW9uIHRocmVlKGEsIGIsIGMpIHtcclxuICAgIGRvU29tZXRoaW5nKGMpO1xyXG4gICAgLy8gYS9iIHVudXNlZDogU2hvdWxkIHJlcG9ydCBhL2IgaW4gXCJhbGxcIiBzZXR0aW5nLCBhbmQgc2hvdWxkIG5vdCByZXBvcnQgaW4gXCJhZnRlci11c2VkXCIgc2V0dGluZylcclxufVxyXG5cclxub25lKCk7XHJcbnR3bygpO1xyXG50aHJlZSgpOyIsIm9wdGlvbnMiOnsicGFyc2VyT3B0aW9ucyI6eyJlY21hVmVyc2lvbiI6Niwic291cmNlVHlwZSI6InNjcmlwdCIsImVjbWFGZWF0dXJlcyI6e319LCJydWxlcyI6e30sImVudiI6e319fQ==).

**What does the rule currently do for this code?**

For code block above: Reports that `e` is unused.

For demo: See lint results.

**What will the rule do after it's changed?**

For code block above: Reports that `d`, and `e` are all unused.

* `a`, `b`, `d`, and `e` are unused.
* But `d` and `e` are after the last used parameter (`c`).
* So report both `d` and `e`.

For demo: See comments in code.

Note: `args: "all"` works correctly and needs no changes.

--------

I'll champion.

If there's a strong reason for reporting only one of the unused arguments (and no, "the docs say so" doesn't count, that's circular reasoning), that can be expressed here as part of a :-1: to this proposal.

Pull request description:
Breaking: no-unused-vars reports all after-used params (fixes #9909)
<!--
    ESLint adheres to the [JS Foundation Code of Conduct](https://js.foundation/community/code-of-conduct).
-->

**What is the purpose of this pull request? (put an "X" next to item)**

[ ] Documentation update
[ ] Bug fix ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/bug-report.md))
[ ] New rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-proposal.md))
[x] Changes an existing rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-change-proposal.md))
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

<!--
    If the item you've checked above has a template, please paste the template questions below and answer them. (If this pull request is addressing an issue, you can just paste a link to the issue here instead.)
-->

See #9909.

<!--
    Please ensure your pull request is ready:

    - Read the pull request guide (https://eslint.org/docs/developer-guide/contributing/pull-requests)
    - Include tests for this change
    - Update documentation for this change (if appropriate)
-->

<!--
    The following is required for all pull requests:
-->

**What changes did you make? (Give an overview)**

The `no-unused-vars` rule was changed some time ago (likely unintentionally) to only report the last unused parameter when using the `args: "after-used"` setting. This resulted in poor UX in the case of a function with multiple unused parameters after the last used parameter, because ESLint would only report one parameter, and then report another parameter after the user fixed the first reported parameter.

This change simplifies the `args: "after-used"` logic by using this simple algorithm to check if a parameter is "after used parameters": If there exists at least one parameter (declared after the one being examined) which has at least one reference, then the parameter being examined is not "after the last used parameter" and so it shouldn't be reported. This approach allows for multiple parameters to be reported, as needed.

**Is there anything you'd like reviewers to focus on?**

* Do we need any new unit tests to cover destructuring cases that are not covered well enough right now?
* Any suggestions for improving the new description I've put in the documentation?

Patch:
diff --git a/docs/rules/no-unused-vars.md b/docs/rules/no-unused-vars.md
index d7502ed5c..996dea181 100644
--- a/docs/rules/no-unused-vars.md
+++ b/docs/rules/no-unused-vars.md
@@ -150,7 +150,7 @@ console.log(secondVar);
 
 The `args` option has three settings:
 
-* `after-used` - only the last argument must be used. This allows you, for instance, to have two named parameters to a function and as long as you use the second argument, ESLint will not warn you about the first. This is the default setting.
+* `after-used` - unused positional arguments that occur before the last used argument will not be checked, but all named arguments and all positional arguments after the last used argument will be checked.
 * `all` - all named arguments must be used.
 * `none` - do not check arguments.
 
@@ -161,9 +161,10 @@ Examples of **incorrect** code for the default `{ "args": "after-used" }` option
 ```js
 /*eslint no-unused-vars: ["error", { "args": "after-used" }]*/
 
-// 1 error
+// 2 errors, for the parameters after the last used parameter (bar)
 // "baz" is defined but never used
-(function(foo, bar, baz) {
+// "qux" is defined but never used
+(function(foo, bar, baz, qux) {
     return bar;
 })();
 ```
@@ -173,8 +174,8 @@ Examples of **correct** code for the default `{ "args": "after-used" }` option:
 ```js
 /*eslint no-unused-vars: ["error", {"args": "after-used"}]*/
 
-(function(foo, bar, baz) {
-    return baz;
+(function(foo, bar, baz, qux) {
+    return qux;
 })();
 ```
 
diff --git a/lib/rules/no-unused-vars.js b/lib/rules/no-unused-vars.js
index 6ba573498..0d5194bc8 100644
--- a/lib/rules/no-unused-vars.js
+++ b/lib/rules/no-unused-vars.js
@@ -457,30 +457,19 @@ module.exports = {
         }
 
         /**
-         * Checks whether the given variable is the last parameter in the non-ignored parameters.
+         * Checks whether the given variable is after the last used parameter.
          *
          * @param {eslint-scope.Variable} variable - The variable to check.
-         * @returns {boolean} `true` if the variable is the last.
+         * @returns {boolean} `true` if the variable is defined after the last
+         * used parameter.
          */
-        function isLastInNonIgnoredParameters(variable) {
+        function isAfterLastUsedArg(variable) {
             const def = variable.defs[0];
+            const params = context.getDeclaredVariables(def.node);
+            const posteriorParams = params.slice(params.indexOf(variable) + 1);
 
-            // This is the last.
-            if (def.index === def.node.params.length - 1) {
-                return true;
-            }
-
-            // if all parameters preceded by this variable are ignored and unused, this is the last.
-            if (config.argsIgnorePattern) {
-                const params = context.getDeclaredVariables(def.node);
-                const posteriorParams = params.slice(params.indexOf(variable) + 1);
-
-                if (posteriorParams.every(v => v.references.length === 0 && config.argsIgnorePattern.test(v.name))) {
-                    return true;
-                }
-            }
-
-            return false;
+            // If any used parameters occur after this parameter, do not report.
+            return !posteriorParams.some(v => v.references.length > 0);
         }
 
         /**
@@ -549,8 +538,8 @@ module.exports = {
                                 continue;
                             }
 
-                            // if "args" option is "after-used", skip all but the last parameter
-                            if (config.args === "after-used" && astUtils.isFunction(def.name.parent) && !isLastInNonIgnoredParameters(variable)) {
+                            // if "args" option is "after-used", skip used variables
+                            if (config.args === "after-used" && astUtils.isFunction(def.name.parent) && !isAfterLastUsedArg(variable)) {
                                 continue;
                             }
                         } else {


Test patch:
diff --git a/tests/lib/rules/no-unused-vars.js b/tests/lib/rules/no-unused-vars.js
index 3f529aa39..f188f8ce3 100644
--- a/tests/lib/rules/no-unused-vars.js
+++ b/tests/lib/rules/no-unused-vars.js
@@ -596,25 +596,59 @@ ruleTester.run("no-unused-vars", rule, {
         {
             code: "(function(a, b, c) {})",
             options: [{ argsIgnorePattern: "c" }],
-            errors: [{ message: "'b' is defined but never used. Allowed unused args must match /c/." }]
+            errors: [
+                {
+                    message: "'a' is defined but never used. Allowed unused args must match /c/."
+                },
+                {
+                    message: "'b' is defined but never used. Allowed unused args must match /c/."
+                }
+            ]
         },
         {
             code: "(function(a, b, {c, d}) {})",
             options: [{ argsIgnorePattern: "[cd]" }],
             parserOptions: { ecmaVersion: 6 },
-            errors: [{ message: "'b' is defined but never used. Allowed unused args must match /[cd]/." }]
+            errors: [
+                {
+                    message: "'a' is defined but never used. Allowed unused args must match /[cd]/."
+                },
+                {
+                    message: "'b' is defined but never used. Allowed unused args must match /[cd]/."
+                }
+            ]
         },
         {
             code: "(function(a, b, {c, d}) {})",
             options: [{ argsIgnorePattern: "c" }],
             parserOptions: { ecmaVersion: 6 },
-            errors: [{ message: "'d' is defined but never used. Allowed unused args must match /c/." }]
+            errors: [
+                {
+                    message: "'a' is defined but never used. Allowed unused args must match /c/."
+                },
+                {
+                    message: "'b' is defined but never used. Allowed unused args must match /c/."
+                },
+                {
+                    message: "'d' is defined but never used. Allowed unused args must match /c/."
+                }
+            ]
         },
         {
             code: "(function(a, b, {c, d}) {})",
             options: [{ argsIgnorePattern: "d" }],
             parserOptions: { ecmaVersion: 6 },
-            errors: [{ message: "'c' is defined but never used. Allowed unused args must match /d/." }]
+            errors: [
+                {
+                    message: "'a' is defined but never used. Allowed unused args must match /d/."
+                },
+                {
+                    message: "'b' is defined but never used. Allowed unused args must match /d/."
+                },
+                {
+                    message: "'c' is defined but never used. Allowed unused args must match /d/."
+                }
+            ]
         },
         {
             code: "/*global\rfoo*/",