fix #3561: treeshaking of known Symbol instances

Author: evanw

CHANGELOG.md

@@ -64,6 +64,17 @@
     function f(){return $.template('<p contenteditable="false">hello world</p>')}
     ```
 
+* Minifier: consider properties named using known `Symbol` instances to be side-effect free ([#3561](https://github.com/evanw/esbuild/issues/3561))
+
+    Many things in JavaScript can have side effects including property accesses and ToString operations, so using a symbol such as `Symbol.iterator` as a computed property name is not obviously side-effect free. This release adds a special case for known `Symbol` instances so that they are considered side-effect free when used as property names. For example, this class declaration will now be considered side-effect free:
+
+    ```js
+    class Foo {
+      *[Symbol.iterator]() {
+      }
+    }
+    ```
+
 * Provide the `stop()` API in node to exit esbuild's child process ([#3558](https://github.com/evanw/esbuild/issues/3558))
 
     You can now call `stop()` in esbuild's node API to exit esbuild's child process to reclaim the resources used. It only makes sense to do this for a long-lived node process when you know you will no longer be making any more esbuild API calls. It is not necessary to call this to allow node to exit, and it's advantageous to not call this in between calls to esbuild's API as sharing a single long-lived esbuild child process is more efficient than re-creating a new esbuild child process for every API call. This API call used to exist but was removed in [version 0.9.0](https://github.com/evanw/esbuild/releases/v0.9.0). This release adds it back due to a user request.

internal/bundler_tests/bundler_dce_test.go

@@ -4595,3 +4595,36 @@ func TestDropLabelTreeShakingBugIssue3311(t *testing.T) {
 		},
 	})
 }
+
+func TestDCEOfSymbolInstances(t *testing.T) {
+	dce_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/class.js": `
+				class Remove1 {}
+				class Remove2 { *[Symbol.iterator]() {} }
+				class Remove3 { *[Symbol['iterator']]() {} }
+
+				class Keep1 { *[Symbol.iterator]() {} [keep] }
+				class Keep2 { [keep]; *[Symbol.iterator]() {} }
+				class Keep3 { *[Symbol.wtf]() {} }
+			`,
+			"/object.js": `
+				let remove1 = {}
+				let remove2 = { *[Symbol.iterator]() {} }
+				let remove3 = { *[Symbol['iterator']]() {} }
+
+				let keep1 = { *[Symbol.iterator]() {}, [keep]: null }
+				let keep2 = { [keep]: null, *[Symbol.iterator]() {} }
+				let keep3 = { *[Symbol.wtf]() {} }
+			`,
+		},
+		entryPaths: []string{
+			"/class.js",
+			"/object.js",
+		},
+		options: config.Options{
+			Mode:         config.ModeBundle,
+			AbsOutputDir: "/out",
+		},
+	})
+}

internal/bundler_tests/snapshots/snapshots_dce.txt

@@ -555,6 +555,34 @@ use(isNotPure);
   } };
 })();
 
+================================================================================
+TestDCEOfSymbolInstances
+---------- /out/class.js ----------
+// class.js
+var Keep1 = class {
+  *[Symbol.iterator]() {
+  }
+  [keep];
+};
+var Keep2 = class {
+  [keep];
+  *[Symbol.iterator]() {
+  }
+};
+var Keep3 = class {
+  *[Symbol.wtf]() {
+  }
+};
+
+---------- /out/object.js ----------
+// object.js
+var keep1 = { *[Symbol.iterator]() {
+}, [keep]: null };
+var keep2 = { [keep]: null, *[Symbol.iterator]() {
+} };
+var keep3 = { *[Symbol.wtf]() {
+} };
+
 ================================================================================
 TestDCEOfUsingDeclarations
 ---------- /out/entry.js ----------

internal/config/globals.go

@@ -890,6 +890,10 @@ const (
 	// output, even when the arguments have side effects. This is used to
 	// implement the "--drop:console" flag.
 	MethodCallsMustBeReplacedWithUndefined
+
+	// Symbol values are known to not have side effects when used as property
+	// names in class declarations and object literals.
+	IsSymbolInstance
 )
 
 func (flags DefineFlags) Has(flag DefineFlags) bool {
@@ -943,7 +947,16 @@ func ProcessDefines(userDefines map[string]DefineData) ProcessedDefines {
 		if len(parts) == 1 {
 			result.IdentifierDefines[tail] = DefineData{Flags: CanBeRemovedIfUnused}
 		} else {
-			result.DotDefines[tail] = append(result.DotDefines[tail], DotDefine{Parts: parts, Data: DefineData{Flags: CanBeRemovedIfUnused}})
+			flags := CanBeRemovedIfUnused
+
+			// All properties on the "Symbol" global are currently symbol instances
+			// (i.e. "typeof Symbol.iterator === 'symbol'"). This is used to avoid
+			// treating properties with these names as having side effects.
+			if parts[0] == "Symbol" {
+				flags |= IsSymbolInstance
+			}
+
+			result.DotDefines[tail] = append(result.DotDefines[tail], DotDefine{Parts: parts, Data: DefineData{Flags: flags}})
 		}
 	}
 

internal/js_ast/js_ast.go

@@ -624,12 +624,17 @@ type EDot struct {
 	// unwrapped if the resulting value is unused. Unwrapping means discarding
 	// the call target but keeping any arguments with side effects.
 	CallCanBeUnwrappedIfUnused bool
+
+	// Symbol values are known to not have side effects when used as property
+	// names in class declarations and object literals.
+	IsSymbolInstance bool
 }
 
 func (a *EDot) HasSameFlagsAs(b *EDot) bool {
 	return a.OptionalChain == b.OptionalChain &&
 		a.CanBeRemovedIfUnused == b.CanBeRemovedIfUnused &&
-		a.CallCanBeUnwrappedIfUnused == b.CallCanBeUnwrappedIfUnused
+		a.CallCanBeUnwrappedIfUnused == b.CallCanBeUnwrappedIfUnused &&
+		a.IsSymbolInstance == b.IsSymbolInstance
 }
 
 type EIndex struct {
@@ -646,12 +651,17 @@ type EIndex struct {
 	// unwrapped if the resulting value is unused. Unwrapping means discarding
 	// the call target but keeping any arguments with side effects.
 	CallCanBeUnwrappedIfUnused bool
+
+	// Symbol values are known to not have side effects when used as property
+	// names in class declarations and object literals.
+	IsSymbolInstance bool
 }
 
 func (a *EIndex) HasSameFlagsAs(b *EIndex) bool {
 	return a.OptionalChain == b.OptionalChain &&
 		a.CanBeRemovedIfUnused == b.CanBeRemovedIfUnused &&
-		a.CallCanBeUnwrappedIfUnused == b.CallCanBeUnwrappedIfUnused
+		a.CallCanBeUnwrappedIfUnused == b.CallCanBeUnwrappedIfUnused &&
+		a.IsSymbolInstance == b.IsSymbolInstance
 }
 
 type EArrow struct {

internal/js_ast/js_ast_helpers.go

@@ -184,6 +184,17 @@ func MaybeSimplifyEqualityComparison(loc logger.Loc, e *EBinary, unsupportedFeat
 	return Expr{}, false
 }
 
+func IsSymbolInstance(data E) bool {
+	switch e := data.(type) {
+	case *EDot:
+		return e.IsSymbolInstance
+
+	case *EIndex:
+		return e.IsSymbolInstance
+	}
+	return false
+}
+
 func IsPrimitiveLiteral(data E) bool {
 	switch e := data.(type) {
 	case *EAnnotation:
@@ -2173,7 +2184,7 @@ func (ctx HelperContext) ClassCanBeRemovedIfUnused(class Class) bool {
 			return false
 		}
 
-		if property.Flags.Has(PropertyIsComputed) && !IsPrimitiveLiteral(property.Key.Data) {
+		if property.Flags.Has(PropertyIsComputed) && !IsPrimitiveLiteral(property.Key.Data) && !IsSymbolInstance(property.Key.Data) {
 			return false
 		}
 
@@ -2327,7 +2338,10 @@ func (ctx HelperContext) ExprCanBeRemovedIfUnused(expr Expr) bool {
 	case *EObject:
 		for _, property := range e.Properties {
 			// The key must still be evaluated if it's computed or a spread
-			if property.Kind == PropertySpread || (property.Flags.Has(PropertyIsComputed) && !IsPrimitiveLiteral(property.Key.Data)) {
+			if property.Kind == PropertySpread {
+				return false
+			}
+			if property.Flags.Has(PropertyIsComputed) && !IsPrimitiveLiteral(property.Key.Data) && !IsSymbolInstance(property.Key.Data) {
 				return false
 			}
 			if property.ValueOrNil.Data != nil && !ctx.ExprCanBeRemovedIfUnused(property.ValueOrNil) {

internal/js_parser/js_parser.go

@@ -13412,6 +13412,9 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 					if define.Data.Flags.Has(config.CallCanBeUnwrappedIfUnused) && !p.options.ignoreDCEAnnotations {
 						e.CallCanBeUnwrappedIfUnused = true
 					}
+					if define.Data.Flags.Has(config.IsSymbolInstance) {
+						e.IsSymbolInstance = true
+					}
 					break
 				}
 			}
@@ -13535,6 +13538,9 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 						if define.Data.Flags.Has(config.CallCanBeUnwrappedIfUnused) && !p.options.ignoreDCEAnnotations {
 							e.CallCanBeUnwrappedIfUnused = true
 						}
+						if define.Data.Flags.Has(config.IsSymbolInstance) {
+							e.IsSymbolInstance = true
+						}
 						break
 					}
 				}