fix #185: add strict option for class fields

CHANGELOG.md

@@ -2,9 +2,15 @@
 
 ## Unreleased
 
-* Add the `--strict` option
+* Add the `--strict:nullish-coalescing` option
 
-    This currently only affects the transform for the `??` nullish coalescing operator. In loose mode (the default), `a ?? b` is transformed to `a != null ? a : b`. This works fine in all cases except when `a` is the special object `document.all`. In strict mode, `a ?? b` is transformed to `a !== null && a !== void 0 ? a : b` which works correctly with `document.all`. Enable `--strict` if you need to use `document.all` with the `??` operator.
+    This affects the transform for the `??` nullish coalescing operator. In loose mode (the default), `a ?? b` is transformed to `a != null ? a : b`. This works fine in all cases except when `a` is the special object `document.all`. In strict mode, `a ?? b` is transformed to `a !== null && a !== void 0 ? a : b` which works correctly with `document.all`. Enable `--strict:nullish-coalescing` if you need to use `document.all` with the `??` operator.
+
+* Add the `--strict:class-fields` option
+
+    This affects the transform for instance and static class fields. In loose mode (the default), class field initialization is transformed to a normal assignment. This is what the TypeScript compiler does by default. However, it doesn't follow the JavaScript specification exactly (e.g. it may call setter methods). Enable `--strict:class-fields` if you need accurate class field initialization.
+
+Note that you can also just use `--strict` to enable strictness for all transforms instead of using `--strict:...` for each transform.
 
 ## 0.5.8
 

README.md

@@ -114,15 +114,54 @@ These syntax features are conditionally transformed for older browsers depending
 | [Logical assignment operators](https://github.com/tc39/proposal-logical-assignment) | `esnext`                             | `a ??= b`                  |
 
 <details>
-<summary>Syntax transform caveats (click to expand)</summary><br>
+<summary>Syntax transform caveats (click to expand)</summary>
 
 * **Nullish coalescing correctness**
 
-    By default `a ?? b` is transformed into `a != null ? a : b`, which works because `a != null` is only false if `a` is `null` or `undefined`. However, there's exactly one obscure edge case where this doesn't work. For legacy reasons, the value of `document.all` is special-cased such that `document.all != null` is false. If you need to use this value with the nullish coalescing operator, you should enable `--strict` transforms so `a ?? b` becomes `a !== null && a !== void 0 ? a : b` instead, which works correctly with `document.all`. The strict transform isn't done by default because it causes code bloat for an obscure edge case that shouldn't matter in modern code.
+    By default `a ?? b` is transformed into `a != null ? a : b`, which works because `a != null` is only false if `a` is `null` or `undefined`. However, there's exactly one obscure edge case where this doesn't work. For legacy reasons, the value of `document.all` is special-cased such that `document.all != null` is false. If you need to use this value with the nullish coalescing operator, you should enable `--strict:nullish-coalescing` transforms so `a ?? b` becomes `a !== null && a !== void 0 ? a : b` instead, which works correctly with `document.all`. The strict transform isn't done by default because it causes code bloat for an obscure edge case that shouldn't matter in modern code.
+
+* **Class field correctness**
+
+    Class fields look like this:
+
+    ```js
+    class Foo {
+      foo = 123
+    }
+    ```
+
+    By default, the transform for class fields uses a normal assignment for initialization. That looks like this:
+
+    ```js
+    class Foo {
+      constructor() {
+        this.foo = 123;
+      }
+    }
+    ```
+
+    This doesn't increase code size by much and stays on the heavily-optimized path for modern JavaScript JITs. It also matches the default behavior of the TypeScript compiler. However, this doesn't exactly follow the initialization behavior in the JavaScript specification. For example, it can cause a setter to be called if one exists with that property name, which isn't supposed to happen. A more accurate transformation would be to use `Object.defineProperty()` instead like this:
+
+    ```js
+    class Foo {
+      constructor() {
+        Object.defineProperty(this, "foo", {
+          enumerable: true,
+          configurable: true,
+          writable: true,
+          value: 123
+        });
+      }
+    }
+    ```
+
+    This increases code size and decreases performance, but follows the JavaScript specification more accurately. If you need this accuracy, you should enable the `--strict:class-fields` option.
 
 * **Private member performance**
 
     This transform uses `WeakMap` and `WeakSet` to preserve the privacy properties of this feature, similar to the corresponding transforms in the Babel and TypeScript compilers. Most modern JavaScript engines (V8, JavaScriptCore, and SpiderMonkey but not ChakraCore) may not have good performance characteristics for large `WeakMap` and `WeakSet` objects. Creating many instances of classes with private fields or private methods with this syntax transform active may cause a lot of overhead for the garbage collector. This is because modern engines (other than ChakraCore) store weak values in an actual map object instead of as hidden properties on the keys themselves, and large map objects can cause performance issues with garbage collection. See [this reference](https://github.com/tc39/ecma262/issues/1657#issuecomment-518916579) for more information.
+
+Note that if you want to enable strictness for all transforms, you can just pass `--strict` instead of using `--strict:...` for each transform.
 </details>
 
 These syntax features are currently always passed through un-transformed:

internal/bundler/bundler_lower_test.go

@@ -1750,3 +1750,79 @@ export {
 		},
 	})
 }
