Merge pull request eslint#3664 from mysticatea/no-unused-vars/fix-loc…

…ations-of-global Fix: `no-unused-vars` had not been showing correct locations for `/*global`
EnableSoftware · Sep 5, 2015 · 2551594 · 2551594
2 parents 7877229 + fe3a047
commit 2551594
Show file tree

Hide file tree

Showing 4 changed files with 95 additions and 7 deletions.
diff --git a/lib/eslint.js b/lib/eslint.js
@@ -46,9 +46,10 @@ estraverse.VisitorKeys.Property.push("argument");
  * Parses a list of "name:boolean_value" or/and "name" options divided by comma or
  * whitespace.
  * @param {string} string The string to parse.
+ * @param {Comment} comment The comment node which has the string.
  * @returns {Object} Result map object of names and boolean values
  */
-function parseBooleanConfig(string) {
+function parseBooleanConfig(string, comment) {
     var items = {};
     // Collapse whitespace around : to make parsing easier
     string = string.replace(/\s*:\s*/g, ":");
@@ -65,7 +66,10 @@ function parseBooleanConfig(string) {
             name = name.substring(0, pos);
         }
 
-        items[name] = (value === "true");
+        items[name] = {
+            value: (value === "true"),
+            comment: comment
+        };
 
     });
     return items;
@@ -181,9 +185,10 @@ function addDeclaredGlobals(program, globalScope, config) {
         if (!variable) {
             variable = new escope.Variable(name, globalScope);
             variable.eslintExplicitGlobal = true;
+            variable.eslintExplicitGlobalComment = explicitGlobals[name].comment;
             globalScope.variables.push(variable);
         }
-        variable.writeable = explicitGlobals[name];
+        variable.writeable = explicitGlobals[name].value;
     });
 
     // mark all exported variables as such
