Fix duplicated assertThisInitialized calls in constructors

babel · Feb 5, 2019 · 9a9e428 · 9a9e428
1 parent 65cbbc1
commit 9a9e428
Show file tree

Hide file tree

Showing 6 changed files with 43 additions and 61 deletions.
diff --git a/packages/babel-plugin-transform-classes/src/transformClass.js b/packages/babel-plugin-transform-classes/src/transformClass.js
@@ -32,25 +32,6 @@ const verifyConstructorVisitor = traverse.visitors.merge([
         );
       }
     },
-
-    ThisExpression(path, state) {
-      if (!state.isDerived) return;
-
-      const { node, parentPath } = path;
-      if (parentPath.isMemberExpression({ object: node })) {
-        // In cases like this.foo or this[foo], there is no need to add
-        // assertThisInitialized, since they already throw if this is
-        // undefined.
-        return;
-      }
-
-      const assertion = t.callExpression(
-        state.file.addHelper("assertThisInitialized"),
-        [node],
-      );
-      path.replaceWith(assertion);
-      path.skip();
-    },
   },
 ]);
 
@@ -84,7 +65,6 @@ export default function transformClass(
     instancePropRefs: {},
     staticPropBody: [],
     body: [],
-    bareSupers: new Set(),
     superThises: [],
     pushedConstructor: false,
     pushedInherits: false,
@@ -217,34 +197,22 @@ export default function transformClass(
 
         replaceSupers.replace();
 
-        // TODO this needs to be cleaned up. But, one step at a time.
-        const state = {
-          returns: [],
-          bareSupers: new Set(),
-        };
+        const superReturns = [];
         path.traverse(
           traverse.visitors.merge([
             environmentVisitor,
             {
-              ReturnStatement(path, state) {
+              ReturnStatement(path) {
                 if (!path.getFunctionParent().isArrowFunctionExpression()) {
-                  state.returns.push(path);
-                }
-              },
-
-              Super(path, state) {
-                const { node, parentPath } = path;
-                if (parentPath.isCallExpression({ callee: node })) {
-                  state.bareSupers.add(parentPath);
+                  superReturns.push(path);
                 }
               },
             },
           ]),
-          state,
         );
 
         if (isConstructor) {
-          pushConstructor(state, node, path);
+          pushConstructor(superReturns, node, path);
         } else {
           pushMethod(node, path);
         }
@@ -378,15 +346,43 @@ export default function transformClass(
 
     path.traverse(findThisesVisitor);
 
-    let guaranteedSuperBeforeFinish = !!classState.bareSupers.size;
-
     let thisRef = function() {
       const ref = path.scope.generateDeclaredUidIdentifier("this");
       thisRef = () => t.cloneNode(ref);
       return ref;
     };
 
-    for (const bareSuper of classState.bareSupers) {
+    for (const thisPath of classState.superThises) {
+      const { node, parentPath } = thisPath;
+      if (parentPath.isMemberExpression({ object: node })) {
+        thisPath.replaceWith(thisRef());
+        continue;
+      }
+      thisPath.replaceWith(
+        t.callExpression(classState.file.addHelper("assertThisInitialized"), [
+          thisRef(),
+        ]),
+      );
+    }
+
+    const bareSupers = new Set();
+    path.traverse(
+      traverse.visitors.merge([
+        environmentVisitor,
+        {
+          Super(path) {
+            const { node, parentPath } = path;
+            if (parentPath.isCallExpression({ callee: node })) {
+              bareSupers.add(parentPath);
+            }
+          },
+        },
+      ]),
+    );
+
+    let guaranteedSuperBeforeFinish = !!bareSupers.size;
+
+    for (const bareSuper of bareSupers) {
       wrapSuperCall(bareSuper, classState.superName, thisRef, body);
 
       if (guaranteedSuperBeforeFinish) {
@@ -408,19 +404,6 @@ export default function transformClass(
       }
     }
 
-    for (const thisPath of classState.superThises) {
-      const { node, parentPath } = thisPath;
-      if (parentPath.isMemberExpression({ object: node })) {
-        thisPath.replaceWith(thisRef());
-        continue;
-      }
-      thisPath.replaceWith(
-        t.callExpression(classState.file.addHelper("assertThisInitialized"), [
-          thisRef(),
-        ]),
-      );
-    }
-
     let wrapReturn;
 
     if (classState.isLoose) {
@@ -535,7 +518,7 @@ export default function transformClass(
    * Replace the constructor body of our class.
    */
   function pushConstructor(
-    replaceSupers,
+    superReturns,
     method: { type: "ClassMethod" },
     path: NodePath,
   ) {
@@ -548,8 +531,7 @@ export default function transformClass(
       userConstructorPath: path,
       userConstructor: method,
       hasConstructor: true,
-      bareSupers: replaceSupers.bareSupers,
-      superReturns: replaceSupers.returns,
+      superReturns,
     });
 
     const { construct } = classState;

diff --git a/packages/babel-plugin-transform-classes/test/fixtures/regression/3028/output.js b/packages/babel-plugin-transform-classes/test/fixtures/regression/3028/output.js
@@ -21,7 +21,7 @@ function (_b) {
     _this = babelHelpers.possibleConstructorReturn(this, babelHelpers.getPrototypeOf(a1).call(this));
 
     _this.x = function () {
-      return babelHelpers.assertThisInitialized(babelHelpers.assertThisInitialized(_this));
+      return babelHelpers.assertThisInitialized(_this);
     };
 
     return _this;
@@ -42,7 +42,7 @@ function (_b2) {
     _this2 = babelHelpers.possibleConstructorReturn(this, babelHelpers.getPrototypeOf(a2).call(this));
 
     _this2.x = function () {
-      return babelHelpers.assertThisInitialized(babelHelpers.assertThisInitialized(_this2));
+      return babelHelpers.assertThisInitialized(_this2);
     };
 
     return _this2;

diff --git a/...m-classes/test/fixtures/spec/this-not-allowed-before-super-in-derived-classes-2/output.js b/...m-classes/test/fixtures/spec/this-not-allowed-before-super-in-derived-classes-2/output.js
@@ -9,7 +9,7 @@ function (_Bar) {
     var _this;
 
     babelHelpers.classCallCheck(this, Foo);
-    return _this = babelHelpers.possibleConstructorReturn(this, babelHelpers.getPrototypeOf(Foo).call(this, babelHelpers.assertThisInitialized(babelHelpers.assertThisInitialized(_this))));
+    return _this = babelHelpers.possibleConstructorReturn(this, babelHelpers.getPrototypeOf(Foo).call(this, babelHelpers.assertThisInitialized(_this)));
   }
 
   return Foo;

diff --git a/...m-classes/test/fixtures/spec/this-not-allowed-before-super-in-derived-classes-3/output.js b/...m-classes/test/fixtures/spec/this-not-allowed-before-super-in-derived-classes-3/output.js
@@ -10,7 +10,7 @@ function (_Bar) {
 
     babelHelpers.classCallCheck(this, Foo);
 
-    var fn = () => babelHelpers.assertThisInitialized(babelHelpers.assertThisInitialized(_this));
+    var fn = () => babelHelpers.assertThisInitialized(_this);
 
     fn();
     return _this = babelHelpers.possibleConstructorReturn(this, babelHelpers.getPrototypeOf(Foo).call(this));

diff --git a/...m-classes/test/fixtures/spec/this-not-allowed-before-super-in-derived-classes-4/output.js b/...m-classes/test/fixtures/spec/this-not-allowed-before-super-in-derived-classes-4/output.js
@@ -10,7 +10,7 @@ function (_Bar) {
 
     babelHelpers.classCallCheck(this, Foo);
 
-    var fn = () => babelHelpers.assertThisInitialized(babelHelpers.assertThisInitialized(_this));
+    var fn = () => babelHelpers.assertThisInitialized(_this);
 
     _this = babelHelpers.possibleConstructorReturn(this, babelHelpers.getPrototypeOf(Foo).call(this));
     fn();

diff --git a/...m-classes/test/fixtures/spec/this-not-allowed-before-super-in-derived-classes-5/output.js b/...m-classes/test/fixtures/spec/this-not-allowed-before-super-in-derived-classes-5/output.js
@@ -9,7 +9,7 @@ function (_Bar) {
     var _this;
 
     babelHelpers.classCallCheck(this, Foo);
-    Foo[babelHelpers.assertThisInitialized(babelHelpers.assertThisInitialized(_this))];
+    Foo[babelHelpers.assertThisInitialized(_this)];
     return babelHelpers.possibleConstructorReturn(_this);
   }