Fix class properties after nested class' bare super (#7671)

* Fix class properties after nested class' bare super

Fixes #7371.

* Fix node 4 test

* This damn node 4 test

* All of the ClassBody, but not the methods or field inits

* tmp

* tmp

* Use common class environment visitor

* Tests

* Use skipKey to avoid recursive traversal

* Remove old state

* Use jest expect

CONTRIBUTING.md

@@ -197,8 +197,8 @@ In an `exec.js` test, we run or check that the code actually does what it's supp
 
 ```js
 // exec.js
-assert.equal(8, 2 ** 3);
-assert.equal(24, 3 * 2 ** 3);
+expect(2 ** 3).toBe(8);
+expect(3 * 2 ** 3).toBe(24);
 ```
 
 If you need to check for an error that is thrown you can add to the `options.json`

packages/babel-helper-replace-supers/src/index.js

@@ -1,4 +1,5 @@
 import type { NodePath, Scope } from "@babel/traverse";
+import traverse from "@babel/traverse";
 import optimiseCall from "@babel/helper-optimise-call-expression";
 import * as t from "@babel/types";
 
@@ -25,59 +26,69 @@ function getPrototypeOfExpression(objectRef, isStatic, file) {
   return t.callExpression(file.addHelper("getPrototypeOf"), [targetRef]);
 }
 