+
+func TestLowerClassFieldStrictNoBundle(t *testing.T) {
+	expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.js": `
+				class Foo {
+					foo = 123
+					bar
+					static foo = 234
+					static bar
+				}
+			`,
+		},
+		entryPaths: []string{"/entry.js"},
+		parseOptions: parser.ParseOptions{
+			IsBundling: false,
+			Target:     parser.ES2019,
+			Strict: parser.StrictOptions{
+				ClassFields: true,
+			},
+		},
+		bundleOptions: BundleOptions{
+			IsBundling:    false,
+			AbsOutputFile: "/out.js",
+		},
+		expected: map[string]string{
+			"/out.js": `class Foo {
+  constructor() {
+    __publicField(this, "foo", 123);
+    __publicField(this, "bar", void 0);
+  }
+}
+__publicField(Foo, "foo", 234);
+__publicField(Foo, "bar", void 0);
+`,
+		},
+	})
+}
+
+func TestTSLowerClassFieldStrictNoBundle(t *testing.T) {
+	expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.ts": `
+				class Foo {
+					foo = 123
+					bar
+					static foo = 234
+					static bar
+				}
+			`,
+		},
+		entryPaths: []string{"/entry.ts"},
+		parseOptions: parser.ParseOptions{
+			IsBundling: false,
+			Target:     parser.ES2019,
+			Strict: parser.StrictOptions{
+				ClassFields: true,
+			},
+		},
+		bundleOptions: BundleOptions{
+			IsBundling:    false,
+			AbsOutputFile: "/out.js",
+		},
+		expected: map[string]string{
+			"/out.js": `class Foo {
+  constructor() {
+    __publicField(this, "foo", 123);
+    __publicField(this, "bar", void 0);
+  }
+}
+__publicField(Foo, "foo", 234);
+__publicField(Foo, "bar", void 0);
+`,
+		},
+	})
+}

internal/parser/parser.go

@@ -8942,6 +8942,15 @@ type StrictOptions struct {
 	// legacy reasons such that "document.all != null" is false even though it's
 	// not "null" or "undefined".
 	NullishCoalescing bool
+
+	// Loose:  "class Foo { foo = 1 }" => "class Foo { constructor() { this.foo = 1; } }"
+	// Strict: "class Foo { foo = 1 }" => "class Foo { constructor() { __publicField(this, 'foo', 1); } }"
+	//
+	// The disadvantage of strictness here is code bloat and performance. The
+	// advantage is following the class field specification accurately. For
+	// example, loose mode will incorrectly trigger setter methods while strict
+	// mode won't.
+	ClassFields bool
 }
 
 type ParseOptions struct {

internal/parser/parser_lower.go

@@ -885,17 +885,20 @@ func (p *parser) lowerClass(stmt ast.Stmt, expr ast.Expr) ([]ast.Stmt, ast.Expr)
 			}
 		}
 
+		// The TypeScript class field transform requires removing fields without
+		// initializers. If the field is removed, then we only need the key for
+		// its side effects and we don't need a temporary reference for the key.
+		// However, the TypeScript compiler doesn't remove the field when doing
+		// strict class field initialization, so we shouldn't either.
+		shouldOmitFieldInitializer := p.TS.Parse && !prop.IsMethod && prop.Initializer == nil && !p.Strict.ClassFields
+
 		// Make sure the order of computed property keys doesn't change. These
 		// expressions have side effects and must be evaluated in order.
 		keyExprNoSideEffects := prop.Key
 		if prop.IsComputed && (p.TS.Parse || computedPropertyCache.Data != nil ||
 			(!prop.IsMethod && p.Target < ESNext) || len(prop.TSDecorators) > 0) {
 			needsKey := true
-
-			// The TypeScript class field transform requires removing fields without
-			// initializers. If the field is removed, then we only need the key for
-			// its side effects and we don't need a temporary reference for the key.
-			if len(prop.TSDecorators) == 0 && (prop.IsMethod || (p.TS.Parse && prop.Initializer == nil)) {
+			if len(prop.TSDecorators) == 0 && (prop.IsMethod || shouldOmitFieldInitializer) {
 				needsKey = false
 			}
 
@@ -975,7 +978,7 @@ func (p *parser) lowerClass(stmt ast.Stmt, expr ast.Expr) ([]ast.Stmt, ast.Expr)
 			// The TypeScript compiler doesn't follow the JavaScript spec for
 			// uninitialized fields. They are supposed to be set to undefined but the
 			// TypeScript compiler just omits them entirely.
-			if !p.TS.Parse || prop.Initializer != nil || prop.Value != nil || (privateField != nil && p.Target < privateNameTarget) {
+			if !p.TS.Parse || !shouldOmitFieldInitializer || prop.Value != nil || (privateField != nil && p.Target < privateNameTarget) {
 				loc := prop.Key.Loc
 
 				// Determine where to store the field
@@ -1028,6 +1031,12 @@ func (p *parser) lowerClass(stmt ast.Stmt, expr ast.Expr) ([]ast.Stmt, ast.Expr)
 							init,
 						},
 					}}
+				} else if p.Strict.ClassFields {
+					expr = p.callRuntime(loc, "__publicField", []ast.Expr{
+						target,
+						prop.Key,
+						init,
+					})
 				} else {
 					if key, ok := prop.Key.Data.(*ast.EString); ok && !prop.IsComputed {
 						target = ast.Expr{loc, &ast.EDot{

internal/parser/parser_test.go

@@ -122,6 +122,7 @@ func expectPrintedTargetStrict(t *testing.T, target LanguageTarget, contents str
 			Target: target,
 			Strict: StrictOptions{
 				NullishCoalescing: true,
+				ClassFields:       true,
 			},
 		})
 		msgs := log.Done()

internal/runtime/runtime.go

@@ -72,7 +72,10 @@ const Code = `
 	}
 	export let __param = (index, decorator) => (target, key) => decorator(target, key, index)
 
