From 9493e3b5c9432df91ca914cd8a5dd19af6cf18a7 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Thu, 8 Dec 2022 00:06:37 -0500 Subject: [PATCH] fix #2719: avoid `define` creating syntax errors --- CHANGELOG.md | 21 +++++ internal/bundler/bundler_default_test.go | 69 ++++++++++++++ .../bundler/snapshots/snapshots_default.txt | 18 ++++ internal/helpers/quote.go | 36 ++++++- internal/js_parser/js_parser.go | 93 +++++++++++++++++-- internal/logger/msg_ids.go | 5 + 6 files changed, 230 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 770e41706ee..7b23d497b00 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/internal/bundler/bundler_default_test.go b/internal/bundler/bundler_default_test.go index 7d26746b534..bd5bd72121f 100644 --- a/internal/bundler/bundler_default_test.go +++ b/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{ diff --git a/internal/bundler/snapshots/snapshots_default.txt b/internal/bundler/snapshots/snapshots_default.txt index a65df6b76bd..b58648fa0ce 100644 --- a/internal/bundler/snapshots/snapshots_default.txt +++ b/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 ---------- diff --git a/internal/helpers/quote.go b/internal/helpers/quote.go index 704982acfbc..a505ad4a78c 100644 --- a/internal/helpers/quote.go +++ b/internal/helpers/quote.go @@ -17,7 +17,15 @@ 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 { @@ -25,8 +33,16 @@ func QuoteForJSON(text string, asciiOnly bool) []byte { 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) } diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 7bbc9b9ae52..c69f14041c4 100644 --- a/internal/js_parser/js_parser.go +++ b/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 diff --git a/internal/logger/msg_ids.go b/internal/logger/msg_ids.go index 43353fb651c..94c232eb749 100644 --- a/internal/logger/msg_ids.go +++ b/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: