Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: Resolve references of global variables before start traversing. #4615

Closed
mysticatea opened this Issue Dec 5, 2015 · 11 comments

Comments

Projects
None yet
5 participants
@mysticatea
Copy link
Member

mysticatea commented Dec 5, 2015

Currently, references of global variables are failed to resolve.
So some rules need a fallback for references.

I felt those fallback is hateful.
So I'd like to propose that it resolves references of global variables the end of this function.

As a result, we can remove those fallback and simplify the logic.

If this is accepted, I will work on this.

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Dec 5, 2015

@mysticatea Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 5, 2015

@mysticatea Can you explain what exactly do you mean by "resolve references"? Add global variable names to the list? Or something more?

@mysticatea

This comment has been minimized.

Copy link
Member Author

mysticatea commented Dec 5, 2015

It's to make Variable#references, Reference#resolved, and Scope#through properties correctly.
Currently,

  • Variable#references property of global variables are an empty array.
  • Reference#resolved property of global variables are null.
  • Scope#through of the global scope contains existing variables'.
@mysticatea

This comment has been minimized.

Copy link
Member Author

mysticatea commented Dec 6, 2015

FYI, codes of rules which are using references reduced about: master...mysticatea:core/resolve-references-of-global
Added main code is the first 12 lines.

But this might be breaking plugin rule behaviors if it's using Scope#through or Reference#resolved for global variables. Actually, no-alert, no-eval, and no-native-reassign had needed to modify to follow this change.

However, I believe this change helps to make and maintain rules which uses references.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Dec 6, 2015

@mysticatea escope purposely omits globals from its analysis because their references cannot be known statically.

@mysticatea

This comment has been minimized.

Copy link
Member Author

mysticatea commented Dec 6, 2015

@michaelficarra And eslint is adding many global variables after escope's analysis :)

@nzakas nzakas removed the triage label Dec 6, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 6, 2015

@mysticatea are you just talking about globals added using /* global */ and in a config file? Can you show an example of the problem?

@mysticatea

This comment has been minimized.

Copy link
Member Author

mysticatea commented Dec 7, 2015

@nzakas I'm taking about "all references that can resolve but are not resolved". It contains /* global */', config files', var declarations, and function declarations. In short, I want to remove the special fallback for unresolved references. Several rules have the same fallback, I think it's redundant duplication.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 8, 2015

I see. How would you expose this information to rules?

@mysticatea

This comment has been minimized.

Copy link
Member Author

mysticatea commented Dec 8, 2015

My plan is:

diff --git a/lib/eslint.js b/lib/eslint.js
index 480f413..96138e4 100755
--- a/lib/eslint.js
+++ b/lib/eslint.js
@@ -173,6 +173,18 @@ function addDeclaredGlobals(program, globalScope, config) {
             variable.eslintUsed = true;
         }
     });
+
+    // Re-resolves references.
+    globalScope.through = globalScope.through.filter(function(reference) {
+        var name = reference.identifier.name;
+        var variable = globalScope.set.get(name);
+        if (variable) {
+            reference.resolved = variable;
+            variable.references.push(reference);
+            return false;
+        }
+        return true;
+    });
 }

 /**

Reference object and Variable object have references each other. Reference#resolved property is a Variable object which the reference object is pointing. Variable#references property is an array of Reference objects which are pointing the variable. Several rules are using this information.

But, the reason the rules need the fallback is that there are cases Reference#resolved and Variable#references is invalid value. Global variables of /*global*/ comments, config files, var declarations, or function declarations have always an empty array in Variable#references. And references of those variables have always null in Reference#resolved. So rules need the fallback for Variable#references and Reference#resolved.

So this proposal is to set up Variable#references and Reference#resolved with correct value before rules use those.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 8, 2015

Got it. 👍

@nzakas nzakas added accepted and removed evaluating labels Dec 8, 2015

@mysticatea mysticatea self-assigned this Dec 9, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Dec 9, 2015

mysticatea added a commit to mysticatea/eslint that referenced this issue Dec 10, 2015

nzakas added a commit that referenced this issue Dec 11, 2015

Merge pull request #4675 from eslint/pr4648
Breaking: Correct links between variables and references (fixes #4615)

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.