Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multiple static blocks #12738

Merged
merged 5 commits into from Mar 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/babel-parser/src/parser/error-message.js
Expand Up @@ -49,7 +49,6 @@ export const ErrorMessages = Object.freeze({
"`%0` has already been exported. Exported identifiers must be unique.",
DuplicateProto: "Redefinition of __proto__ property",
DuplicateRegExpFlags: "Duplicate regular expression flag",
DuplicateStaticBlock: "Duplicate static block in the same class",
ElementAfterRest: "Rest element must be last element",
EscapedCharNotAnIdentifier: "Invalid Unicode escape",
ExportBindingIsString:
Expand Down
12 changes: 1 addition & 11 deletions packages/babel-parser/src/parser/statement.js
Expand Up @@ -1204,7 +1204,6 @@ export default class StatementParser extends ExpressionParser {
const state: N.ParseClassMemberState = {
constructorAllowsSuper,
hadConstructor: false,
hadStaticBlock: false,
};
let decorators: N.Decorator[] = [];
const classBody: N.ClassBody = this.startNode();
Expand Down Expand Up @@ -1313,11 +1312,7 @@ export default class StatementParser extends ExpressionParser {
return;
}
if (this.eat(tt.braceL)) {
this.parseClassStaticBlock(
classBody,
((member: any): N.StaticBlock),
state,
);
this.parseClassStaticBlock(classBody, ((member: any): N.StaticBlock));
return;
}
}
Expand Down Expand Up @@ -1521,7 +1516,6 @@ export default class StatementParser extends ExpressionParser {
parseClassStaticBlock(
classBody: N.ClassBody,
member: N.StaticBlock & { decorators?: Array<N.Decorator> },
state: N.ParseClassMemberState,
) {
this.expectPlugin("classStaticBlock", member.start);
// Start a new lexical scope
Expand All @@ -1538,13 +1532,9 @@ export default class StatementParser extends ExpressionParser {
this.scope.exit();
this.state.labels = oldLabels;
classBody.body.push(this.finishNode<N.StaticBlock>(member, "StaticBlock"));
if (state.hadStaticBlock) {
this.raise(member.start, Errors.DuplicateStaticBlock);
}
if (member.decorators?.length) {
this.raise(member.start, Errors.DecoratorStaticBlock);
}
state.hadStaticBlock = true;
}

pushClassProperty(classBody: N.ClassBody, prop: N.ClassProperty) {
Expand Down
1 change: 0 additions & 1 deletion packages/babel-parser/src/types.js
Expand Up @@ -1528,6 +1528,5 @@ export type ParseSubscriptState = {

export type ParseClassMemberState = {|
hadConstructor: boolean,
hadStaticBlock: boolean,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parser types are not exported, so we are free to refactor here.

constructorAllowsSuper: boolean,
|};
@@ -1,9 +1,6 @@
{
"type": "File",
"start":0,"end":53,"loc":{"start":{"line":1,"column":0},"end":{"line":5,"column":1}},
"errors": [
"SyntaxError: Duplicate static block in the same class (4:2)"
],
"program": {
"type": "Program",
"start":0,"end":53,"loc":{"start":{"line":1,"column":0},"end":{"line":5,"column":1}},
Expand Down
36 changes: 17 additions & 19 deletions packages/babel-plugin-proposal-class-static-block/src/index.js
Expand Up @@ -30,30 +30,28 @@ export default declare(({ types: t, template, assertVersion }) => {
const { scope } = path;
const classBody = path.get("body");
const privateNames = new Set();
let staticBlockPath;
for (const path of classBody.get("body")) {
const body = classBody.get("body");
for (const path of body) {
if (path.isPrivate()) {
privateNames.add(path.get("key.id").node.name);
} else if (path.isStaticBlock()) {
staticBlockPath = path;
}
}
if (!staticBlockPath) {
return;
for (const path of body) {
if (!path.isStaticBlock()) continue;
const staticBlockPrivateId = generateUid(scope, privateNames);
Copy link
Contributor Author

@JLHwung JLHwung Mar 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inserted unique staticBlockPrivateId may accidentally shadow private id defined on upper levels:

class C {
  static #_;
  constructor() {
    class D {
      static {
        C.#_ = 42;
      }
    }
  }
}

The injected #_ = AIIFE(static block) will shadow #_ defined on C. Consider reuse the privateNameVisitorFactory in @babel/helper-create-class-features-plugin.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we just use the filename + source location to generate a private name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does not really solve the mentioned issue, just makes it way less likely to happen. I don't think this is a blocker and we can address that in another PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably consider tracking private identifiers in @babel/traverse's generateUid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we provide a new generateUniquePrivateKeyAPI since plain identifier does not conflict with private identifiers.

privateNames.add(staticBlockPrivateId);
const staticBlockRef = t.privateName(
t.identifier(staticBlockPrivateId),
);
path.replaceWith(
t.classPrivateProperty(
staticBlockRef,
template.expression.ast`(() => { ${path.node.body} })()`,
[],
/* static */ true,
),
);
}
const staticBlockRef = t.privateName(
t.identifier(generateUid(scope, privateNames)),
);
classBody.pushContainer(
"body",
t.classPrivateProperty(
staticBlockRef,
template.expression.ast`(() => { ${staticBlockPath.node.body} })()`,
[],
/* static */ true,
),
);
staticBlockPath.remove();
},
},
};
Expand Down
@@ -1,7 +1,7 @@
class Foo {
static bar = 42;
static {
this.foo = Foo.bar;
}
static bar = 42;
}
expect(Foo.foo).toBe(42);
@@ -0,0 +1,7 @@
class Foo {
static bar = 42;
static {
this.foo = Foo.bar;
}
}
expect(Foo.foo).toBe(42);
@@ -0,0 +1,8 @@
class Foo {
static bar = 42;
static #_ = (() => {
this.foo = Foo.bar;
})();
}

expect(Foo.foo).toBe(42);
@@ -1,7 +1,7 @@
class Foo {
static bar = 42;
static {
this.foo = this.bar;
}
static bar = 42;
}
expect(Foo.foo).toBe(42);
@@ -1,7 +1,7 @@
class Foo {
static bar = 42;
static {
this.foo = this.bar;
}
static bar = 42;
}
expect(Foo.foo).toBe(42);
@@ -1,8 +1,14 @@
class Foo {
static #bar = 21;
static {
this.foo = this.#bar + this.qux;
this.foo = this.#bar;
this.qux1 = this.qux;
}
static qux = 21;
static {
this.qux2 = this.qux;
}
}
expect(Foo.foo).toBe(42);
expect(Foo.foo).toBe(21);
expect(Foo.qux1).toBe(undefined);
expect(Foo.qux2).toBe(21);
@@ -0,0 +1,11 @@
class Foo {
static #bar = 21;
static {
this.foo = this.#bar;
this.qux1 = this.qux;
}
static qux = 21;
static {
this.qux2 = this.qux;
}
}
@@ -0,0 +1,11 @@
class Foo {
static #bar = 21;
static #_ = (() => {
this.foo = this.#bar;
this.qux1 = this.qux;
})();
static qux = 21;
static #_2 = (() => {
this.qux2 = this.qux;
})();
}
@@ -0,0 +1,10 @@
class Base {
constructor() {
this.Foo = class {
static {
this.foo = new.target;
}
}
}
}
expect((new Base).Foo.foo).toBe(undefined);
@@ -0,0 +1,10 @@
class Base {
constructor() {
this.Foo = class {
static {
this.foo = new.target;
}
}
}
}
expect((new Base).Foo.foo).toBe(undefined);
@@ -0,0 +1,12 @@
class Base {
constructor() {
this.Foo = class {
static #_ = (() => {
this.foo = new.target;
})();
};
}

}

expect(new Base().Foo.foo).toBe(undefined);
@@ -0,0 +1,10 @@
class Base {
constructor() {
this.Foo = class {
static {
this.foo = new.target;
}
}
}
}
expect((new Base).Foo.foo).toBe(undefined);
@@ -0,0 +1,10 @@
class Base {
constructor() {
this.Foo = class {
static {
this.foo = new.target;
}
}
}
}
expect((new Base).Foo.foo).toBe(undefined);
@@ -1,7 +1,7 @@
class Foo {
static bar = 42;
static {
this.foo = Foo.bar;
}
static bar = 42;
}
expect(Foo.foo).toBe(42);
@@ -1,7 +1,7 @@
class Foo {
static bar = 42;
static {
this.foo = Foo.bar;
}
static bar = 42;
}
expect(Foo.foo).toBe(42);
@@ -1,7 +1,7 @@
class Foo {
static bar = 42;
static {
this.foo = this.bar;
}
static bar = 42;
}
expect(Foo.foo).toBe(42);
@@ -1,7 +1,7 @@
class Foo {
static bar = 42;
static {
this.foo = this.bar;
}
static bar = 42;
}
expect(Foo.foo).toBe(42);
@@ -1,8 +1,14 @@
class Foo {
static #bar = 21;
static {
this.foo = this.#bar + this.qux;
this.foo = this.#bar;
this.qux1 = this.qux;
}
static qux = 21;
static {
this.qux2 = this.qux;
}
}
expect(Foo.foo).toBe(42);
expect(Foo.foo).toBe(21);
expect(Foo.qux1).toBe(undefined);
expect(Foo.qux2).toBe(21);
@@ -0,0 +1,11 @@
class Foo {
static #bar = 21;
static {
this.foo = this.#bar;
this.qux1 = this.qux;
}
static qux = 21;
static {
this.qux2 = this.qux;
}
}
@@ -0,0 +1,26 @@
var _bar = babelHelpers.classPrivateFieldLooseKey("bar");

var _ = babelHelpers.classPrivateFieldLooseKey("_");

var _2 = babelHelpers.classPrivateFieldLooseKey("_2");

class Foo {}

Object.defineProperty(Foo, _bar, {
writable: true,
value: 21
});
Object.defineProperty(Foo, _, {
writable: true,
value: (() => {
Foo.foo = babelHelpers.classPrivateFieldLooseBase(Foo, _bar)[_bar];
Foo.qux1 = Foo.qux;
})()
});
Foo.qux = 21;
Object.defineProperty(Foo, _2, {
writable: true,
value: (() => {
Foo.qux2 = Foo.qux;
})()
});
@@ -0,0 +1,10 @@
class Base {
constructor() {
this.Foo = class {
static {
this.foo = new.target;
}
}
}
}
expect((new Base).Foo.foo).toBe(undefined);
@@ -0,0 +1,11 @@
class Base {
constructor() {
this.Foo = class {
static {
// fixme: new.target should be undefined after transformed
this.foo = new.target;
}
}
}
}
expect((new Base).Foo.foo).toBe(undefined);
@@ -1,7 +1,7 @@
class Foo {
static bar = 42;
static {
this.foo = Foo.bar;
}
static bar = 42;
}
expect(Foo.foo).toBe(42);
@@ -1,7 +1,7 @@
class Foo {
static bar = 42;
static {
this.foo = Foo.bar;
}
static bar = 42;
}
expect(Foo.foo).toBe(42);
@@ -1,7 +1,7 @@
class Foo {
static bar = 42;
static {
this.foo = this.bar;
}
static bar = 42;
}
expect(Foo.foo).toBe(42);