fix #2719: avoid define creating syntax errors

CHANGELOG.md

@@ -2,6 +2,27 @@
 
 ## Unreleased
 
+* Prevent `define` from substituting constants into assignment position ([#2719](https://github.com/evanw/esbuild/issues/2719))
+
+    The `define` feature lets you replace certain expressions with constants. For example, you could use it to replace references to the global property reference `window.DEBUG` with `false` at compile time, which can then potentially help esbuild remove unused code from your bundle. It's similar to [DefinePlugin](https://webpack.js.org/plugins/define-plugin/) in Webpack.
+
+    However, if you write code such as `window.DEBUG = true` and then defined `window.DEBUG` to `false`, esbuild previously generated the output `false = true` which is a syntax error in JavaScript. This behavior is not typically a problem because it doesn't make sense to substitute `window.DEBUG` with a constant if its value changes at run-time (Webpack's `DefinePlugin` also generates `false = true` in this case). But it can be alarming to have esbuild generate code with a syntax error.
+
+    So with this release, esbuild will no longer substitute `define` constants into assignment position to avoid generating code with a syntax error. Instead esbuild will generate a warning, which currently looks like this:
+
+    ```
+    ▲ [WARNING] Suspicious assignment to defined constant "window.DEBUG" [assign-to-define]
+
+        example.js:1:0:
+          1 │ window.DEBUG = true
+            ╵ ~~~~~~~~~~~~
+
+      The expression "window.DEBUG" has been configured to be replaced with a constant using the
+      "define" feature. If this expression is supposed to be a compile-time constant, then it doesn't
+      make sense to assign to it here. Or if this expression is supposed to change at run-time, this
+      "define" substitution should be removed.
+    ```
+
 * Fix a regression with `npm install --no-optional` ([#2720](https://github.com/evanw/esbuild/issues/2720))
 
     Normally when you install esbuild with `npm install`, npm itself is the tool that downloads the correct binary executable for the current platform. This happens because of how esbuild's primary package uses npm's `optionalDependencies` feature. However, if you deliberately disable this with `npm install --no-optional` then esbuild's install script will attempt to repair the installation by manually downloading and extracting the binary executable from the package that was supposed to be installed.

internal/bundler/bundler_default_test.go

@@ -4764,6 +4764,75 @@ func TestDefineInfiniteLoopIssue2407(t *testing.T) {
 	})
 }
 
+func TestDefineAssignWarning(t *testing.T) {
+	defines := config.ProcessDefines(map[string]config.DefineData{
+		"a": {
+			DefineExpr: &config.DefineExpr{
+				Constant: js_ast.ENullShared,
+			},
+		},
+		"b.c": {
+			DefineExpr: &config.DefineExpr{
+				Constant: js_ast.ENullShared,
+			},
+		},
+		"d": {
+			DefineExpr: &config.DefineExpr{
+				Parts: []string{"ident"},
+			},
+		},
+		"e.f": {
+			DefineExpr: &config.DefineExpr{
+				Parts: []string{"ident"},
+			},
+		},
+		"g": {
+			DefineExpr: &config.DefineExpr{
+				Parts: []string{"dot", "chain"},
+			},
+		},
+		"h.i": {
+			DefineExpr: &config.DefineExpr{
+				Parts: []string{"dot", "chain"},
+			},
+		},
+	})
+	default_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/read.js": `
+				console.log(
+					[a, b.c, b['c']],
+					[d, e.f, e['f']],
+					[g, h.i, h['i']],
+				)
+			`,
+			"/write.js": `
+				console.log(
+					[a = 0, b.c = 0, b['c'] = 0],
+					[d = 0, e.f = 0, e['f'] = 0],
+					[g = 0, h.i = 0, h['i'] = 0],
+				)
+			`,
+		},
+		entryPaths: []string{
+			"/read.js",
+			"/write.js",
+		},
+		options: config.Options{
+			Mode:         config.ModeBundle,
+			AbsOutputDir: "/out",
+			Defines:      &defines,
+		},
+		expectedScanLog: `write.js: WARNING: Suspicious assignment to defined constant "a"
+NOTE: The expression "a" has been configured to be replaced with a constant using the "define" feature. If this expression is supposed to be a compile-time constant, then it doesn't make sense to assign to it here. Or if this expression is supposed to change at run-time, this "define" substitution should be removed.
+write.js: WARNING: Suspicious assignment to defined constant "b.c"
+NOTE: The expression "b.c" has been configured to be replaced with a constant using the "define" feature. If this expression is supposed to be a compile-time constant, then it doesn't make sense to assign to it here. Or if this expression is supposed to change at run-time, this "define" substitution should be removed.
+write.js: WARNING: Suspicious assignment to defined constant "b['c']"
+NOTE: The expression "b['c']" has been configured to be replaced with a constant using the "define" feature. If this expression is supposed to be a compile-time constant, then it doesn't make sense to assign to it here. Or if this expression is supposed to change at run-time, this "define" substitution should be removed.
+`,
+	})
+}
+
 func TestKeepNamesTreeShaking(t *testing.T) {
 	default_suite.expectBundled(t, bundled{
 		files: map[string]string{

internal/bundler/snapshots/snapshots_default.txt

@@ -395,6 +395,24 @@ for (const d in x)
 for (const e of x)
   console.log(e);
 
+================================================================================
+TestDefineAssignWarning
+---------- /out/read.js ----------
+// read.js
+console.log(
+  [null, null, null],
+  [ident, ident, ident],
+  [dot.chain, dot.chain, dot.chain]
+);
+
+---------- /out/write.js ----------
+// write.js
+console.log(
+  [a = 0, b.c = 0, b["c"] = 0],
+  [ident = 0, ident = 0, ident = 0],
+  [dot.chain = 0, dot.chain = 0, dot.chain = 0]
+);
+
 ================================================================================
 TestDefineImportMeta
 ---------- /out.js ----------

internal/helpers/quote.go

@@ -17,16 +17,32 @@ func canPrintWithoutEscape(c rune, asciiOnly bool) bool {
 	}
 }
 
+func QuoteSingle(text string, asciiOnly bool) []byte {
+	return internalQuote(text, asciiOnly, '\'')
+}
+
 func QuoteForJSON(text string, asciiOnly bool) []byte {
+	return internalQuote(text, asciiOnly, '"')
+}
+
+func internalQuote(text string, asciiOnly bool, quoteChar byte) []byte {
 	// Estimate the required length
 	lenEstimate := 2
 	for _, c := range text {
 		if canPrintWithoutEscape(c, asciiOnly) {
 			lenEstimate += utf8.RuneLen(c)
 		} else {
 			switch c {
-			case '\b', '\f', '\n', '\r', '\t', '\\', '"':
+			case '\b', '\f', '\n', '\r', '\t', '\\':
 				lenEstimate += 2
+			case '"':
+				if quoteChar == '"' {
+					lenEstimate += 2
+				}
+			case '\'':
+				if quoteChar == '\'' {
+					lenEstimate += 2
+				}
 			default:
 				if c <= 0xFFFF {
 					lenEstimate += 6
@@ -41,7 +57,7 @@ func QuoteForJSON(text string, asciiOnly bool) []byte {
 	bytes := make([]byte, 0, lenEstimate)
 	i := 0
 	n := len(text)
-	bytes = append(bytes, '"')
+	bytes = append(bytes, quoteChar)
 
 	for i < n {
 		c, width := DecodeWTF8Rune(text[i:])
@@ -87,7 +103,19 @@ func QuoteForJSON(text string, asciiOnly bool) []byte {
 			i++
 
 		case '"':
-			bytes = append(bytes, "\\\""...)
+			if quoteChar == '"' {
+				bytes = append(bytes, "\\\""...)
+			} else {
+				bytes = append(bytes, '"')
+			}
+			i++
+
+		case '\'':
+			if quoteChar == '\'' {
+				bytes = append(bytes, "\\'"...)
+			} else {
+				bytes = append(bytes, '\'')
+			}
 			i++
 
 		default:
@@ -110,5 +138,5 @@ func QuoteForJSON(text string, asciiOnly bool) []byte {
 		}
 	}
 
-	return append(bytes, '"')
+	return append(bytes, quoteChar)
 }

internal/js_parser/js_parser.go

@@ -1749,6 +1749,64 @@ func (p *parser) logDeferredArrowArgErrors(errors *deferredErrors) {
 	}
 }
 
+func defineValueCanBeUsedInAssignTarget(data js_ast.E) bool {
+	switch data.(type) {
+	case *js_ast.EIdentifier, *js_ast.EDot:
+		return true
+	}
+
+	// Substituting a constant into an assignment target (e.g. "x = 1" becomes
+	// "0 = 1") will cause a syntax error, so we avoid doing this. The caller
+	// will log a warning instead.
+	return false
+}
+
+func (p *parser) logAssignToDefine(r logger.Range, name string, expr js_ast.Expr) {
+	// If this is a compound expression, pretty-print it for the error message.
+	// We don't use a literal slice of the source text in case it contains
+	// problematic things (e.g. spans multiple lines, has embedded comments).
+	if expr.Data != nil {
+		var parts []string
+		for {
+			if id, ok := expr.Data.(*js_ast.EIdentifier); ok {
+				parts = append(parts, p.loadNameFromRef(id.Ref))
+				break
+			} else if dot, ok := expr.Data.(*js_ast.EDot); ok {
+				parts = append(parts, dot.Name)
+				parts = append(parts, ".")
+				expr = dot.Target
+			} else if index, ok := expr.Data.(*js_ast.EIndex); ok {
+				if str, ok := index.Index.Data.(*js_ast.EString); ok {
+					parts = append(parts, "]")
+					parts = append(parts, string(helpers.QuoteSingle(helpers.UTF16ToString(str.Value), false)))
+					parts = append(parts, "[")
+					expr = index.Target
+				} else {
+					return
+				}
+			} else {
+				return
+			}
+		}
+		for i, j := 0, len(parts)-1; i < j; i, j = i+1, j-1 {
+			parts[i], parts[j] = parts[j], parts[i]
+		}
+		name = strings.Join(parts, "")
+	}
+
+	kind := logger.Warning
+	if p.suppressWarningsAboutWeirdCode {
+		kind = logger.Debug
+	}
+
+	p.log.AddIDWithNotes(logger.MsgID_JS_AssignToDefine, kind, &p.tracker, r,
+		fmt.Sprintf("Suspicious assignment to defined constant %q", name),
+		[]logger.MsgData{{Text: fmt.Sprintf(
+			"The expression %q has been configured to be replaced with a constant using the \"define\" feature. "+
+				"If this expression is supposed to be a compile-time constant, then it doesn't make sense to assign to it here. "+
+				"Or if this expression is supposed to change at run-time, this \"define\" substitution should be removed.", name)}})
+}
+
 // The "await" and "yield" expressions are never allowed in argument lists but
 // may or may not be allowed otherwise depending on the details of the enclosing
 // function or module. This needs to be handled when parsing an arrow function
@@ -12301,12 +12359,11 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 						isCallTarget:   isCallTarget,
 						isDeleteTarget: isDeleteTarget,
 					})
-
-					// Don't substitute an identifier for a non-identifier if this is an
-					// assignment target, since it'll cause a syntax error
-					if _, ok := new.Data.(*js_ast.EIdentifier); in.assignTarget == js_ast.AssignTargetNone || ok {
+					if in.assignTarget == js_ast.AssignTargetNone || defineValueCanBeUsedInAssignTarget(new.Data) {
 						p.ignoreUsage(e.Ref)
 						return new, exprOut{}
+					} else {
+						p.logAssignToDefine(js_lexer.RangeOfIdentifier(p.source, expr.Loc), name, js_ast.Expr{})
 					}
 				}
 
@@ -13272,11 +13329,19 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 				if p.isDotOrIndexDefineMatch(expr, define.Parts) {
 					// Substitute user-specified defines
 					if define.Data.DefineExpr != nil {
-						return p.instantiateDefineExpr(expr.Loc, *define.Data.DefineExpr, identifierOpts{
+						new := p.instantiateDefineExpr(expr.Loc, *define.Data.DefineExpr, identifierOpts{
 							assignTarget:   in.assignTarget,
 							isCallTarget:   isCallTarget,
 							isDeleteTarget: isDeleteTarget,
-						}), exprOut{}
+						})
+						if in.assignTarget == js_ast.AssignTargetNone || defineValueCanBeUsedInAssignTarget(new.Data) {
+							// Note: We don't need to "ignoreRef" on the underlying identifier
+							// because we have only parsed it but not visited it yet
+							return new, exprOut{}
+						} else {
+							r := logger.Range{Loc: expr.Loc, Len: js_lexer.RangeOfIdentifier(p.source, e.NameLoc).End() - expr.Loc.Start}
+							p.logAssignToDefine(r, "", expr)
+						}
 					}
 
 					// Copy the side effect flags over in case this expression is unused
@@ -13370,11 +13435,23 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 					if p.isDotOrIndexDefineMatch(expr, define.Parts) {
 						// Substitute user-specified defines
 						if define.Data.DefineExpr != nil {
-							return p.instantiateDefineExpr(expr.Loc, *define.Data.DefineExpr, identifierOpts{
+							new := p.instantiateDefineExpr(expr.Loc, *define.Data.DefineExpr, identifierOpts{
 								assignTarget:   in.assignTarget,
 								isCallTarget:   isCallTarget,
 								isDeleteTarget: isDeleteTarget,
-							}), exprOut{}
+							})
+							if in.assignTarget == js_ast.AssignTargetNone || defineValueCanBeUsedInAssignTarget(new.Data) {
+								// Note: We don't need to "ignoreRef" on the underlying identifier
+								// because we have only parsed it but not visited it yet
+								return new, exprOut{}
+							} else {
+								r := logger.Range{Loc: expr.Loc}
+								afterIndex := logger.Loc{Start: p.source.RangeOfString(e.Index.Loc).End()}
+								if closeBracket := p.source.RangeOfOperatorAfter(afterIndex, "]"); closeBracket.Len > 0 {
+									r.Len = closeBracket.End() - r.Loc.Start
+								}
+								p.logAssignToDefine(r, "", expr)
+							}
 						}
 
 						// Copy the side effect flags over in case this expression is unused

internal/logger/msg_ids.go

@@ -14,6 +14,7 @@ const (
 	// JavaScript
 	MsgID_JS_AssertTypeJSON
 	MsgID_JS_AssignToConstant
+	MsgID_JS_AssignToDefine
 	MsgID_JS_AssignToImport
 	MsgID_JS_CallImportNamespace
 	MsgID_JS_CommonJSVariableInESM
@@ -92,6 +93,8 @@ func StringToMsgIDs(str string, logLevel LogLevel, overrides map[MsgID]LogLevel)
 		overrides[MsgID_JS_AssertTypeJSON] = logLevel
 	case "assign-to-constant":
 		overrides[MsgID_JS_AssignToConstant] = logLevel
+	case "assign-to-define":
+		overrides[MsgID_JS_AssignToDefine] = logLevel
 	case "assign-to-import":
 		overrides[MsgID_JS_AssignToImport] = logLevel
 	case "call-import-namespace":
@@ -206,6 +209,8 @@ func MsgIDToString(id MsgID) string {
 		return "assert-type-json"
 	case MsgID_JS_AssignToConstant:
 		return "assign-to-constant"
+	case MsgID_JS_AssignToDefine:
+		return "assign-to-define"
 	case MsgID_JS_AssignToImport:
 		return "assign-to-import"
 	case MsgID_JS_CallImportNamespace: