feat: get correct scope ref from ThisExpression, fix #2

motiz88 · Sep 11, 2016 · daf8df5 · daf8df5
1 parent a38554f
commit daf8df5
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 17 deletions.
diff --git a/src/index.js b/src/index.js
@@ -4,11 +4,11 @@ import type { NodePath, Scope } from 'babel-traverse';
 
 class InnerScopeVisitor {
   referencedScopes: Scope[];
-  seenThisExpression: boolean;
 
   ReferencedIdentifier = (path: NodePath) => {
     const binding = path.scope.getBinding(path.node.name);
     if (binding) {
+      // istanbul ignore next: could be initialized elsewhere
       if (!this.referencedScopes) {
         this.referencedScopes = [];
       }
@@ -17,7 +17,18 @@ class InnerScopeVisitor {
   }
 
   ThisExpression = (path: NodePath) => {
-    this.seenThisExpression = true;
+    let {scope} = path;
+    while (scope && (scope = scope.getFunctionParent())) { // eslint-disable-line no-cond-assign
+      if (!scope.path.isArrowFunctionExpression()) {
+        // istanbul ignore next: could be initialized elsewhere
+        if (!this.referencedScopes) {
+          this.referencedScopes = [];
+        }
+        this.referencedScopes.push(scope);
+        return;
+      }
+      scope = scope.parent;
+    }
   }
 }
 
@@ -61,7 +72,6 @@ export default function ({types: t, template}: {types: BabelTypes, template: Bab
         // or the global scope.
         const innerScope = new InnerScopeVisitor();
         path.traverse(innerScope);
-        if (innerScope.seenThisExpression && t.isArrowFunctionExpression(path.node)) return;
         const referencedScopes = uniqueScopes(innerScope.referencedScopes || []);
         const targetScope = deepestScopeOf(
           path,

diff --git a/test/fixtures/handle-references-to-this/actual.js b/test/fixtures/handle-references-to-this/actual.js
@@ -11,10 +11,13 @@ class A {
 }
 
 (function A () {
-  // FIXME: We are too conservative regarding "this" in arrow functions.
-  // The arrow func here is not hoisted due to the embedded ThisExpression,
-  // but it's actually safe because A's "this" is not referenced.
-  // This is made even more apparent by the fact that B itself is (correctly)
-  // hoisted, taking the ThisExpression along with it.
-  return () => function B () {return this};
+  // NOTE: hoisted
+  return () =>
+    // NOTE: hoisted
+    function B () {
+      return this;
+    };
 })();
+
+// NOTE: not hoisted
+() => this;
diff --git a/test/fixtures/handle-references-to-this/expected.js b/test/fixtures/handle-references-to-this/expected.js
@@ -1,3 +1,5 @@
+var _this = this;
+
 (function () {
   // NOTE: not hoisted
   return () => this;
@@ -10,15 +12,18 @@ class A {
   }
 }
 
-var _B = function B() {
+var
+// NOTE: hoisted
+_B = function B() {
   return this;
 };
 
+var _hoistedAnonymousFunc2 = () => _B;
+
 (function A() {
-  // FIXME: We are too conservative regarding "this" in arrow functions.
-  // The arrow func here is not hoisted due to the embedded ThisExpression,
-  // but it's actually safe because A's "this" is not referenced.
-  // This is made even more apparent by the fact that B itself is (correctly)
-  // hoisted, taking the ThisExpression along with it.
-  return () => _B;
-})();
+  // NOTE: hoisted
+  return _hoistedAnonymousFunc2;
+})();
+
+// NOTE: not hoisted
+() => _this;