-const visitor = {
+function skipAllButComputedKey(path) {
+  // If the path isn't computed, just skip everything.
+  if (!path.node.computed) {
+    path.skip();
+    return;
+  }
+
+  // So it's got a computed key. Make sure to skip every other key the
+  // traversal would visit.
+  const keys = t.VISITOR_KEYS[path.type];
+  for (const key of keys) {
+    if (key !== "key") path.skipKey(key);
+  }
+}
+
+export const environmentVisitor = {
   Function(path) {
+    // Methods will be handled by the Method visit
     if (path.isMethod()) return;
+    // Arrow functions inherit their parent's environment
     if (path.isArrowFunctionExpression()) return;
     path.skip();
   },
 
-  Method(path, state) {
-    // Don't traverse ClassMethod's body
-    path.skip();
-
-    // We do have to traverse the key, since it's evaluated in the outer class
-    // context.
-    if (path.node.computed) {
-      path.get("key").traverse(visitor, state);
-    }
+  Method(path) {
+    skipAllButComputedKey(path);
   },
 
-  "ClassProperty|ClassPrivateProperty"(path, state) {
-    // Don't traverse the ClassProp's value.
-    if (!path.node.static) path.skip();
-
-    // We do have to traverse the key, since it's evaluated in the outer class
-    // context.
-    if (path.node.computed) {
-      path.get("key").traverse(visitor, state);
-    }
-  },
-
-  ReturnStatement(path, state) {
-    if (!path.getFunctionParent().isArrowFunctionExpression()) {
-      state.returns.push(path);
-    }
-  },
-
-  ThisExpression(path, state) {
-    if (!HARDCORE_THIS_REF.has(path.node)) {
-      state.thises.push(path);
-    }
-  },
-
-  Super(path, state) {
-    state.hasSuper = true;
-
-    const { node, parentPath } = path;
-    if (parentPath.isCallExpression({ callee: node })) {
-      state.bareSupers.add(parentPath);
-      return;
-    }
-    state[state.isLoose ? "looseHandle" : "specHandle"](path);
+  "ClassProperty|ClassPrivateProperty"(path) {
+    // If the property is computed, we need to visit everything.
+    if (path.node.static) return;
+    skipAllButComputedKey(path);
   },
 };
 
+const visitor = traverse.visitors.merge([
+  environmentVisitor,
+  {
+    ReturnStatement(path, state) {
+      if (!path.getFunctionParent().isArrowFunctionExpression()) {
+        state.returns.push(path);
+      }
+    },
+
+    ThisExpression(path, state) {
+      if (!HARDCORE_THIS_REF.has(path.node)) {
+        state.thises.push(path);
+      }
+    },
+
+    Super(path, state) {
+      state.hasSuper = true;
+
+      const { node, parentPath } = path;
+      if (parentPath.isCallExpression({ callee: node })) {
+        state.bareSupers.add(parentPath);
+        return;
+      }
+      state[state.isLoose ? "looseHandle" : "specHandle"](path);
+    },
+  },
+]);
+
 export default class ReplaceSupers {
   constructor(opts: Object, inClass?: boolean = false) {
     this.forceSuperMemoisation = opts.forceSuperMemoisation;

packages/babel-plugin-proposal-class-properties/package.json

@@ -11,6 +11,7 @@
   "dependencies": {
     "@babel/helper-function-name": "7.0.0-beta.44",
     "@babel/helper-plugin-utils": "7.0.0-beta.44",
+    "@babel/helper-replace-supers": "7.0.0-beta.44",
     "@babel/plugin-syntax-class-properties": "7.0.0-beta.44"
   },
   "peerDependencies": {

packages/babel-plugin-proposal-class-properties/src/index.js

@@ -1,25 +1,31 @@
 import { declare } from "@babel/helper-plugin-utils";
 import nameFunction from "@babel/helper-function-name";
 import syntaxClassProperties from "@babel/plugin-syntax-class-properties";
-import { template, types as t } from "@babel/core";
+import { template, traverse, types as t } from "@babel/core";
+import { environmentVisitor } from "@babel/helper-replace-supers";
 
 export default declare((api, options) => {
   api.assertVersion(7);
 
   const { loose } = options;
 
-  const findBareSupers = {
-    Super(path) {
-      if (path.parentPath.isCallExpression({ callee: path.node })) {
-        this.push(path.parentPath);
-      }
+  const findBareSupers = traverse.visitors.merge([
+    {
+      Super(path) {
+        const { node, parentPath } = path;
+        if (parentPath.isCallExpression({ callee: node })) {
+          this.push(parentPath);
+        }
+      },
     },
-  };
+    environmentVisitor,
+  ]);
 
   const referenceVisitor = {
     "TSTypeAnnotation|TypeAnnotation"(path) {
       path.skip();
     },
+
     ReferencedIdentifier(path) {
       if (this.scope.hasOwnBinding(path.node.name)) {
         this.scope.rename(path.node.name);
@@ -28,25 +34,22 @@ export default declare((api, options) => {
     },
   };
 
-  const ClassFieldDefinitionEvaluationTDZVisitor = {
-    Expression(path) {
-      if (path === this.shouldSkip) {
-        path.skip();
-      }
-    },
+  const classFieldDefinitionEvaluationTDZVisitor = traverse.visitors.merge([
+    {
+      ReferencedIdentifier(path) {
+        if (this.classRef === path.scope.getBinding(path.node.name)) {
+          const classNameTDZError = this.file.addHelper("classNameTDZError");
+          const throwNode = t.callExpression(classNameTDZError, [
+            t.stringLiteral(path.node.name),
+          ]);
 
-    ReferencedIdentifier(path) {
-      if (this.classRef === path.scope.getBinding(path.node.name)) {
-        const classNameTDZError = this.file.addHelper("classNameTDZError");
-        const throwNode = t.callExpression(classNameTDZError, [
-          t.stringLiteral(path.node.name),
-        ]);
-
-        path.replaceWith(t.sequenceExpression([throwNode, path.node]));
-        path.skip();
-      }
+          path.replaceWith(t.sequenceExpression([throwNode, path.node]));
+          path.skip();
+        }
+      },
     },
-  };
+    environmentVisitor,
+  ]);
 
   const buildClassPropertySpec = (ref, { key, value, computed }, scope) => {
     return template.statement`
@@ -122,10 +125,9 @@ export default declare((api, options) => {
           // Make sure computed property names are only evaluated once (upon class definition)
           // and in the right order in combination with static properties
           if (!computedPath.get("key").isConstantExpression()) {
-            computedPath.traverse(ClassFieldDefinitionEvaluationTDZVisitor, {
+            computedPath.traverse(classFieldDefinitionEvaluationTDZVisitor, {
               classRef: path.scope.getBinding(ref.name),
               file: this.file,
-              shouldSkip: computedPath.get("value"),
             });
             const ident = path.scope.generateUidIdentifierBasedOnNode(
               computedNode.key,

packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/exec.js

@@ -0,0 +1,99 @@
+"use strict";
+class C {
+}
+
+class A extends C {
+  field = 1;
+
+  constructor() {
+    super();
+
+    class B extends C {
+      constructor() {
+        super();
+
+        expect(this.field).toBeUndefined();
+      }
+    }
+
+    expect(this.field).toBe(1)
+
+    new B();
+  }
+}
+
+new A();
+
+class Obj {
+  constructor() {
+    return {};
+  }
+}
+
+// ensure superClass is still transformed
+class SuperClass extends Obj {
+  field = 1;
+
+  constructor() {
+    class B extends (super(), Obj) {
+      constructor() {
+        super();
+
+        expect(this.field).toBeUndefined()
+      }
+    }
+
+    expect(this.field).toBe(1)
+
+    new B();
+  }
+}
+
+new SuperClass();
+
+// ensure ComputedKey Method is still transformed
+class ComputedMethod extends Obj {
+  field = 1;
+
+  constructor() {
+    class B extends Obj {
+      constructor() {
+        super();
+
+        expect(this.field).toBeUndefined()
+      }
+
+      [super()]() { }
+    }
+
+    expect(this.field).toBe(1)
+
+    new B();
+  }
+}
+
+new ComputedMethod();
+
+
+// ensure ComputedKey Field is still transformed
+class ComputedField extends Obj {
+  field = 1;
+
+  constructor() {
+    class B extends Obj {
+      constructor() {
+        super();
+
+        expect(this.field).toBeUndefined()
+      }
+
+      [super()] = 1;
+    }
+
+    expect(this.field).toBe(1)
+
+    new B();
+  }
+}
+
+new ComputedField();

packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/input.js

@@ -0,0 +1,99 @@
+"use strict";
+class C {
+}
+
+class A extends C {
+  field = 1;
+
+  constructor() {
+    super();
+
+    class B extends C {
+      constructor() {
+        super();
+
+        expect(this.field).toBeUndefined();
+      }
+    }
+
+    expect(this.field).toBe(1)
+
+    new B();
+  }
+}
+
+new A();
+
+class Obj {
+  constructor() {
+    return {};
+  }
+}
+
+// ensure superClass is still transformed
+class SuperClass extends Obj {
+  field = 1;
+
+  constructor() {
+    class B extends (super(), Obj) {
+      constructor() {
+        super();
+
+        expect(this.field).toBeUndefined()
+      }
+    }
+
+    expect(this.field).toBe(1)
+
+    new B();
+  }
+}
+
+new SuperClass();
+
+// ensure ComputedKey Method is still transformed
+class ComputedMethod extends Obj {
+  field = 1;
+
+  constructor() {
+    class B extends Obj {
+      constructor() {
+        super();
+
+        expect(this.field).toBeUndefined()
+      }
+
+      [super()]() { }
+    }
+
+    expect(this.field).toBe(1)
+
+    new B();
+  }
+}
+
+new ComputedMethod();
+
+
+// ensure ComputedKey Field is still transformed
+class ComputedField extends Obj {
+  field = 1;
+
+  constructor() {
+    class B extends Obj {
+      constructor() {
+        super();
+
+        expect(this.field).toBeUndefined()
+      }
+
+      [super()] = 1;
+    }
+
+    expect(this.field).toBe(1)
+
+    new B();
+  }
+}
+
+new ComputedField();

packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/options.json

@@ -0,0 +1,3 @@
+{
+  "plugins": ["external-helpers", "proposal-class-properties", "transform-arrow-functions"]
+}

packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/output.js

@@ -0,0 +1,122 @@
+"use strict";
+
+class C {}
+
+class A extends C {
+  constructor() {
+    super();
+    Object.defineProperty(this, "field", {
+      configurable: true,
+      enumerable: true,
+      writable: true,
+      value: 1
+    });
+
+    class B extends C {
+      constructor() {
+        super();
+        expect(this.field).toBeUndefined();
+      }
+
+    }
+
+    expect(this.field).toBe(1);
+    new B();
+  }
+
+}
+
+new A();
+
+class Obj {
+  constructor() {
+    return {};
+  }
+
+} // ensure superClass is still transformed
+
+
+class SuperClass extends Obj {
+  constructor() {
+    var _temp;
+
+    class B extends ((_temp = super(), Object.defineProperty(this, "field", {
+      configurable: true,
+      enumerable: true,
+      writable: true,
+      value: 1
+    }), _temp), Obj) {
+      constructor() {
+        super();
+        expect(this.field).toBeUndefined();
+      }
+
+    }
+
+    expect(this.field).toBe(1);
+    new B();
+  }
+
+}
+
+new SuperClass(); // ensure ComputedKey Method is still transformed
+
+class ComputedMethod extends Obj {
+  constructor() {
+    var _temp2;
+
+    class B extends Obj {
+      constructor() {
+        super();
+        expect(this.field).toBeUndefined();
+      }
+
+      [(_temp2 = super(), Object.defineProperty(this, "field", {
+        configurable: true,
+        enumerable: true,
+        writable: true,
+        value: 1
+      }), _temp2)]() {}
+
+    }
+
+    expect(this.field).toBe(1);
+    new B();
+  }
+
+}
+
+new ComputedMethod(); // ensure ComputedKey Field is still transformed
+
+class ComputedField extends Obj {
+  constructor() {
+    var _temp3;
+
+    var _ref = (_temp3 = super(), Object.defineProperty(this, "field", {
+      configurable: true,
+      enumerable: true,
+      writable: true,
+      value: 1
+    }), _temp3);
+
+    class B extends Obj {
+      constructor() {
+        super();
+        Object.defineProperty(this, _ref, {
+          configurable: true,
+          enumerable: true,
+          writable: true,
+          value: 1
+        });
+        expect(this.field).toBeUndefined();
+      }
+
+    }
+
+    expect(this.field).toBe(1);
+    new B();
+  }
+
+}
+
+new ComputedField();

packages/babel-plugin-transform-classes/src/transformClass.js

@@ -1,22 +1,14 @@
 import type { NodePath } from "@babel/traverse";
 import nameFunction from "@babel/helper-function-name";
-import ReplaceSupers from "@babel/helper-replace-supers";
+import ReplaceSupers, {
+  environmentVisitor,
+} from "@babel/helper-replace-supers";
 import optimiseCall from "@babel/helper-optimise-call-expression";
 import * as defineMap from "@babel/helper-define-map";
 import { traverse, template, types as t } from "@babel/core";
 
 type ReadonlySet<T> = Set<T> | { has(val: T): boolean };
 
-const noMethodVisitor = {
-  "FunctionExpression|FunctionDeclaration"(path) {
-    path.skip();
-  },
-
-  Method(path) {
-    path.skip();
-  },
-};
-
 function buildConstructor(classRef, constructorBody, node) {
   const func = t.functionDeclaration(
     t.cloneNode(classRef),
@@ -78,7 +70,7 @@ export default function transformClass(
   };
 
   const verifyConstructorVisitor = traverse.visitors.merge([
-    noMethodVisitor,
+    environmentVisitor,
     {
       CallExpression: {
         exit(path) {
@@ -115,7 +107,7 @@ export default function transformClass(
   ]);
 
   const findThisesVisitor = traverse.visitors.merge([
-    noMethodVisitor,
+    environmentVisitor,
     {
       ThisExpression(path) {
         classState.superThises.push(path);

packages/babel-plugin-transform-classes/test/fixtures/spec/nested-class-super-property-in-key/output.js

@@ -22,8 +22,6 @@ function (_Hello) {
   babelHelpers.inherits(Outer, _Hello);
 
   function Outer() {
-    var _this2 = this;
-
     var _this;
 
     babelHelpers.classCallCheck(this, Outer);
@@ -37,7 +35,7 @@ function (_Hello) {
       }
 
       babelHelpers.createClass(Inner, [{
-        key: babelHelpers.get(babelHelpers.getPrototypeOf(Outer.prototype), "toString", babelHelpers.assertThisInitialized(_this2)).call(_this2),
+        key: babelHelpers.get(babelHelpers.getPrototypeOf(Outer.prototype), "toString", babelHelpers.assertThisInitialized(_this)).call(_this),
         value: function value() {
           return 'hello';
         }

packages/babel-plugin-transform-classes/test/fixtures/spec/nested-object-super-property-in-key/output.js

@@ -27,7 +27,7 @@ function (_Hello) {
     babelHelpers.classCallCheck(this, Outer);
     _this = babelHelpers.possibleConstructorReturn(this, babelHelpers.getPrototypeOf(Outer).call(this));
     var Inner = {
-      [babelHelpers.get(babelHelpers.getPrototypeOf(Outer.prototype), "toString", babelHelpers.assertThisInitialized(this)).call(this)]() {
+      [babelHelpers.get(babelHelpers.getPrototypeOf(Outer.prototype), "toString", babelHelpers.assertThisInitialized(_this)).call(_this)]() {
         return 'hello';
       }
 

packages/babel-traverse/src/path/modification.js

@@ -14,18 +14,20 @@ export function insertBefore(nodes) {
 
   nodes = this._verifyNodeList(nodes);
 
+  const { parentPath } = this;
+
   if (
-    this.parentPath.isExpressionStatement() ||
-    this.parentPath.isLabeledStatement() ||
-    this.parentPath.isExportNamedDeclaration() ||
-    (this.parentPath.isExportDefaultDeclaration() && this.isDeclaration())
+    parentPath.isExpressionStatement() ||
+    parentPath.isLabeledStatement() ||
+    parentPath.isExportNamedDeclaration() ||
+    (parentPath.isExportDefaultDeclaration() && this.isDeclaration())
   ) {
-    return this.parentPath.insertBefore(nodes);
+    return parentPath.insertBefore(nodes);
   } else if (
     (this.isNodeType("Expression") &&
       this.listKey !== "params" &&
       this.listKey !== "arguments") ||
-    (this.parentPath.isForStatement() && this.key === "init")
+    (parentPath.isForStatement() && this.key === "init")
   ) {
     if (this.node) nodes.push(this.node);
     return this.replaceExpressionWithStatements(nodes);
@@ -96,19 +98,26 @@ export function insertAfter(nodes) {
 
   nodes = this._verifyNodeList(nodes);
 
+  const { parentPath } = this;
   if (
-    this.parentPath.isExpressionStatement() ||
-    this.parentPath.isLabeledStatement() ||
-    this.parentPath.isExportNamedDeclaration() ||
-    (this.parentPath.isExportDefaultDeclaration() && this.isDeclaration())
+    parentPath.isExpressionStatement() ||
+    parentPath.isLabeledStatement() ||
+    parentPath.isExportNamedDeclaration() ||
+    (parentPath.isExportDefaultDeclaration() && this.isDeclaration())
   ) {
-    return this.parentPath.insertAfter(nodes);
+    return parentPath.insertAfter(nodes);
   } else if (
     this.isNodeType("Expression") ||
-    (this.parentPath.isForStatement() && this.key === "init")
+    (parentPath.isForStatement() && this.key === "init")
   ) {
     if (this.node) {
-      const temp = this.scope.generateDeclaredUidIdentifier();
+      let { scope } = this;
+      // Inserting after the computed key of a method should insert the
+      // temporary binding in the method's parent's scope.
+      if (parentPath.isMethod({ computed: true, key: this.node })) {
+        scope = scope.parent;
+      }
+      const temp = scope.generateDeclaredUidIdentifier();
       nodes.unshift(
         t.expressionStatement(
           t.assignmentExpression("=", t.cloneNode(temp), this.node),