-	// For class private members
+	// For class members
+	export let __publicField = (obj, key, value) => {
+		return __defineProperty(obj, key, {enumerable: true, configurable: true, writable: true, value})
+	}
 	let __accessCheck = (obj, member, msg) => {
 		if (!member.has(obj)) throw TypeError('Cannot ' + msg)
 	}

lib/api-common.ts

@@ -2,7 +2,8 @@ import * as types from "./api-types";
 
 function pushCommonFlags(flags: string[], options: types.CommonOptions, isTTY: boolean, logLevelDefault: types.LogLevel): void {
   if (options.target) flags.push(`--target=${options.target}`);
-  if (options.strict) flags.push(`--strict${options.strict === true ? '' : `=${options.strict.join(',')}`}`);
+  if (options.strict === true) flags.push(`--strict`);
+  else if (options.strict) for (let key of options.strict) flags.push(`--strict:${key}`);
 
   if (options.minify) flags.push('--minify');
   if (options.minifySyntax) flags.push('--minify-syntax');

pkg/api/api.go

@@ -162,6 +162,7 @@ const (
 
 type StrictOptions struct {
 	NullishCoalescing bool
+	ClassFields       bool
 }
 
 ////////////////////////////////////////////////////////////////////////////////

pkg/api/api_impl.go

@@ -111,6 +111,7 @@ func validateTarget(value Target) parser.LanguageTarget {
 func validateStrict(value StrictOptions) parser.StrictOptions {
 	return parser.StrictOptions{
 		NullishCoalescing: value.NullishCoalescing,
+		ClassFields:       value.ClassFields,
 	}
 }
 

pkg/cli/cli_impl.go

@@ -175,6 +175,7 @@ func parseOptionsImpl(osArgs []string, buildOpts *api.BuildOptions, transformOpt
 		case arg == "--strict":
 			value := api.StrictOptions{
 				NullishCoalescing: true,
+				ClassFields:       true,
 			}
 			if buildOpts != nil {
 				buildOpts.Strict = value
@@ -182,23 +183,21 @@ func parseOptionsImpl(osArgs []string, buildOpts *api.BuildOptions, transformOpt
 				transformOpts.Strict = value
 			}
 
-		case strings.HasPrefix(arg, "--strict="):
-			value := api.StrictOptions{}
-			parts := arg[len("--strict="):]
-			if parts != "" {
-				for _, part := range strings.Split(parts, ",") {
-					switch part {
-					case "nullish-coalescing":
-						value.NullishCoalescing = true
-					default:
-						return fmt.Errorf("Invalid strict value: %q (valid: nullish-coalescing)", part)
-					}
-				}
-			}
+		case strings.HasPrefix(arg, "--strict:"):
+			var value *api.StrictOptions
 			if buildOpts != nil {
-				buildOpts.Strict = value
+				value = &buildOpts.Strict
 			} else {
-				transformOpts.Strict = value
+				value = &transformOpts.Strict
+			}
+			name := arg[len("--strict:"):]
+			switch name {
+			case "nullish-coalescing":
+				value.NullishCoalescing = true
+			case "class-fields":
+				value.ClassFields = true
+			default:
+				return fmt.Errorf("Invalid strict value: %q (valid: nullish-coalescing, class-fields)", name)
 			}
 
 		case strings.HasPrefix(arg, "--platform=") && buildOpts != nil:

scripts/end-to-end-tests.js

@@ -511,6 +511,28 @@
         new Foo().bar()
       `,
     }),
+    test(['in.js', '--outfile=node.js', '--target=es6'], {
+      'in.js': `
+        let called = false
+        class Foo {
+          foo
+          set foo(x) { called = true }
+        }
+        new Foo()
+        if (!called) throw 'fail'
+      `,
+    }),
+    test(['in.js', '--outfile=node.js', '--target=es6', '--strict'], {
+      'in.js': `
+        let called = false
+        class Foo {
+          foo
+          set foo(x) { called = true }
+        }
+        new Foo()
+        if (called) throw 'fail'
+      `,
+    }),
   )
 
   // Async lowering tests