fix #1418: private fields and logical assignment

evanw · Jul 29, 2021 · 748d8b4 · 748d8b4
1 parent bb3a4cd
commit 748d8b4
Show file tree

Hide file tree

Showing 4 changed files with 161 additions and 32 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,50 @@
 
 ## Unreleased
 
+* Fix a bug with private fields and logical assignment operators ([#1418](https://github.com/evanw/esbuild/issues/1418))
+
+    This release fixes a bug where code using private fields in combination with [logical assignment operators](https://github.com/tc39/proposal-logical-assignment) was transformed incorrectly if the target environment supported logical assignment operators but not private fields. Since logical assignment operators are assignment operators, the entire operator must be transformed even if the operator is supported. This should now work correctly:
+
+    ```js
+    // Original code
+    class Foo {
+      #x
+      foo() {
+        this.#x &&= 2
+        this.#x ||= 2
+        this.#x ??= 2
+      }
+    }
+
+    // Old output
+    var _x;
+    class Foo {
+      constructor() {
+        __privateAdd(this, _x, void 0);
+      }
+      foo() {
+        this._x &&= 2;
+        this._x ||= 2;
+        this._x ??= 2;
+      }
+    }
+    _x = new WeakMap();
+
+    // New output
+    var _x, _a;
+    class Foo {
+      constructor() {
+        __privateAdd(this, _x, void 0);
+      }
+      foo() {
+        __privateGet(this, _x) && __privateSet(this, _x, 2);
+        __privateGet(this, _x) || __privateSet(this, _x, 2);
+        __privateGet(this, _x) ?? __privateSet(this, _x, 2);
+      }
+    }
+    _x = new WeakMap();
+    ```
+
 * Fix a hoisting bug in the bundler ([#1455](https://github.com/evanw/esbuild/issues/1455))
 
     This release fixes a bug where variables declared using `var` inside of top-level `for` loop initializers were not hoisted inside lazily-initialized ES modules (such as those that are generated when bundling code that loads an ES module using `require`). This meant that hoisted function declarations incorrectly didn't have access to these loop variables:

diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go
@@ -11265,18 +11265,18 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 			}
 
 		case js_ast.BinOpNullishCoalescingAssign:
-			if p.options.unsupportedJSFeatures.Has(compat.LogicalAssignment) {
-				return p.lowerNullishCoalescingAssignmentOperator(expr.Loc, e), exprOut{}
+			if value, ok := p.lowerNullishCoalescingAssignmentOperator(expr.Loc, e); ok {
+				return value, exprOut{}
 			}
 
 		case js_ast.BinOpLogicalAndAssign:
-			if p.options.unsupportedJSFeatures.Has(compat.LogicalAssignment) {
-				return p.lowerLogicalAssignmentOperator(expr.Loc, e, js_ast.BinOpLogicalAnd), exprOut{}
+			if value, ok := p.lowerLogicalAssignmentOperator(expr.Loc, e, js_ast.BinOpLogicalAnd); ok {
+				return value, exprOut{}
 			}
 
 		case js_ast.BinOpLogicalOrAssign:
-			if p.options.unsupportedJSFeatures.Has(compat.LogicalAssignment) {
-				return p.lowerLogicalAssignmentOperator(expr.Loc, e, js_ast.BinOpLogicalOr), exprOut{}
+			if value, ok := p.lowerLogicalAssignmentOperator(expr.Loc, e, js_ast.BinOpLogicalOr); ok {
+				return value, exprOut{}
 			}
 		}
 

diff --git a/internal/js_parser/js_parser_lower.go b/internal/js_parser/js_parser_lower.go
@@ -848,14 +848,14 @@ func (p *parser) lowerExponentiationAssignmentOperator(loc logger.Loc, e *js_ast
 	})
 }
 
-func (p *parser) lowerNullishCoalescingAssignmentOperator(loc logger.Loc, e *js_ast.EBinary) js_ast.Expr {
+func (p *parser) lowerNullishCoalescingAssignmentOperator(loc logger.Loc, e *js_ast.EBinary) (js_ast.Expr, bool) {
 	if target, privateLoc, private := p.extractPrivateIndex(e.Left); private != nil {
 		if p.options.unsupportedJSFeatures.Has(compat.NullishCoalescing) {
 			// "a.#b ??= c" => "(_a = __privateGet(a, #b)) != null ? _a : __privateSet(a, #b, c)"
 			targetFunc, targetWrapFunc := p.captureValueWithPossibleSideEffects(loc, 2, target, valueDefinitelyNotMutated)
 			left := p.lowerPrivateGet(targetFunc(), privateLoc, private)
 			right := p.lowerPrivateSet(targetFunc(), privateLoc, private, e.Right)
-			return targetWrapFunc(p.lowerNullishCoalescing(loc, left, right))
+			return targetWrapFunc(p.lowerNullishCoalescing(loc, left, right)), true
 		}
 
 		// "a.#b ??= c" => "__privateGet(a, #b) ?? __privateSet(a, #b, c)"
@@ -864,25 +864,29 @@ func (p *parser) lowerNullishCoalescingAssignmentOperator(loc logger.Loc, e *js_
 			Op:    js_ast.BinOpNullishCoalescing,
 			Left:  p.lowerPrivateGet(targetFunc(), privateLoc, private),
 			Right: p.lowerPrivateSet(targetFunc(), privateLoc, private, e.Right),
-		}})
+		}}), true
 	}
 
-	return p.lowerAssignmentOperator(e.Left, func(a js_ast.Expr, b js_ast.Expr) js_ast.Expr {
-		if p.options.unsupportedJSFeatures.Has(compat.NullishCoalescing) {
-			// "a ??= b" => "(_a = a) != null ? _a : a = b"
-			return p.lowerNullishCoalescing(loc, a, js_ast.Assign(b, e.Right))
-		}
+	if p.options.unsupportedJSFeatures.Has(compat.LogicalAssignment) {
+		return p.lowerAssignmentOperator(e.Left, func(a js_ast.Expr, b js_ast.Expr) js_ast.Expr {
+			if p.options.unsupportedJSFeatures.Has(compat.NullishCoalescing) {
+				// "a ??= b" => "(_a = a) != null ? _a : a = b"
+				return p.lowerNullishCoalescing(loc, a, js_ast.Assign(b, e.Right))
+			}
 
-		// "a ??= b" => "a ?? (a = b)"
-		return js_ast.Expr{Loc: loc, Data: &js_ast.EBinary{
-			Op:    js_ast.BinOpNullishCoalescing,
-			Left:  a,
-			Right: js_ast.Assign(b, e.Right),
-		}}
-	})
+			// "a ??= b" => "a ?? (a = b)"
+			return js_ast.Expr{Loc: loc, Data: &js_ast.EBinary{
+				Op:    js_ast.BinOpNullishCoalescing,
+				Left:  a,
+				Right: js_ast.Assign(b, e.Right),
+			}}
+		}), true
+	}
+
+	return js_ast.Expr{}, false
 }
 
