From 668358c4d0010ffbb6ebca395759f4a5a16065f9 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Sat, 14 Apr 2018 13:48:38 -0400 Subject: [PATCH] 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 | 4 +- .../babel-helper-replace-supers/src/index.js | 95 ++++++++------ .../package.json | 1 + .../src/index.js | 54 ++++---- .../test/fixtures/regression/7371/exec.js | 99 ++++++++++++++ .../test/fixtures/regression/7371/input.js | 99 ++++++++++++++ .../fixtures/regression/7371/options.json | 3 + .../test/fixtures/regression/7371/output.js | 122 ++++++++++++++++++ .../src/transformClass.js | 18 +-- .../output.js | 4 +- .../output.js | 2 +- .../babel-traverse/src/path/modification.js | 35 +++-- 12 files changed, 436 insertions(+), 100 deletions(-) create mode 100644 packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/exec.js create mode 100644 packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/input.js create mode 100644 packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/options.json create mode 100644 packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/output.js diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index c15537e81f..a1a3a73812 100644 --- a/CONTRIBUTING.md +++ b/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` diff --git a/packages/babel-helper-replace-supers/src/index.js b/packages/babel-helper-replace-supers/src/index.js index 0e4a4d68ef..124e998de2 100644 --- a/packages/babel-helper-replace-supers/src/index.js +++ b/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; diff --git a/packages/babel-plugin-proposal-class-properties/package.json b/packages/babel-plugin-proposal-class-properties/package.json index afc6cd7df2..dda28ca3dd 100644 --- a/packages/babel-plugin-proposal-class-properties/package.json +++ b/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": { diff --git a/packages/babel-plugin-proposal-class-properties/src/index.js b/packages/babel-plugin-proposal-class-properties/src/index.js index a070839836..f9cab17b7b 100644 --- a/packages/babel-plugin-proposal-class-properties/src/index.js +++ b/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, diff --git a/packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/exec.js b/packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/exec.js new file mode 100644 index 0000000000..26b0444dca --- /dev/null +++ b/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(); diff --git a/packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/input.js b/packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/input.js new file mode 100644 index 0000000000..26b0444dca --- /dev/null +++ b/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(); diff --git a/packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/options.json b/packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/options.json new file mode 100644 index 0000000000..77632408b3 --- /dev/null +++ b/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"] +} diff --git a/packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/output.js b/packages/babel-plugin-proposal-class-properties/test/fixtures/regression/7371/output.js new file mode 100644 index 0000000000..965640e31e --- /dev/null +++ b/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(); diff --git a/packages/babel-plugin-transform-classes/src/transformClass.js b/packages/babel-plugin-transform-classes/src/transformClass.js index 125fdf7519..d6e3c05404 100644 --- a/packages/babel-plugin-transform-classes/src/transformClass.js +++ b/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 = Set | { 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); diff --git a/packages/babel-plugin-transform-classes/test/fixtures/spec/nested-class-super-property-in-key/output.js b/packages/babel-plugin-transform-classes/test/fixtures/spec/nested-class-super-property-in-key/output.js index 50f327ba01..e220d98da3 100644 --- a/packages/babel-plugin-transform-classes/test/fixtures/spec/nested-class-super-property-in-key/output.js +++ b/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'; } diff --git a/packages/babel-plugin-transform-classes/test/fixtures/spec/nested-object-super-property-in-key/output.js b/packages/babel-plugin-transform-classes/test/fixtures/spec/nested-object-super-property-in-key/output.js index 38f1038862..11e118e874 100644 --- a/packages/babel-plugin-transform-classes/test/fixtures/spec/nested-object-super-property-in-key/output.js +++ b/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'; } diff --git a/packages/babel-traverse/src/path/modification.js b/packages/babel-traverse/src/path/modification.js index e111c76b0a..0092a20652 100644 --- a/packages/babel-traverse/src/path/modification.js +++ b/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),