Skip to content

Commit

Permalink
fix #2158: private+super+static+bundle edge case
Browse files Browse the repository at this point in the history
  • Loading branch information
evanw committed Apr 7, 2022
1 parent 9ce27c8 commit bdee212
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 0 deletions.
59 changes: 59 additions & 0 deletions CHANGELOG.md
Expand Up @@ -2,6 +2,65 @@

## Unreleased

* Fix a regression regarding `super` ([#2158](https://github.com/evanw/esbuild/issues/2158))

This fixes a regression from the previous release regarding classes with a super class, a private member, and a static field in the scenario where the static field needs to be lowered but where private members are supported by the configured target environment. In this scenario, esbuild could incorrectly inject the instance field initializers that use `this` into the constructor before the call to `super()`, which is invalid. This problem has now been fixed (notice that `this` is now used after `super()` instead of before):

```js
// Original code
class Foo extends Object {
static FOO;
constructor() {
super();
}
#foo;
}

// Old output (with --bundle)
var _foo;
var Foo = class extends Object {
constructor() {
__privateAdd(this, _foo, void 0);
super();
}
};
_foo = new WeakMap();
__publicField(Foo, "FOO");

// New output (with --bundle)
var _foo;
var Foo = class extends Object {
constructor() {
super();
__privateAdd(this, _foo, void 0);
}
};
_foo = new WeakMap();
__publicField(Foo, "FOO");
```

During parsing, esbuild scans the class and makes certain decisions about the class such as whether to lower all static fields, whether to lower each private member, or whether calls to `super()` need to be tracked and adjusted. Previously esbuild made two passes through the class members to compute this information. However, with the new `super()` call lowering logic added in the previous release, we now need three passes to capture the whole dependency chain for this case: 1) lowering static fields requires 2) lowering private members which requires 3) adjusting `super()` calls.

The reason lowering static fields requires lowering private members is because lowering static fields moves their initializers outside of the class body, where they can't access private members anymore. Consider this code:

```js
class Foo {
get #foo() {}
static bar = new Foo().#foo
}
```

We can't just lower static fields without also lowering private members, since that causes a syntax error:

```js
class Foo {
get #foo() {}
}
Foo.bar = new Foo().#foo;
```

And the reason lowering private members requires adjusting `super()` calls is because the injected private member initializers use `this`, which is only accessible after `super()` calls in the constructor.

* Add Linux ARM64 support for Deno ([#2156](https://github.com/evanw/esbuild/issues/2156))

This release adds Linux ARM64 support to esbuild's [Deno](https://deno.land/) API implementation, which allows esbuild to be used with Deno on a Raspberry Pi.
Expand Down
22 changes: 22 additions & 0 deletions internal/bundler/bundler_lower_test.go
Expand Up @@ -1532,6 +1532,28 @@ func TestLowerPrivateSuperES2021(t *testing.T) {
})
}

// https://github.com/evanw/esbuild/issues/2158
func TestLowerPrivateSuperStaticBundleIssue2158(t *testing.T) {
lower_suite.expectBundled(t, bundled{
files: map[string]string{
"/entry.js": `
export class Foo extends Object {
static FOO;
constructor() {
super();
}
#foo;
}
`,
},
entryPaths: []string{"/entry.js"},
options: config.Options{
Mode: config.ModeBundle,
AbsOutputFile: "/out.js",
},
})
}

func TestLowerClassField2020NoBundle(t *testing.T) {
lower_suite.expectBundled(t, bundled{
files: map[string]string{
Expand Down
17 changes: 17 additions & 0 deletions internal/bundler/snapshots/snapshots_lower.txt
Expand Up @@ -1633,6 +1633,23 @@ export {
foo8_default as foo8
};

================================================================================
TestLowerPrivateSuperStaticBundleIssue2158
---------- /out.js ----------
// entry.js
var _foo;
var Foo = class extends Object {
constructor() {
super();
__privateAdd(this, _foo, void 0);
}
};
_foo = new WeakMap();
__publicField(Foo, "FOO");
export {
Foo
};

================================================================================
TestLowerStaticAsyncArrowSuperES2016
---------- /out.js ----------
Expand Down
9 changes: 9 additions & 0 deletions internal/js_parser/js_parser.go
Expand Up @@ -10158,6 +10158,7 @@ func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class, defaul
// field initializers outside of the class body and "this" will no longer
// reference the same thing.
classLoweringInfo := p.computeClassLoweringInfo(class)
recomputeClassLoweringInfo := false

// Sometimes we need to lower private members even though they are supported.
// This flags them for lowering so that we lower references to them as we
Expand Down Expand Up @@ -10186,6 +10187,7 @@ func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class, defaul
// The private getter must be lowered too.
if private, ok := prop.Key.Data.(*js_ast.EPrivateIdentifier); ok {
p.symbols[private.Ref.InnerIndex].Flags |= js_ast.PrivateSymbolMustBeLowered
recomputeClassLoweringInfo = true
}
}
}
Expand All @@ -10197,11 +10199,18 @@ func (p *parser) visitClass(nameScopeLoc logger.Loc, class *js_ast.Class, defaul
if private, ok := prop.Key.Data.(*js_ast.EPrivateIdentifier); ok {
if symbol := &p.symbols[private.Ref.InnerIndex]; p.classPrivateBrandChecksToLower[symbol.OriginalName] {
symbol.Flags |= js_ast.PrivateSymbolMustBeLowered
recomputeClassLoweringInfo = true
}
}
}
}

// If we changed private symbol lowering decisions, then recompute class
// lowering info because that may have changed other decisions too
if recomputeClassLoweringInfo {
classLoweringInfo = p.computeClassLoweringInfo(class)
}

p.pushScopeForVisitPass(js_ast.ScopeClassName, nameScopeLoc)
oldEnclosingClassKeyword := p.enclosingClassKeyword
p.enclosingClassKeyword = class.ClassKeyword
Expand Down
20 changes: 20 additions & 0 deletions scripts/end-to-end-tests.js
Expand Up @@ -4923,6 +4923,26 @@
}
`,
}, { async: true }),
test(['in.js', '--outfile=node.js', '--bundle'].concat(flags), {
// Check edge case in https://github.com/evanw/esbuild/issues/2158
'in.js': `
class Foo {
constructor(x) {
this.base = x
}
}
class Bar extends Foo {
static FOO = 1
constructor() {
super(2)
this.derived = this.#foo + Bar.FOO
}
#foo = 3
}
let bar = new Bar
if (bar.base !== 2 || bar.derived !== 4) throw 'fail'
`,
}),
test(['in.js', '--outfile=node.js', '--keep-names', '--bundle'].concat(flags), {
// Check default export name preservation with lowered "super" inside lowered "async"
'in.js': `
Expand Down

0 comments on commit bdee212

Please sign in to comment.