From 9c26f987dd9bbd33861a86b2a9d0b347f9ec297e Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Sat, 21 Sep 2024 20:58:11 -0400 Subject: [PATCH] lower decorators for useDefineForClassFields #3913 --- .../bundler_tests/bundler_tsconfig_test.go | 58 ++++++++++++++ .../snapshots/snapshots_tsconfig.txt | 75 +++++++++++++++++++ internal/js_ast/js_ast.go | 9 +++ internal/js_parser/js_parser.go | 61 ++++++++------- internal/js_parser/js_parser_lower_class.go | 12 ++- 5 files changed, 183 insertions(+), 32 deletions(-) diff --git a/internal/bundler_tests/bundler_tsconfig_test.go b/internal/bundler_tests/bundler_tsconfig_test.go index c7c0795b4ef..83c4d3d4de7 100644 --- a/internal/bundler_tests/bundler_tsconfig_test.go +++ b/internal/bundler_tests/bundler_tsconfig_test.go @@ -2869,3 +2869,61 @@ func TestTsconfigJsonExtendsArrayIssue3898(t *testing.T) { }, }) } + +func TestTsconfigDecoratorsUseDefineForClassFieldsFalse(t *testing.T) { + tsconfig_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/Users/user/project/src/entry.ts": ` + class Class { + } + class ClassMethod { + foo() {} + } + class ClassField { + foo = 123 + bar + } + class ClassAccessor { + accessor foo = 123 + accessor bar + } + new Class + new ClassMethod + new ClassField + new ClassAccessor + `, + "/Users/user/project/src/entrywithdec.ts": ` + @dec class Class { + } + class ClassMethod { + @dec foo() {} + } + class ClassField { + @dec foo = 123 + @dec bar + } + class ClassAccessor { + @dec accessor foo = 123 + @dec accessor bar + } + new Class + new ClassMethod + new ClassField + new ClassAccessor + `, + "/Users/user/project/src/tsconfig.json": `{ + "compilerOptions": { + "useDefineForClassFields": false + } + }`, + }, + entryPaths: []string{ + "/Users/user/project/src/entry.ts", + "/Users/user/project/src/entrywithdec.ts", + }, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputDir: "/Users/user/project/out", + }, + }) +} diff --git a/internal/bundler_tests/snapshots/snapshots_tsconfig.txt b/internal/bundler_tests/snapshots/snapshots_tsconfig.txt index f77255a3a94..35f690e8093 100644 --- a/internal/bundler_tests/snapshots/snapshots_tsconfig.txt +++ b/internal/bundler_tests/snapshots/snapshots_tsconfig.txt @@ -82,6 +82,81 @@ var foo = 123; // Users/user/project/src/entry.ts console.log(foo); +================================================================================ +TestTsconfigDecoratorsUseDefineForClassFieldsFalse +---------- /Users/user/project/out/entry.js ---------- +// Users/user/project/src/entry.ts +var Class = class { +}; +var ClassMethod = class { + foo() { + } +}; +var ClassField = class { + constructor() { + this.foo = 123; + } +}; +var ClassAccessor = class { + accessor foo = 123; + accessor bar; +}; +new Class(); +new ClassMethod(); +new ClassField(); +new ClassAccessor(); + +---------- /Users/user/project/out/entrywithdec.js ---------- +// Users/user/project/src/entrywithdec.ts +var _Class_decorators, _init; +_Class_decorators = [dec]; +var Class = class { +}; +_init = __decoratorStart(null); +Class = __decorateElement(_init, 0, "Class", _Class_decorators, Class); +__runInitializers(_init, 1, Class); +var _foo_dec, _init2; +_foo_dec = [dec]; +var ClassMethod = class { + constructor() { + __runInitializers(_init2, 5, this); + } + foo() { + } +}; +_init2 = __decoratorStart(null); +__decorateElement(_init2, 1, "foo", _foo_dec, ClassMethod); +__decoratorMetadata(_init2, ClassMethod); +var _bar_dec, _foo_dec2, _init3; +_foo_dec2 = [dec], _bar_dec = [dec]; +var ClassField = class { + constructor() { + this.foo = __runInitializers(_init3, 8, this, 123), __runInitializers(_init3, 11, this); + } +}; +_init3 = __decoratorStart(null); +__decorateElement(_init3, 5, "foo", _foo_dec2, ClassField); +__decorateElement(_init3, 5, "bar", _bar_dec, ClassField); +__decoratorMetadata(_init3, ClassField); +var _bar_dec2, _foo_dec3, _init4, _foo, _bar; +_foo_dec3 = [dec], _bar_dec2 = [dec]; +var ClassAccessor = class { + constructor() { + __privateAdd(this, _foo, __runInitializers(_init4, 8, this, 123)), __runInitializers(_init4, 11, this); + __privateAdd(this, _bar, __runInitializers(_init4, 12, this)), __runInitializers(_init4, 15, this); + } +}; +_init4 = __decoratorStart(null); +_foo = new WeakMap(); +_bar = new WeakMap(); +__decorateElement(_init4, 4, "foo", _foo_dec3, ClassAccessor, _foo); +__decorateElement(_init4, 4, "bar", _bar_dec2, ClassAccessor, _bar); +__decoratorMetadata(_init4, ClassAccessor); +new Class(); +new ClassMethod(); +new ClassField(); +new ClassAccessor(); + ================================================================================ TestTsconfigExtendsArray ---------- /Users/user/project/out/main.js ---------- diff --git a/internal/js_ast/js_ast.go b/internal/js_ast/js_ast.go index f44f1f83b2f..f8d3fe32f5b 100644 --- a/internal/js_ast/js_ast.go +++ b/internal/js_ast/js_ast.go @@ -374,6 +374,15 @@ type Class struct { BodyLoc logger.Loc CloseBraceLoc logger.Loc + // If true, JavaScript decorators (i.e. not TypeScript experimental + // decorators) should be lowered. This is the case either if JavaScript + // decorators are not supported in the configured target environment, or + // if "useDefineForClassFields" is set to false and this class has + // decorators on it. Note that this flag is not necessarily set to true if + // "useDefineForClassFields" is false and a class has an "accessor" even + // though the accessor feature comes from the decorator specification. + ShouldLowerStandardDecorators bool + // If true, property field initializers cannot be assumed to have no side // effects. For example: // diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 8da1a703ce4..7e11b3d6eb9 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -6353,6 +6353,33 @@ func (p *parser) parseClass(classKeyword logger.Range, name *ast.LocRef, classOp closeBraceLoc := p.saveExprCommentsHere() p.lexer.Expect(js_lexer.TCloseBrace) + + // TypeScript has legacy behavior that uses assignment semantics instead of + // define semantics for class fields when "useDefineForClassFields" is enabled + // (in which case TypeScript behaves differently than JavaScript, which is + // arguably "wrong"). + // + // This legacy behavior exists because TypeScript added class fields to + // TypeScript before they were added to JavaScript. They decided to go with + // assignment semantics for whatever reason. Later on TC39 decided to go with + // define semantics for class fields instead. This behaves differently if the + // base class has a setter with the same name. + // + // The value of "useDefineForClassFields" defaults to false when it's not + // specified and the target is earlier than "ES2022" since the class field + // language feature was added in ES2022. However, TypeScript's "target" + // setting currently defaults to "ES3" which unfortunately means that the + // "useDefineForClassFields" setting defaults to false (i.e. to "wrong"). + // + // We default "useDefineForClassFields" to true (i.e. to "correct") instead. + // This is partially because our target defaults to "esnext", and partially + // because this is a legacy behavior that no one should be using anymore. + // Users that want the wrong behavior can either set "useDefineForClassFields" + // to false in "tsconfig.json" explicitly, or set TypeScript's "target" to + // "ES2021" or earlier in their in "tsconfig.json" file. + useDefineForClassFields := !p.options.ts.Parse || p.options.ts.Config.UseDefineForClassFields == config.True || + (p.options.ts.Config.UseDefineForClassFields == config.Unspecified && p.options.ts.Config.Target != config.TSTargetBelowES2022) + return js_ast.Class{ ClassKeyword: classKeyword, Decorators: classOpts.decorators, @@ -6362,31 +6389,15 @@ func (p *parser) parseClass(classKeyword logger.Range, name *ast.LocRef, classOp Properties: properties, CloseBraceLoc: closeBraceLoc, - // TypeScript has legacy behavior that uses assignment semantics instead of - // define semantics for class fields when "useDefineForClassFields" is enabled - // (in which case TypeScript behaves differently than JavaScript, which is - // arguably "wrong"). - // - // This legacy behavior exists because TypeScript added class fields to - // TypeScript before they were added to JavaScript. They decided to go with - // assignment semantics for whatever reason. Later on TC39 decided to go with - // define semantics for class fields instead. This behaves differently if the - // base class has a setter with the same name. - // - // The value of "useDefineForClassFields" defaults to false when it's not - // specified and the target is earlier than "ES2022" since the class field - // language feature was added in ES2022. However, TypeScript's "target" - // setting currently defaults to "ES3" which unfortunately means that the - // "useDefineForClassFields" setting defaults to false (i.e. to "wrong"). - // - // We default "useDefineForClassFields" to true (i.e. to "correct") instead. - // This is partially because our target defaults to "esnext", and partially - // because this is a legacy behavior that no one should be using anymore. - // Users that want the wrong behavior can either set "useDefineForClassFields" - // to false in "tsconfig.json" explicitly, or set TypeScript's "target" to - // "ES2021" or earlier in their in "tsconfig.json" file. - UseDefineForClassFields: !p.options.ts.Parse || p.options.ts.Config.UseDefineForClassFields == config.True || - (p.options.ts.Config.UseDefineForClassFields == config.Unspecified && p.options.ts.Config.Target != config.TSTargetBelowES2022), + // Always lower standard decorators if they are present and TypeScript's + // "useDefineForClassFields" setting is false even if the configured target + // environment supports decorators. This setting changes the behavior of + // class fields, and so we must lower decorators so they behave correctly. + ShouldLowerStandardDecorators: (!p.options.ts.Parse && p.options.unsupportedJSFeatures.Has(compat.Decorators)) || + (p.options.ts.Parse && p.options.ts.Config.ExperimentalDecorators != config.True && + (p.options.unsupportedJSFeatures.Has(compat.Decorators) || !useDefineForClassFields)), + + UseDefineForClassFields: useDefineForClassFields, } } diff --git a/internal/js_parser/js_parser_lower_class.go b/internal/js_parser/js_parser_lower_class.go index 944b0e42509..3e7366140a0 100644 --- a/internal/js_parser/js_parser_lower_class.go +++ b/internal/js_parser/js_parser_lower_class.go @@ -416,8 +416,7 @@ func (p *parser) computeClassLoweringInfo(class *js_ast.Class) (result classLowe // due to the complexity of the decorator specification. The specification is // also still evolving so trying to optimize it now is also potentially // premature. - if p.options.unsupportedJSFeatures.Has(compat.Decorators) && - (!p.options.ts.Parse || p.options.ts.Config.ExperimentalDecorators != config.True) { + if class.ShouldLowerStandardDecorators { for _, prop := range class.Properties { if len(prop.Decorators) > 0 { for _, prop := range class.Properties { @@ -1140,7 +1139,7 @@ func (ctx *lowerClassContext) analyzeProperty(p *parser, prop js_ast.Property, c // they will end up being lowered (if they are even being lowered at all) if p.options.ts.Parse && p.options.ts.Config.ExperimentalDecorators == config.True { analysis.propExperimentalDecorators = prop.Decorators - } else if p.options.unsupportedJSFeatures.Has(compat.Decorators) { + } else if ctx.class.ShouldLowerStandardDecorators { analysis.propDecorators = prop.Decorators } @@ -1451,7 +1450,7 @@ func (ctx *lowerClassContext) processProperties(p *parser, classLoweringInfo cla propertyKeyTempRefs, decoratorTempRefs := ctx.hoistComputedProperties(p, classLoweringInfo) // Save the initializer index for each field and accessor element - if p.options.unsupportedJSFeatures.Has(compat.Decorators) && (!p.options.ts.Parse || p.options.ts.Config.ExperimentalDecorators != config.True) { + if ctx.class.ShouldLowerStandardDecorators { var counts [4]int // Count how many initializers there are in each section @@ -1484,8 +1483,7 @@ func (ctx *lowerClassContext) processProperties(p *parser, classLoweringInfo cla } // Evaluate the decorator expressions inline - if p.options.unsupportedJSFeatures.Has(compat.Decorators) && len(ctx.class.Decorators) > 0 && - (!p.options.ts.Parse || p.options.ts.Config.ExperimentalDecorators != config.True) { + if ctx.class.ShouldLowerStandardDecorators && len(ctx.class.Decorators) > 0 { name := ctx.nameToKeep if name == "" { name = "class" @@ -2079,7 +2077,7 @@ func (ctx *lowerClassContext) finishAndGenerateCode(p *parser, result visitClass if p.options.ts.Parse && p.options.ts.Config.ExperimentalDecorators == config.True { classExperimentalDecorators = ctx.class.Decorators ctx.class.Decorators = nil - } else if p.options.unsupportedJSFeatures.Has(compat.Decorators) { + } else if ctx.class.ShouldLowerStandardDecorators { classDecorators = ctx.decoratorClassDecorators }