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

Applied only for `for-in` loops with only one `return` statement.

Also clean jsdocs for internal helpers

lib/rules/no-unused-vars.js

@@ -124,7 +124,7 @@ module.exports = {
         /**
          * Determines if a reference is a read operation.
          * @param {Reference} ref - An escope Reference
-         * @returns {Boolean} whether the given reference represents a read operation
+         * @returns {boolean} whether the given reference represents a read operation
          * @private
          */
         function isReadRef(ref) {
@@ -219,11 +219,48 @@ module.exports = {
             );
         }
 
+        /**
+         * Determine if an identifier is used either in for-in loops.
+         *
+         * @param {Reference} ref - The reference to check.
+         * @returns {boolean} whether reference is used in the for-in loops
+         * @private
+         */
+        function isForInRef(ref) {
+            var target = ref.identifier.parent;
+
+
+            // "for (var ...) { return; }"
+            if (target.type === "VariableDeclarator") {
+                target = target.parent.parent;
+            }
+
+            if (target.type !== "ForInStatement") {
+                return false;
+            }
+
+            // "for (...) { return; }"
+            if (target.body.type === "BlockStatement") {
+                target = target.body.body[0];
+
+            // "for (...) return;"
+            } else {
+                target = target.body;
+            }
+
+            // For empty loop body
+            if (!target) {
+                return false;
+            }
+
+            return target.type === "ReturnStatement";
+        }
+
         /**
          * Determines if the variable is used.
          * @param {Variable} variable - The variable to check.
-         * @param {Reference[]} references - The variable references to check.
          * @returns {boolean} True if the variable is used
+         * @private
          */
         function isUsedVariable(variable) {
             var functionNodes = variable.defs.filter(function(def) {
@@ -235,6 +272,10 @@ module.exports = {
                 rhsNode = null;
 
             return variable.references.some(function(ref) {
+                if (isForInRef(ref)) {
+                    return true;
+                }
+
                 var forItself = isReadForItself(ref, rhsNode);
 
                 rhsNode = getRhsNode(ref, rhsNode);
@@ -344,6 +385,7 @@ module.exports = {
          * @param {escope.Variable} variable - A variable to get.
          * @param {ASTNode} comment - A comment node which includes the variable name.
          * @returns {number} The index of the variable name's location.
+         * @private
          */
         function getColumnInComment(variable, comment) {
             var namePattern = new RegExp("[\\s,]" + lodash.escapeRegExp(variable.name) + "(?:$|[\\s,:])", "g");
@@ -363,6 +405,7 @@ module.exports = {
          *
          * @param {escope.Variable} variable - A variable to get its location.
          * @returns {{line: number, column: number}} The location object for the variable.
+         * @private
          */
         function getLocation(variable) {
             var comment = variable.eslintExplicitGlobalComment;

tests/lib/rules/no-unused-vars.js

@@ -125,6 +125,20 @@ ruleTester.run("no-unused-vars", rule, {
         { code: "function foo(a, _b) { return a; } foo();", options: [ { args: "after-used", argsIgnorePattern: "^_" } ] },
         { code: "var [ firstItemIgnored, secondItem ] = items;\nconsole.log(secondItem);", parserOptions: { ecmaVersion: 6 }, options: [ { vars: "all", varsIgnorePattern: "[iI]gnored" } ] },
 
+        // for-in loops (see #2342)
+        "(function(obj) { var name; for ( name in obj ) return; })({});",
+        "(function(obj) { var name; for ( name in obj ) { return; } })({});",
+        "(function(obj) { for ( var name in obj ) { return true } })({})",
+        "(function(obj) { for ( var name in obj ) return true })({})",
+
+        { code: "(function(obj) { let name; for ( name in obj ) return; })({});", parserOptions: { ecmaVersion: 6 }},
+        { code: "(function(obj) { let name; for ( name in obj ) { return; } })({});", parserOptions: { ecmaVersion: 6 }},
+        { code: "(function(obj) { for ( let name in obj ) { return true } })({})", parserOptions: { ecmaVersion: 6 }},
+        { code: "(function(obj) { for ( let name in obj ) return true })({})", parserOptions: { ecmaVersion: 6 }},
+
+        { code: "(function(obj) { for ( const name in obj ) { return true } })({})", parserOptions: { ecmaVersion: 6 }},
+        { code: "(function(obj) { for ( const name in obj ) return true })({})", parserOptions: { ecmaVersion: 6 }},
+
         // caughtErrors
         {
             code: "try{}catch(err){console.error(err);}",
@@ -198,6 +212,11 @@ ruleTester.run("no-unused-vars", rule, {
         { code: "function foo(a, _b, c) { return a; } foo();", options: [ { args: "after-used", argsIgnorePattern: "^_" } ], errors: [{ message: "'c' is defined but never used", line: 1, column: 21 }] },
         { code: "var [ firstItemIgnored, secondItem ] = items;", parserOptions: { ecmaVersion: 6 }, options: [ { vars: "all", varsIgnorePattern: "[iI]gnored" } ], errors: [{ message: "'secondItem' is defined but never used", line: 1, column: 25 }] },
 
+        // for-in loops (see #2342)
+        { code: "(function(obj) { var name; for ( name in obj ) { i(); return; } })({});", errors: [{ message: "'name' is defined but never used", line: 1, column: 22 }] },
+        { code: "(function(obj) { var name; for ( name in obj ) { } })({});", errors: [{ message: "'name' is defined but never used", line: 1, column: 22 }] },
+        { code: "(function(obj) { for ( var name in obj ) { } })({});", errors: [{ message: "'name' is defined but never used", line: 1, column: 28 }] },
+
         // https://github.com/eslint/eslint/issues/3617
         {
             code: "\n/* global foobar, foo, bar */\nfoobar;",