-func (p *parser) lowerLogicalAssignmentOperator(loc logger.Loc, e *js_ast.EBinary, op js_ast.OpCode) js_ast.Expr {
+func (p *parser) lowerLogicalAssignmentOperator(loc logger.Loc, e *js_ast.EBinary, op js_ast.OpCode) (js_ast.Expr, bool) {
 	if target, privateLoc, private := p.extractPrivateIndex(e.Left); private != nil {
 		// "a.#b &&= c" => "__privateGet(a, #b) && __privateSet(a, #b, c)"
 		// "a.#b ||= c" => "__privateGet(a, #b) || __privateSet(a, #b, c)"
@@ -891,18 +895,22 @@ func (p *parser) lowerLogicalAssignmentOperator(loc logger.Loc, e *js_ast.EBinar
 			Op:    op,
 			Left:  p.lowerPrivateGet(targetFunc(), privateLoc, private),
 			Right: p.lowerPrivateSet(targetFunc(), privateLoc, private, e.Right),
-		}})
+		}}), true
 	}
 
-	return p.lowerAssignmentOperator(e.Left, func(a js_ast.Expr, b js_ast.Expr) js_ast.Expr {
-		// "a &&= b" => "a && (a = b)"
-		// "a ||= b" => "a || (a = b)"
-		return js_ast.Expr{Loc: loc, Data: &js_ast.EBinary{
-			Op:    op,
-			Left:  a,
-			Right: js_ast.Assign(b, e.Right),
-		}}
-	})
+	if p.options.unsupportedJSFeatures.Has(compat.LogicalAssignment) {
+		return p.lowerAssignmentOperator(e.Left, func(a js_ast.Expr, b js_ast.Expr) js_ast.Expr {
+			// "a &&= b" => "a && (a = b)"
+			// "a ||= b" => "a || (a = b)"
+			return js_ast.Expr{Loc: loc, Data: &js_ast.EBinary{
+				Op:    op,
+				Left:  a,
+				Right: js_ast.Assign(b, e.Right),
+			}}
+		}), true
+	}
+
+	return js_ast.Expr{}, false
 }
 
 func (p *parser) lowerNullishCoalescing(loc logger.Loc, left js_ast.Expr, right js_ast.Expr) js_ast.Expr {

diff --git a/internal/js_parser/js_parser_lower_test.go b/internal/js_parser/js_parser_lower_test.go
@@ -65,17 +65,48 @@ func TestLowerNullishCoalescingAssign(t *testing.T) {
 	expectPrintedTarget(t, 2019, "a[b] ??= c", "var _a;\n(_a = a[b]) != null ? _a : a[b] = c;\n")
 	expectPrintedTarget(t, 2019, "a()[b()] ??= c", "var _a, _b, _c;\n(_c = (_a = a())[_b = b()]) != null ? _c : _a[_b] = c;\n")
 
+	expectPrintedTarget(t, 2019, "class Foo { #x; constructor() { this.#x ??= 2 } }", `var _x;
+class Foo {
+  constructor() {
+    __privateAdd(this, _x, void 0);
+    var _a;
+    (_a = __privateGet(this, _x)) != null ? _a : __privateSet(this, _x, 2);
+  }
+}
+_x = new WeakMap();
+`)
+
 	expectPrintedTarget(t, 2020, "a ??= b", "a ?? (a = b);\n")
 	expectPrintedTarget(t, 2020, "a.b ??= c", "a.b ?? (a.b = c);\n")
 	expectPrintedTarget(t, 2020, "a().b ??= c", "var _a;\n(_a = a()).b ?? (_a.b = c);\n")
 	expectPrintedTarget(t, 2020, "a[b] ??= c", "a[b] ?? (a[b] = c);\n")
 	expectPrintedTarget(t, 2020, "a()[b()] ??= c", "var _a, _b;\n(_a = a())[_b = b()] ?? (_a[_b] = c);\n")
 
+	expectPrintedTarget(t, 2020, "class Foo { #x; constructor() { this.#x ??= 2 } }", `var _x;
+class Foo {
+  constructor() {
+    __privateAdd(this, _x, void 0);
+    __privateGet(this, _x) ?? __privateSet(this, _x, 2);
+  }
+}
+_x = new WeakMap();
+`)
+
 	expectPrintedTarget(t, 2021, "a ??= b", "a ??= b;\n")
 	expectPrintedTarget(t, 2021, "a.b ??= c", "a.b ??= c;\n")
 	expectPrintedTarget(t, 2021, "a().b ??= c", "a().b ??= c;\n")
 	expectPrintedTarget(t, 2021, "a[b] ??= c", "a[b] ??= c;\n")
 	expectPrintedTarget(t, 2021, "a()[b()] ??= c", "a()[b()] ??= c;\n")
+
+	expectPrintedTarget(t, 2021, "class Foo { #x; constructor() { this.#x ??= 2 } }", `var _x;
+class Foo {
+  constructor() {
+    __privateAdd(this, _x, void 0);
+    __privateGet(this, _x) ?? __privateSet(this, _x, 2);
+  }
+}
+_x = new WeakMap();
+`)
 }
 
 func TestLowerLogicalAssign(t *testing.T) {
@@ -88,17 +119,63 @@ func TestLowerLogicalAssign(t *testing.T) {
 	expectPrintedTarget(t, 2020, "a[b] &&= c", "a[b] && (a[b] = c);\n")
 	expectPrintedTarget(t, 2020, "a()[b()] &&= c", "var _a, _b;\n(_a = a())[_b = b()] && (_a[_b] = c);\n")
 
+	expectPrintedTarget(t, 2020, "class Foo { #x; constructor() { this.#x &&= 2 } }", `var _x;
+class Foo {
+  constructor() {
+    __privateAdd(this, _x, void 0);
+    __privateGet(this, _x) && __privateSet(this, _x, 2);
+  }
+}
+_x = new WeakMap();
+`)
+
+	expectPrintedTarget(t, 2021, "a &&= b", "a &&= b;\n")
+	expectPrintedTarget(t, 2021, "a.b &&= c", "a.b &&= c;\n")
+	expectPrintedTarget(t, 2021, "a().b &&= c", "a().b &&= c;\n")
+	expectPrintedTarget(t, 2021, "a[b] &&= c", "a[b] &&= c;\n")
+	expectPrintedTarget(t, 2021, "a()[b()] &&= c", "a()[b()] &&= c;\n")
+
+	expectPrintedTarget(t, 2021, "class Foo { #x; constructor() { this.#x &&= 2 } }", `var _x;
+class Foo {
+  constructor() {
+    __privateAdd(this, _x, void 0);
+    __privateGet(this, _x) && __privateSet(this, _x, 2);
+  }
+}
+_x = new WeakMap();
+`)
+
 	expectPrintedTarget(t, 2020, "a ||= b", "a || (a = b);\n")
 	expectPrintedTarget(t, 2020, "a.b ||= c", "a.b || (a.b = c);\n")
 	expectPrintedTarget(t, 2020, "a().b ||= c", "var _a;\n(_a = a()).b || (_a.b = c);\n")
 	expectPrintedTarget(t, 2020, "a[b] ||= c", "a[b] || (a[b] = c);\n")
 	expectPrintedTarget(t, 2020, "a()[b()] ||= c", "var _a, _b;\n(_a = a())[_b = b()] || (_a[_b] = c);\n")
 
+	expectPrintedTarget(t, 2020, "class Foo { #x; constructor() { this.#x ||= 2 } }", `var _x;
+class Foo {
+  constructor() {
+    __privateAdd(this, _x, void 0);
+    __privateGet(this, _x) || __privateSet(this, _x, 2);
+  }
+}
+_x = new WeakMap();
+`)
+
 	expectPrintedTarget(t, 2021, "a ||= b", "a ||= b;\n")
 	expectPrintedTarget(t, 2021, "a.b ||= c", "a.b ||= c;\n")
 	expectPrintedTarget(t, 2021, "a().b ||= c", "a().b ||= c;\n")
 	expectPrintedTarget(t, 2021, "a[b] ||= c", "a[b] ||= c;\n")
 	expectPrintedTarget(t, 2021, "a()[b()] ||= c", "a()[b()] ||= c;\n")
+
+	expectPrintedTarget(t, 2021, "class Foo { #x; constructor() { this.#x ||= 2 } }", `var _x;
+class Foo {
+  constructor() {
+    __privateAdd(this, _x, void 0);
+    __privateGet(this, _x) || __privateSet(this, _x, 2);
+  }
+}
+_x = new WeakMap();
+`)
 }
 
 func TestLowerAsyncFunctions(t *testing.T) {