feat!: Close var and function references on global scope

Author: mdjermanovic

packages/eslint-scope/lib/scope.js

@@ -122,18 +122,6 @@ function registerScope(scopeManager, scope) {
     }
 }
 
-/**
- * Should be statically
- * @param {Object} def def
- * @returns {boolean} should be statically
- */
-function shouldBeStatically(def) {
-    return (
-        (def.type === Variable.ClassName) ||
-        (def.type === Variable.Variable && def.parent.kind !== "var")
-    );
-}
-
 /**
  * @constructor Scope
  */
@@ -267,22 +255,7 @@ class Scope {
     }
 
     __shouldStaticallyClose(scopeManager) {
-        return (!this.dynamic || scopeManager.__isOptimistic());
-    }
-
-    __shouldStaticallyCloseForGlobal(ref) {
-
-        // On global scope, let/const/class declarations should be resolved statically.
-        const name = ref.identifier.name;
-
-        if (!this.set.has(name)) {
-            return false;
-        }
-
-        const variable = this.set.get(name);
-        const defs = variable.defs;
-
-        return defs.length > 0 && defs.every(shouldBeStatically);
+        return (!this.dynamic || scopeManager.__isOptimistic() || this.type === "global");
     }
 
     __staticCloseRef(ref) {
@@ -302,26 +275,13 @@ class Scope {
         } while (current);
     }
 
-    __globalCloseRef(ref) {
-
-        // let/const/class declarations should be resolved statically.
-        // others should be resolved dynamically.
-        if (this.__shouldStaticallyCloseForGlobal(ref)) {
-            this.__staticCloseRef(ref);
-        } else {
-            this.__dynamicCloseRef(ref);
-        }
-    }
-
     __close(scopeManager) {
         let closeRef;
 
         if (this.__shouldStaticallyClose(scopeManager)) {
             closeRef = this.__staticCloseRef;
-        } else if (this.type !== "global") {
-            closeRef = this.__dynamicCloseRef;
         } else {
-            closeRef = this.__globalCloseRef;
+            closeRef = this.__dynamicCloseRef;
         }
 
         // Try Resolving all references in this scope.
@@ -560,9 +520,11 @@ class GlobalScope extends Scope {
 
         }
 
-        this.implicit.left = this.__left;
+        super.__close(scopeManager);
+
+        this.implicit.left = [...this.through];
 
-        return super.__close(scopeManager);
+        return null;
     }
 
     __defineImplicit(node, def) {

packages/eslint-scope/tests/implicit-global-reference.test.js

@@ -44,6 +44,8 @@ describe("implicit global reference", () => {
         );
 
         expect(scopes[0].implicit.variables.map(variable => variable.name)).to.be.eql([]);
+        expect(scopes[0].implicit.left.map(reference => reference.identifier.name)).to.be.eql([]);
+        expect(scopes[0].through.map(reference => reference.identifier.name)).to.be.eql([]);
     });
 
     it("assignments global scope without definition", () => {
@@ -66,6 +68,18 @@ describe("implicit global reference", () => {
                 "x"
             ]
         );
+        expect(scopes[0].implicit.left.map(reference => reference.identifier.name)).to.be.eql(
+            [
+                "x",
+                "x"
+            ]
+        );
+        expect(scopes[0].through.map(reference => reference.identifier.name)).to.be.eql(
+            [
+                "x",
+                "x"
+            ]
+        );
     });
 
     it("assignments global scope without definition eval", () => {
@@ -120,6 +134,16 @@ describe("implicit global reference", () => {
                 "x"
             ]
         );
+        expect(scopes[0].implicit.left.map(reference => reference.identifier.name)).to.be.eql(
+            [
+                "x"
+            ]
+        );
+        expect(scopes[0].through.map(reference => reference.identifier.name)).to.be.eql(
+            [
+                "x"
+            ]
+        );
     });
 
     it("assignment doesn't leak", () => {
@@ -151,6 +175,8 @@ describe("implicit global reference", () => {
         );
 
         expect(scopes[0].implicit.variables.map(variable => variable.name)).to.be.eql([]);
+        expect(scopes[0].implicit.left.map(reference => reference.identifier.name)).to.be.eql([]);
+        expect(scopes[0].through.map(reference => reference.identifier.name)).to.be.eql([]);
     });
 
     it("for-in-statement leaks", () => {
@@ -177,6 +203,18 @@ describe("implicit global reference", () => {
                 "x"
             ]
         );
+        expect(scopes[0].implicit.left.map(reference => reference.identifier.name)).to.be.eql(
+            [
+                "x",
+                "y"
+            ]
+        );
+        expect(scopes[0].through.map(reference => reference.identifier.name)).to.be.eql(
+            [
+                "x",
+                "y"
+            ]
+        );
     });
 
     it("for-in-statement doesn't leaks", () => {
@@ -208,5 +246,15 @@ describe("implicit global reference", () => {
         );
 
         expect(scopes[0].implicit.variables.map(variable => variable.name)).to.be.eql([]);
+        expect(scopes[0].implicit.left.map(reference => reference.identifier.name)).to.be.eql(
+            [
+                "y"
+            ]
+        );
+        expect(scopes[0].through.map(reference => reference.identifier.name)).to.be.eql(
+            [
+                "y"
+            ]
+        );
     });
 });