@@ -290,12 +295,12 @@ function modifyConfigsFromComments(filename, ast, config, reportingConfig, messa
             if (comment.type === "Block") {
                 switch (match[1]) {
                     case "exported":
-                        assign(commentConfig.exported, parseBooleanConfig(value));
+                        assign(commentConfig.exported, parseBooleanConfig(value, comment));
                         break;
 
                     case "globals":
                     case "global":
-                        assign(commentConfig.astGlobals, parseBooleanConfig(value));
+                        assign(commentConfig.astGlobals, parseBooleanConfig(value, comment));
                         break;
 
                     case "eslint-env":

diff --git a/lib/rules/no-unused-vars.js b/lib/rules/no-unused-vars.js
@@ -5,6 +5,12 @@
 
 "use strict";
 
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+var escape = require("escape-string-regexp");
+
 //------------------------------------------------------------------------------
 // Rule Definition
 //------------------------------------------------------------------------------
@@ -227,6 +233,35 @@ module.exports = function(context) {
         return unusedVars;
     }
 
+    /**
+     * Creates the correct location of a given variables.
+     * The location is at its name string in a `/*global` comment.
+     *
+     * @param {escope.Variable} variable - A variable to get its location.
+     * @returns {{line: number, column: number}} The location object for the variable.
+     */
+    function getLocation(variable) {
+        var comment = variable.eslintExplicitGlobalComment;
+        var content = comment.value;
+        var baseLoc = comment.loc.start;
+        var namePattern = new RegExp("\\b" + escape(variable.name) + "\\b");
+        var column = namePattern.exec(content).index;
+        var prefix = content.slice(0, column);
+        var lineInComment = (prefix.match(/\n/g) || []).length;
+
+        if (lineInComment > 0) {
+            column -= 1 + prefix.lastIndexOf("\n");
+        } else {
+            // 2 is for `/*`
+            column += baseLoc.column + 2;
+        }
+
+        return {
+            line: baseLoc.line + lineInComment,
+            column: column
+        };
+    }
+
     //--------------------------------------------------------------------------
     // Public
     //--------------------------------------------------------------------------
@@ -243,7 +278,7 @@ module.exports = function(context) {
                 if (unusedVar.eslintUsed) {
                     continue; // explicitly exported variables
                 } else if (unusedVar.eslintExplicitGlobal) {
-                    context.report(programNode, MESSAGE, unusedVar);
+                    context.report(programNode, getLocation(unusedVar), MESSAGE, unusedVar);
                 } else if (unusedVar.defs.length > 0) {
                     context.report(unusedVar.identifiers[0], MESSAGE, unusedVar);
                 }

diff --git a/tests/lib/eslint.js b/tests/lib/eslint.js
@@ -2252,6 +2252,38 @@ describe("eslint", function() {
             var messages = eslint.verify("/* eslint-env node */ return;", null, "eslint-env node");
             assert.equal(messages.length, 0);
         });
+
+        it("should attach a \"/*global\" comment node to declared variables", function() {
+            var code = "/* global foo */\n/* global bar, baz */";
+            var ok = false;
+
+            eslint.defineRules({test: function(context) {
+                return {
+                    "Program": function() {
+                        var scope = context.getScope();
+                        var comments = context.getAllComments();
+                        assert.equal(2, comments.length);
+
+                        var foo = getVariable(scope, "foo");
+                        assert.equal(true, foo.eslintExplicitGlobal);
+                        assert.equal(comments[0], foo.eslintExplicitGlobalComment);
+
+                        var bar = getVariable(scope, "bar");
+                        assert.equal(true, bar.eslintExplicitGlobal);
+                        assert.equal(comments[1], bar.eslintExplicitGlobalComment);
+
+                        var baz = getVariable(scope, "baz");
+                        assert.equal(true, baz.eslintExplicitGlobal);
+                        assert.equal(comments[1], baz.eslintExplicitGlobalComment);
+
+                        ok = true;
+                    }
+                };
+            }});
+
+            eslint.verify(code, {rules: {test: 2}});
+            assert(ok);
+        });
     });
 
     describe("getDeclaredVariables(node)", function() {

diff --git a/tests/lib/rules/no-unused-vars.js b/tests/lib/rules/no-unused-vars.js
@@ -168,6 +168,22 @@ ruleTester.run("no-unused-vars", rule, {
         { code: "var a; function foo() { var _b; var c_; } foo();", options: [ { vars: "local", varsIgnorePattern: "^_" } ], errors: [{ message: "\"c_\" is defined but never used", line: 1, column: 37 }] },
         { code: "function foo(a, _b) { } foo();", options: [ { args: "all", argsIgnorePattern: "^_" } ], errors: [{ message: "\"a\" is defined but never used", line: 1, column: 14 }] },
         { 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;", ecmaFeatures: {destructuring: true}, options: [ { vars: "all", varsIgnorePattern: "[iI]gnored" } ], errors: [{ message: "\"secondItem\" is defined but never used", line: 1, column: 25 }] }
+        { code: "var [ firstItemIgnored, secondItem ] = items;", ecmaFeatures: {destructuring: true}, options: [ { vars: "all", varsIgnorePattern: "[iI]gnored" } ], errors: [{ message: "\"secondItem\" is defined but never used", line: 1, column: 25 }] },
+
+        // https://github.com/eslint/eslint/issues/3617
+        {
+            code: "\n/* global foobar, foo, bar */\nfoobar;",
+            errors: [
+                {line: 2, column: 19, message: "\"foo\" is defined but never used" },
+                {line: 2, column: 24, message: "\"bar\" is defined but never used" }
+            ]
+        },
+        {
+            code: "\n/* global foobar,\n   foo,\n   bar\n */\nfoobar;",
+            errors: [
+                {line: 3, column: 4, message: "\"foo\" is defined but never used" },
+                {line: 4, column: 4, message: "\"bar\" is defined but never used" }
+            ]
+        }
     ]
 });