destructuring should be a side effect (#1183)

CHANGELOG.md

@@ -53,6 +53,17 @@
 
     More detail: When Go panics, it prints a stack trace to stderr (i.e. file descriptor 2). Go's WebAssembly shim calls out to node's `fs.writeSync()` function to do this, and it converts calls to `fs.writeSync()` into calls to `console.log()` in the browser by providing a shim for `fs`. However, Go's shim code stores the shim on `window.fs` in the browser. This is undesirable because it pollutes the global scope and leads to brittle code that can break if other code also uses `window.fs`. To avoid this, esbuild shadows the global object by wrapping Go's shim. But that broke bare references to `fs` since the shim is no longer stored on `window.fs`. This release now stores the shim in a local variable named `fs` so that bare references to `fs` work correctly.
 
+* Undo incorrect dead-code elimination with destructuring ([#1183](https://github.com/evanw/esbuild/issues/1183))
+
+    Previously esbuild eliminated these statements as dead code if tree-shaking was enabled:
+
+    ```js
+    let [a] = {}
+    let { b } = null
+    ```
+
+    This is incorrect because both of these lines will throw an error when evaluated. With this release, esbuild now preserves these statements even when tree shaking is enabled.
+
 * Update to Go 1.17.7
 
     The version of the Go compiler used to compile esbuild has been upgraded from Go 1.17.6 to Go 1.17.7, which contains a few [compiler and security bug fixes](https://github.com/golang/go/issues?q=milestone%3AGo1.17.7+label%3ACherryPickApproved).

internal/js_parser/js_parser.go

@@ -14406,7 +14406,7 @@ func (p *parser) stmtsCanBeRemovedIfUnused(stmts []js_ast.Stmt) bool {
 
 		case *js_ast.SLocal:
 			for _, decl := range s.Decls {
-				if !p.bindingCanBeRemovedIfUnused(decl.Binding) {
+				if _, ok := decl.Binding.Data.(*js_ast.BIdentifier); !ok {
 					return false
 				}
 				if decl.ValueOrNil.Data != nil && !p.exprCanBeRemovedIfUnused(decl.ValueOrNil) {
@@ -14485,35 +14485,6 @@ func (p *parser) classCanBeRemovedIfUnused(class js_ast.Class) bool {
 	return true
 }
 
-func (p *parser) bindingCanBeRemovedIfUnused(binding js_ast.Binding) bool {
-	switch b := binding.Data.(type) {
-	case *js_ast.BArray:
-		for _, item := range b.Items {
-			if !p.bindingCanBeRemovedIfUnused(item.Binding) {
-				return false
-			}
-			if item.DefaultValueOrNil.Data != nil && !p.exprCanBeRemovedIfUnused(item.DefaultValueOrNil) {
-				return false
-			}
-		}
-
-	case *js_ast.BObject:
-		for _, property := range b.Properties {
-			if !property.IsSpread && !p.exprCanBeRemovedIfUnused(property.Key) {
-				return false
-			}
-			if !p.bindingCanBeRemovedIfUnused(property.Value) {
-				return false
-			}
-			if property.DefaultValueOrNil.Data != nil && !p.exprCanBeRemovedIfUnused(property.DefaultValueOrNil) {
-				return false
-			}
-		}
-	}
-
-	return true
-}
-
 func (p *parser) exprCanBeRemovedIfUnused(expr js_ast.Expr) bool {
 	switch e := expr.Data.(type) {
 	case *js_ast.EInlinedEnum:

scripts/end-to-end-tests.js

@@ -3055,6 +3055,34 @@
       'foo/dir/x.js': `global.dce6 = 123`,
       'foo/package.json': `{ "main": "dir/x", "sideEffects": ["**/x.*"] }`,
     }),
+
+    // Test side effect detection for destructuring
+    test(['--bundle', 'entry.js', '--outfile=out.js'], {
+      'entry.js': `
+        let [a] = {}; // This must not be tree-shaken
+      `,
+      'node.js': `
+        pass: {
+          try {
+            require('./out.js')
+          } catch (e) {
+            break pass
+          }
+          throw 'fail'
+        }
+      `,
+    }),
+    test(['--bundle', 'entry.js', '--outfile=node.js'], {
+      'entry.js': `
+        let sideEffect = false
+        let { a } = { // This must not be tree-shaken
+          get a() {
+            sideEffect = true
+          },
+        };
+        if (!sideEffect) throw 'fail'
+      `,
+    }),
   )
 
   // Test obscure CommonJS symbol edge cases