packages/eslint-scope/tests/label.test.js

@@ -66,9 +66,10 @@ describe("label", () => {
         expect(globalScope.type).to.be.equal("global");
         expect(globalScope.variables).to.have.length(1);
         expect(globalScope.variables[0].name).to.be.equal("foo");
-        expect(globalScope.through.length).to.be.equal(3);
-        expect(globalScope.through[2].identifier.name).to.be.equal("foo");
-        expect(globalScope.through[2].isRead()).to.be.true;
+        expect(globalScope.through.length).to.be.equal(1);
+        expect(globalScope.through[0].identifier.name).to.be.equal("console");
+        expect(globalScope.variables[0].references.length).to.be.equal(2);
+        expect(globalScope.variables[0].references[1].isRead()).to.be.true;
     });
 });
 

packages/eslint-scope/tests/references.test.js

@@ -155,7 +155,7 @@ describe("References:", () => {
     });
 
     describe("When there is a `var` declaration on global,", () => {
-        it("the reference on global should NOT be resolved.", () => {
+        it("the reference on global should be resolved.", () => {
             const ast = espree("var a = 0;");
 
             const scopeManager = analyze(ast, { ecmaVersion: 6 });
@@ -171,13 +171,13 @@ describe("References:", () => {
 
             expect(reference.from).to.equal(scope);
             expect(reference.identifier.name).to.equal("a");
-            expect(reference.resolved).to.be.null;
+            expect(reference.resolved).to.equal(scope.variables[0]);
             expect(reference.writeExpr).to.not.be.undefined;
             expect(reference.isWrite()).to.be.true;
             expect(reference.isRead()).to.be.false;
         });
 
-        it("the reference in functions should NOT be resolved.", () => {
+        it("the reference in functions should be resolved.", () => {
             const ast = espree(`
                 var a = 0;
                 function foo() {
@@ -198,15 +198,15 @@ describe("References:", () => {
 
             expect(reference.from).to.equal(scope);
             expect(reference.identifier.name).to.equal("a");
-            expect(reference.resolved).to.be.null;
+            expect(reference.resolved).to.equal(scopeManager.scopes[0].variables[0]);
             expect(reference.writeExpr).to.be.undefined;
             expect(reference.isWrite()).to.be.false;
             expect(reference.isRead()).to.be.true;
         });
     });
 
     describe("When there is a `function` declaration on global,", () => {
-        it("the reference on global should NOT be resolved.", () => {
+        it("the reference on global should be resolved.", () => {
             const ast = espree(`
                 function a() {}
                 a();
@@ -225,13 +225,13 @@ describe("References:", () => {
 
             expect(reference.from).to.equal(scope);
             expect(reference.identifier.name).to.equal("a");
-            expect(reference.resolved).to.be.null;
+            expect(reference.resolved).to.equal(scope.variables[0]);
             expect(reference.writeExpr).to.be.undefined;
             expect(reference.isWrite()).to.be.false;
             expect(reference.isRead()).to.be.true;
         });
 
-        it("the reference in functions should NOT be resolved.", () => {
+        it("the reference in functions should be resolved.", () => {
             const ast = espree(`
                 function a() {}
                 function foo() {
@@ -252,7 +252,7 @@ describe("References:", () => {
 
             expect(reference.from).to.equal(scope);
             expect(reference.identifier.name).to.equal("a");
-            expect(reference.resolved).to.be.null;
+            expect(reference.resolved).to.equal(scopeManager.scopes[0].variables[0]);
             expect(reference.writeExpr).to.be.undefined;
             expect(reference.isWrite()).to.be.false;
             expect(reference.isRead()).to.be.true;