Skip to content

Commit

Permalink
fix another subtle class field ordering bug
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed May 4, 2024
1 parent 85fd597 commit 886c545
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 69 deletions.
5 changes: 2 additions & 3 deletions internal/bundler_tests/snapshots/snapshots_ts.txt
View file
Expand Up @@ -744,16 +744,15 @@ Foo = __decorateClass([

// all_computed.ts
var _a, _b, _c, _d, _e, _f, _g, _h, _i, _j, _k;
_k = mUndef(), _j = mDef(), _h = mDecl(), _g = mAbst(), xUndef(), _f = xDef(), yUndef(), _e = yDef(), _d = sUndef(), _c = sDef(), _a = mDecl();
var Foo2 = class {
constructor() {
this[_j] = 1;
this[_f] = 2;
}
[_i = method()](arg0, arg1) {
[(_k = mUndef(), _j = mDef(), _i = method())](arg0, arg1) {
return new Foo2();
}
static [_b = sMethod()](arg0, arg1) {
static [(_h = mDecl(), _g = mAbst(), xUndef(), _f = xDef(), yUndef(), _e = yDef(), _d = sUndef(), _c = sDef(), _b = sMethod(), _a = mDecl(), _b)](arg0, arg1) {
return new Foo2();
}
};
Expand Down
144 changes: 78 additions & 66 deletions internal/js_parser/js_parser_lower_class.go
View file
Expand Up @@ -1137,72 +1137,81 @@ func (ctx *lowerClassContext) hoistComputedProperties(p *parser, classLoweringIn
if analysis.isComputedPropertyCopiedOrMoved {
inlineKey := prop.Key

if !analysis.needsValueOfKey {
// In certain cases, we only need to evaluate a property key for its
// side effects but we don't actually need the value of the key itself.
// For example, a TypeScript class field without an initializer is
// omitted when TypeScript's "useDefineForClassFields" setting is false.
} else {
// Store the key in a temporary so we can refer to it later
ref := p.generateTempRef(tempRefNeedsDeclare, "")
inlineKey = js_ast.Assign(js_ast.Expr{Loc: prop.Key.Loc, Data: &js_ast.EIdentifier{Ref: ref}}, prop.Key)
p.recordUsage(ref)

// If this property is being duplicated instead of moved or removed, then
// we still need the assignment to the temporary so that we can reference
// it in multiple places, but we don't have to hoist the assignment to an
// earlier property (since this property is still there). In that case
// we can reduce generated code size by avoiding the hoist. One example
// of this case is a decorator on a class element with a computed
// property key:
//
// class Foo {
// @dec [a()]() {}
// }
//
// We want to do this:
//
// var _a;
// class Foo {
// [_a = a()]() {}
// }
// __decorateClass([dec], Foo.prototype, _a, 1);
//
// instead of this:
//
// var _a;
// _a = a();
// class Foo {
// [_a]() {}
// }
// __decorateClass([dec], Foo.prototype, _a, 1);
//
if analysis.rewriteAutoAccessorToGetSet || (!analysis.mustLowerField && !analysis.staticFieldToBlockAssign) {
if propertyKeyTempRefs == nil {
propertyKeyTempRefs = make(map[int]ast.Ref)
}
prop.Key = inlineKey
propertyKeyTempRefs[propIndex] = ref
continue
// If this property is being duplicated instead of moved or removed, then
// we still need the assignment to the temporary so that we can reference
// it in multiple places, but we don't have to hoist the assignment to an
// earlier property (since this property is still there). In that case
// we can reduce generated code size by avoiding the hoist. One example
// of this case is a decorator on a class element with a computed
// property key:
//
// class Foo {
// @dec [a()]() {}
// }
//
// We want to do this:
//
// var _a;
// class Foo {
// [_a = a()]() {}
// }
// __decorateClass([dec], Foo.prototype, _a, 1);
//
// instead of this:
//
// var _a;
// _a = a();
// class Foo {
// [_a]() {}
// }
// __decorateClass([dec], Foo.prototype, _a, 1);
//
// So only do the hoist if this property is being moved or removed.
if !analysis.rewriteAutoAccessorToGetSet && (analysis.mustLowerField || analysis.staticFieldToBlockAssign) {
if !analysis.needsValueOfKey {
// In certain cases, we only need to evaluate a property key for its
// side effects but we don't actually need the value of the key itself.
// For example, a TypeScript class field without an initializer is
// omitted when TypeScript's "useDefineForClassFields" setting is false.
} else {
// Store the key in a temporary so we can refer to it later
ref := p.generateTempRef(tempRefNeedsDeclare, "")
inlineKey = js_ast.Assign(js_ast.Expr{Loc: prop.Key.Loc, Data: &js_ast.EIdentifier{Ref: ref}}, prop.Key)
p.recordUsage(ref)

// Replace this property key with a reference to the temporary. We
// don't need to store the temporary in the "propertyKeyTempRefs"
// map because all references will refer to the temporary, not just
// some of them.
prop.Key = js_ast.Expr{Loc: prop.Key.Loc, Data: &js_ast.EIdentifier{Ref: ref}}
p.recordUsage(ref)
}

// Otherwise, replace this property key with a reference to the
// temporary. We don't need to store the temporary in the
// "propertyKeyTempRefs" map because all references will refer to
// the temporary, not just some of them.
prop.Key = js_ast.Expr{Loc: prop.Key.Loc, Data: &js_ast.EIdentifier{Ref: ref}}
p.recordUsage(ref)
// Figure out where to stick this property's side effect to preserve its order
if nextComputedPropertyKey != nil {
// Insert it before everything that comes after it
*nextComputedPropertyKey = js_ast.JoinWithComma(inlineKey, *nextComputedPropertyKey)
} else {
// Insert it after the first thing that comes before it
ctx.computedPropertyChain = js_ast.JoinWithComma(inlineKey, ctx.computedPropertyChain)
}
continue
}

// Figure out where to stick this property's side effect to preserve its order
if nextComputedPropertyKey != nil {
// Insert it before everything that comes after it
*nextComputedPropertyKey = js_ast.JoinWithComma(inlineKey, *nextComputedPropertyKey)
} else {
// Insert it after the first thing that comes before it
ctx.computedPropertyChain = js_ast.JoinWithComma(inlineKey, ctx.computedPropertyChain)
// Otherwise, we keep the side effects in place (as described above) but
// just store the key in a temporary so we can refer to it later.
ref := p.generateTempRef(tempRefNeedsDeclare, "")
inlineKey = js_ast.Assign(js_ast.Expr{Loc: prop.Key.Loc, Data: &js_ast.EIdentifier{Ref: ref}}, prop.Key)
p.recordUsage(ref)

// Use this temporary when creating duplicate references to this key
if propertyKeyTempRefs == nil {
propertyKeyTempRefs = make(map[int]ast.Ref)
}
continue
prop.Key = inlineKey
propertyKeyTempRefs[propIndex] = ref

// Deliberately continue to fall through to the "computed" case below:
}

// Otherwise, this computed property could be a good location to evaluate
Expand All @@ -1211,13 +1220,16 @@ func (ctx *lowerClassContext) hoistComputedProperties(p *parser, classLoweringIn
// If any side effects after this were hoisted here, then inline them now.
// We don't want to reorder any side effects.
if ctx.computedPropertyChain.Data != nil {
ref := p.generateTempRef(tempRefNeedsDeclare, "")
prop.Key = js_ast.JoinWithComma(js_ast.JoinWithComma(
js_ast.Assign(js_ast.Expr{Loc: prop.Key.Loc, Data: &js_ast.EIdentifier{Ref: ref}}, prop.Key),
ctx.computedPropertyChain),
ref, ok := propertyKeyTempRefs[propIndex]
if !ok {
ref = p.generateTempRef(tempRefNeedsDeclare, "")
prop.Key = js_ast.Assign(js_ast.Expr{Loc: prop.Key.Loc, Data: &js_ast.EIdentifier{Ref: ref}}, prop.Key)
p.recordUsage(ref)
}
prop.Key = js_ast.JoinWithComma(
js_ast.JoinWithComma(prop.Key, ctx.computedPropertyChain),
js_ast.Expr{Loc: prop.Key.Loc, Data: &js_ast.EIdentifier{Ref: ref}})
p.recordUsage(ref)
p.recordUsage(ref)
ctx.computedPropertyChain = js_ast.Expr{}
}

Expand Down
30 changes: 30 additions & 0 deletions scripts/end-to-end-tests.js
View file
Expand Up @@ -5795,6 +5795,36 @@ for (let flags of [['--target=es2022'], ['--target=es6'], ['--bundle', '--target
if (log + '' !== '1,2,3') throw 'fail: ' + log
`,
}),
test(['in.ts', '--outfile=node.js'].concat(flags), {
'in.ts': `
const log = []
class Foo {
@(() => { log.push(3) }) [log.push(1)]() {}
[log.push(2)] = 123;
}
if (log + '' !== '1,2,3') throw 'fail: ' + log
`,
'tsconfig.json': `{
"compilerOptions": {
"experimentalDecorators": true
}
}`,
}),
test(['in.ts', '--outfile=node.js'].concat(flags), {
'in.ts': `
const log = []
class Foo {
@(() => { log.push(3) }) static [log.push(1)]() {}
static [log.push(2)] = 123;
}
if (log + '' !== '1,2,3') throw 'fail: ' + log
`,
'tsconfig.json': `{
"compilerOptions": {
"experimentalDecorators": true
}
}`,
}),

// Check "await" in computed property names
test(['in.ts', '--outfile=node.js', '--format=cjs', '--supported:class-field=false'].concat(flags), {
Expand Down

0 comments on commit 886c545

Please sign in to comment.