From f5f8ff895c4665b661ac6a49940e7bb7f012603b Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Fri, 29 Dec 2023 14:23:43 -0500 Subject: [PATCH] fix #3568: can reorder primitive past side-effect --- CHANGELOG.md | 21 +++++++++ internal/js_ast/js_ast_helpers.go | 8 ++-- internal/js_parser/js_parser.go | 69 +++++++++++++++------------- internal/js_parser/js_parser_test.go | 12 ++++- internal/js_printer/js_printer.go | 2 +- 5 files changed, 72 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c912b09f89..ac399325acc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,27 @@ } ``` +* Minifier: allow reording a primitive past a side-effect ([#3568](https://github.com/evanw/esbuild/issues/3568)) + + The minifier previously allowed reordering a side-effect past a primitive, but didn't handle the case of reordering a primitive past a side-effect. This additional case is now handled: + + ```js + // Original code + function f() { + let x = false; + let y = x; + const boolean = y; + let frag = $.template(`

hello world

`); + return frag; + } + + // Old output (with --minify) + function f(){const e=!1;return $.template(`

hello world

`)} + + // New output (with --minify) + function f(){return $.template('

hello world

')} + ``` + * 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. diff --git a/internal/js_ast/js_ast_helpers.go b/internal/js_ast/js_ast_helpers.go index 3b38a475de8..901d1eac592 100644 --- a/internal/js_ast/js_ast_helpers.go +++ b/internal/js_ast/js_ast_helpers.go @@ -1696,7 +1696,7 @@ func FoldStringAddition(left Expr, right Expr, kind StringAdditionKind) Expr { // // This function intentionally avoids mutating the input AST so it can be // called after the AST has been frozen (i.e. after parsing ends). -func InlineStringsAndNumbersIntoTemplate(loc logger.Loc, e *ETemplate) Expr { +func InlinePrimitivesIntoTemplate(loc logger.Loc, e *ETemplate) Expr { // Can't inline strings if there's a custom template tag if e.TagOrNil.Data != nil { return Expr{Loc: loc, Data: e} @@ -1709,10 +1709,8 @@ func InlineStringsAndNumbersIntoTemplate(loc logger.Loc, e *ETemplate) Expr { if value, ok := part.Value.Data.(*EInlinedEnum); ok { part.Value = value.Value } - if value, ok := part.Value.Data.(*ENumber); ok { - if str, ok := TryToStringOnNumberSafely(value.Value, 10); ok { - part.Value.Data = &EString{Value: helpers.StringToUTF16(str)} - } + if str, ok := ToStringWithoutSideEffects(part.Value.Data); ok { + part.Value.Data = &EString{Value: helpers.StringToUTF16(str)} } if str, ok := part.Value.Data.(*EString); ok { if len(parts) == 0 { diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 9751e481d6f..d37e3cceb71 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -8732,34 +8732,40 @@ func (p *parser) mangleStmts(stmts []js_ast.Stmt, kind stmtsKind) []js_ast.Stmt // by now since they are scoped to this block which we just finished // visiting. if prevS, ok := result[len(result)-1].Data.(*js_ast.SLocal); ok && prevS.Kind != js_ast.LocalVar { - // The variable must be initialized, since we will be substituting - // the value into the usage. - if last := prevS.Decls[len(prevS.Decls)-1]; last.ValueOrNil.Data != nil { - // The binding must be an identifier that is only used once. - // Ignore destructuring bindings since that's not the simple case. - // Destructuring bindings could potentially execute side-effecting - // code which would invalidate reordering. - if id, ok := last.Binding.Data.(*js_ast.BIdentifier); ok { - // Don't do this if "__name" was called on this symbol. In that - // case there is actually more than one use even though it says - // there is only one. The "__name" use isn't counted so that - // tree shaking still works when names are kept. - if symbol := p.symbols[id.Ref.InnerIndex]; symbol.UseCountEstimate == 1 && !symbol.Flags.Has(ast.DidKeepName) { - // Try to substitute the identifier with the initializer. This will - // fail if something with side effects is in between the declaration - // and the usage. - if p.substituteSingleUseSymbolInStmt(stmt, id.Ref, last.ValueOrNil) { - // Remove the previous declaration, since the substitution was - // successful. - if len(prevS.Decls) == 1 { - result = result[:len(result)-1] - } else { - prevS.Decls = prevS.Decls[:len(prevS.Decls)-1] - } + last := prevS.Decls[len(prevS.Decls)-1] + + // The binding must be an identifier that is only used once. + // Ignore destructuring bindings since that's not the simple case. + // Destructuring bindings could potentially execute side-effecting + // code which would invalidate reordering. + if id, ok := last.Binding.Data.(*js_ast.BIdentifier); ok { + // Don't do this if "__name" was called on this symbol. In that + // case there is actually more than one use even though it says + // there is only one. The "__name" use isn't counted so that + // tree shaking still works when names are kept. + if symbol := p.symbols[id.Ref.InnerIndex]; symbol.UseCountEstimate == 1 && !symbol.Flags.Has(ast.DidKeepName) { + replacement := last.ValueOrNil + + // The variable must be initialized, since we will be substituting + // the value into the usage. + if replacement.Data == nil { + replacement = js_ast.Expr{Loc: last.Binding.Loc, Data: js_ast.EUndefinedShared} + } - // Loop back to try again - continue + // Try to substitute the identifier with the initializer. This will + // fail if something with side effects is in between the declaration + // and the usage. + if p.substituteSingleUseSymbolInStmt(stmt, id.Ref, replacement) { + // Remove the previous declaration, since the substitution was + // successful. + if len(prevS.Decls) == 1 { + result = result[:len(result)-1] + } else { + prevS.Decls = prevS.Decls[:len(prevS.Decls)-1] } + + // Loop back to try again + continue } } } @@ -9437,10 +9443,9 @@ func (p *parser) substituteSingleUseSymbolInExpr( if value, status := p.substituteSingleUseSymbolInExpr(part.Value, ref, replacement, replacementCanBeRemoved); status != substituteContinue { e.Parts[i].Value = value - // If we substituted a string or number, merge it into the template - switch value.Data.(type) { - case *js_ast.EString, *js_ast.ENumber: - expr = js_ast.InlineStringsAndNumbersIntoTemplate(expr.Loc, e) + // If we substituted a primitive, merge it into the template + if js_ast.IsPrimitiveLiteral(value.Data) { + expr = js_ast.InlinePrimitivesIntoTemplate(expr.Loc, e) } return expr, status } @@ -9454,7 +9459,7 @@ func (p *parser) substituteSingleUseSymbolInExpr( } // We can always reorder past primitive values - if js_ast.IsPrimitiveLiteral(expr.Data) { + if js_ast.IsPrimitiveLiteral(expr.Data) || js_ast.IsPrimitiveLiteral(replacement.Data) { return expr, substituteContinue } @@ -13268,7 +13273,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO // it may no longer be a template literal after this point (it may turn into // a plain string literal instead). if p.shouldFoldTypeScriptConstantExpressions || p.options.minifySyntax { - expr = js_ast.InlineStringsAndNumbersIntoTemplate(expr.Loc, e) + expr = js_ast.InlinePrimitivesIntoTemplate(expr.Loc, e) } shouldLowerTemplateLiteral := p.options.unsupportedJSFeatures.Has(compat.TemplateLiteral) diff --git a/internal/js_parser/js_parser_test.go b/internal/js_parser/js_parser_test.go index 79f432188e0..e2ae3d9674a 100644 --- a/internal/js_parser/js_parser_test.go +++ b/internal/js_parser/js_parser_test.go @@ -4974,15 +4974,23 @@ func TestMangleInlineLocals(t *testing.T) { check("let x = 1; return void x", "let x = 1;") check("let x = 1; return typeof x", "return typeof 1;") + // Can substitute into template literals + check("let x = 1; return `<${x}>`", "return `<1>`;") + check("let x = 1n; return `<${x}>`", "return `<1>`;") + check("let x = null; return `<${x}>`", "return ``;") + check("let x = undefined; return `<${x}>`", "return ``;") + check("let x = false; return `<${x}>`", "return ``;") + check("let x = true; return `<${x}>`", "return ``;") + // Check substituting a side-effect free value into normal binary operators check("let x = 1; return x + 2", "return 1 + 2;") check("let x = 1; return 2 + x", "return 2 + 1;") check("let x = 1; return x + arg0", "return 1 + arg0;") check("let x = 1; return arg0 + x", "return arg0 + 1;") check("let x = 1; return x + fn()", "return 1 + fn();") - check("let x = 1; return fn() + x", "let x = 1;\nreturn fn() + x;") + check("let x = 1; return fn() + x", "return fn() + 1;") check("let x = 1; return x + undef", "return 1 + undef;") - check("let x = 1; return undef + x", "let x = 1;\nreturn undef + x;") + check("let x = 1; return undef + x", "return undef + 1;") // Check substituting a value with side-effects into normal binary operators check("let x = fn(); return x + 2", "return fn() + 2;") diff --git a/internal/js_printer/js_printer.go b/internal/js_printer/js_printer.go index 08e888b3946..4dd481d3ec9 100644 --- a/internal/js_printer/js_printer.go +++ b/internal/js_printer/js_printer.go @@ -2999,7 +2999,7 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla if replaced != nil { copy := *e copy.Parts = replaced - switch e2 := js_ast.InlineStringsAndNumbersIntoTemplate(logger.Loc{}, ©).Data.(type) { + switch e2 := js_ast.InlinePrimitivesIntoTemplate(logger.Loc{}, ©).Data.(type) { case *js_ast.EString: p.printQuotedUTF16(e2.Value, printQuotedAllowBacktick) return