From 04b65729e72c1be671f32e8674c3c9c6e22c0fc2 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Sun, 10 Jul 2022 12:56:41 -0400 Subject: [PATCH] fix #2361: keeps inlined `const` for direct `eval` --- CHANGELOG.md | 15 +++++ internal/bundler/bundler_dce_test.go | 52 +++++++++++++++ internal/bundler/snapshots/snapshots_dce.txt | 27 ++++++++ internal/js_ast/js_ast.go | 2 + internal/js_parser/js_parser.go | 67 ++++++++++++-------- 5 files changed, 137 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3263ea38873..4aea8293260 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,21 @@ ## Unreleased +* Keep inlined constants when direct `eval` is present ([#2361](https://github.com/evanw/esbuild/issues/2361)) + + Version 0.14.19 of esbuild added inlining of certain `const` variables during minification, which replaces all references to the variable with the initializer and then removes the variable declaration. However, this could generate incorrect code when direct `eval` is present because the direct `eval` could reference the constant by name. This release fixes the problem by preserving the `const` variable declaration in this case: + + ```js + // Original code + console.log((() => { const x = 123; return x + eval('x') })) + + // Old output (with --minify) + console.log(()=>123+eval("x")); + + // New output (with --minify) + console.log(()=>{const x=123;return 123+eval("x")}); + ``` + * Fix an incorrect error in TypeScript when targeting ES5 ([#2375](https://github.com/evanw/esbuild/issues/2375)) Previously when compiling TypeScript code to ES5, esbuild could incorrectly consider the following syntax forms as a transformation error: diff --git a/internal/bundler/bundler_dce_test.go b/internal/bundler/bundler_dce_test.go index 401dff4886b..670fd228479 100644 --- a/internal/bundler/bundler_dce_test.go +++ b/internal/bundler/bundler_dce_test.go @@ -3014,6 +3014,58 @@ const-update.js: NOTE: The symbol "x" was declared a constant here: }) } +func TestConstValueInliningDirectEval(t *testing.T) { + dce_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/top-level-no-eval.js": ` + const x = 1 + console.log(x, evil('x')) + `, + "/top-level-eval.js": ` + const x = 1 + console.log(x, eval('x')) + `, + "/nested-no-eval.js": ` + (() => { + const x = 1 + console.log(x, evil('x')) + })() + `, + "/nested-eval.js": ` + (() => { + const x = 1 + console.log(x, eval('x')) + })() + `, + "/ts-namespace-no-eval.ts": ` + namespace y { + export const x = 1 + console.log(x, evil('x')) + } + `, + "/ts-namespace-eval.ts": ` + namespace z { + export const x = 1 + console.log(x, eval('x')) + } + `, + }, + entryPaths: []string{ + "/top-level-no-eval.js", + "/top-level-eval.js", + "/nested-no-eval.js", + "/nested-eval.js", + "/ts-namespace-no-eval.ts", + "/ts-namespace-eval.ts", + }, + options: config.Options{ + Mode: config.ModePassThrough, + AbsOutputDir: "/out", + MinifySyntax: true, + }, + }) +} + func TestCrossModuleConstantFolding(t *testing.T) { dce_suite.expectBundled(t, bundled{ files: map[string]string{ diff --git a/internal/bundler/snapshots/snapshots_dce.txt b/internal/bundler/snapshots/snapshots_dce.txt index 62616279fe2..3b2539c7bc3 100644 --- a/internal/bundler/snapshots/snapshots_dce.txt +++ b/internal/bundler/snapshots/snapshots_dce.txt @@ -96,6 +96,33 @@ function bar() { // non-circular-export-entry.js console.log(123, bar()); +================================================================================ +TestConstValueInliningDirectEval +---------- /out/top-level-no-eval.js ---------- +const x = 1; +console.log(1, evil("x")); + +---------- /out/top-level-eval.js ---------- +const x = 1; +console.log(1, eval("x")); + +---------- /out/nested-no-eval.js ---------- +console.log(1, evil("x")); + +---------- /out/nested-eval.js ---------- +(() => { + const x = 1; + console.log(1, eval("x")); +})(); + +---------- /out/ts-namespace-no-eval.js ---------- +var y; +((y2) => (y2.x = 1, console.log(1, evil("x"))))(y ||= {}); + +---------- /out/ts-namespace-eval.js ---------- +var z; +((z) => (z.x = 1, console.log(1, eval("x"))))(z ||= {}); + ================================================================================ TestConstValueInliningNoBundle ---------- /out/top-level.js ---------- diff --git a/internal/js_ast/js_ast.go b/internal/js_ast/js_ast.go index 362f6367306..b5e13840bf3 100644 --- a/internal/js_ast/js_ast.go +++ b/internal/js_ast/js_ast.go @@ -486,6 +486,8 @@ var ESuperShared = &ESuper{} var ENullShared = &ENull{} var EUndefinedShared = &EUndefined{} var EThisShared = &EThis{} +var SEmptyShared = &SEmpty{} +var SDebuggerShared = &SDebugger{} type ENew struct { Target Expr diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index d892f548ec8..11e57bfe1ec 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -5825,7 +5825,7 @@ func (p *parser) parseStmt(opts parseStmtOpts) js_ast.Stmt { switch p.lexer.Token { case js_lexer.TSemicolon: p.lexer.Next() - return js_ast.Stmt{Loc: loc, Data: &js_ast.SEmpty{}} + return js_ast.Stmt{Loc: loc, Data: js_ast.SEmptyShared} case js_lexer.TExport: previousExportKeyword := p.esmExportKeyword @@ -6805,7 +6805,7 @@ func (p *parser) parseStmt(opts parseStmtOpts) js_ast.Stmt { case js_lexer.TDebugger: p.lexer.Next() p.lexer.ExpectOrInsertSemicolon() - return js_ast.Stmt{Loc: loc, Data: &js_ast.SDebugger{}} + return js_ast.Stmt{Loc: loc, Data: js_ast.SDebuggerShared} case js_lexer.TOpenBrace: p.pushScopeForParsePass(js_ast.ScopeBlock, loc) @@ -7606,6 +7606,41 @@ func isDirectiveSupported(s *js_ast.SDirective) bool { } func (p *parser) mangleStmts(stmts []js_ast.Stmt, kind stmtsKind) []js_ast.Stmt { + // Remove inlined constants now that we know whether any of these statements + // contained a direct eval() or not. This can't be done earlier when we + // encounter the constant because we haven't encountered the eval() yet. + // Inlined constants are not removed if they are in a top-level scope or + // if they are exported (which could be in a nested TypeScript namespace). + if p.currentScope.Parent != nil && !p.currentScope.ContainsDirectEval { + for i, stmt := range stmts { + switch s := stmt.Data.(type) { + case *js_ast.SEmpty, *js_ast.SComment, *js_ast.SDirective, *js_ast.SDebugger, *js_ast.STypeScript: + continue + + case *js_ast.SLocal: + if !s.IsExport { + end := 0 + for _, d := range s.Decls { + if id, ok := d.Binding.Data.(*js_ast.BIdentifier); ok { + if _, ok := p.constValues[id.Ref]; ok { + continue + } + } + s.Decls[end] = d + end++ + } + if end == 0 { + stmts[i].Data = js_ast.SEmptyShared + } else { + s.Decls = s.Decls[:end] + } + } + continue + } + break + } + } + // Merge adjacent statements during mangling result := make([]js_ast.Stmt, 0, len(stmts)) isControlFlowDead := false @@ -8402,7 +8437,7 @@ func (p *parser) visitSingleStmt(stmt js_ast.Stmt, kind stmtsKind) js_ast.Stmt { // One statement could potentially expand to several statements func stmtsToSingleStmt(loc logger.Loc, stmts []js_ast.Stmt) js_ast.Stmt { if len(stmts) == 0 { - return js_ast.Stmt{Loc: loc, Data: &js_ast.SEmpty{}} + return js_ast.Stmt{Loc: loc, Data: js_ast.SEmptyShared} } if len(stmts) == 1 { // "let" and "const" must be put in a block when in a single-statement context @@ -8547,7 +8582,7 @@ func dropFirstStatement(body js_ast.Stmt, replaceOrNil js_ast.Stmt) js_ast.Stmt if replaceOrNil.Data != nil { return replaceOrNil } - return js_ast.Stmt{Loc: body.Loc, Data: &js_ast.SEmpty{}} + return js_ast.Stmt{Loc: body.Loc, Data: js_ast.SEmptyShared} } func mangleFor(s *js_ast.SFor) { @@ -9252,9 +9287,8 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_ // Local statements do not end the const local prefix p.currentScope.IsAfterConstLocalPrefix = wasAfterAfterConstLocalPrefix - end := 0 for i := range s.Decls { - d := s.Decls[i] + d := &s.Decls[i] p.visitBinding(d.Binding, bindingOpts{}) // Visit the initializer @@ -9306,13 +9340,6 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_ p.constValues = make(map[js_ast.Ref]js_ast.ConstValue) } p.constValues[id.Ref] = value - - // Only keep this declaration if it's top-level or exported (which - // could be in a nested TypeScript namespace), otherwise erase it - if p.currentScope.Parent == nil || s.IsExport { - s.Decls[end] = d - end++ - } continue } } @@ -9325,20 +9352,8 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_ p.currentScope.IsAfterConstLocalPrefix = true } } - - // Keep the declaration if we didn't continue the loop above - s.Decls[end] = d - end++ - } - - // Remove this declaration entirely if all symbols are inlined constants - if end == 0 { - return stmts } - // Trim the removed declarations - s.Decls = s.Decls[:end] - // Handle being exported inside a namespace if s.IsExport && p.enclosingNamespaceArgRef != nil { wrapIdentifier := func(loc logger.Loc, ref js_ast.Ref) js_ast.Expr { @@ -9444,7 +9459,7 @@ func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_ stmt = s.Stmts[0] } else if len(s.Stmts) == 0 { // Trim empty blocks - stmt = js_ast.Stmt{Loc: stmt.Loc, Data: &js_ast.SEmpty{}} + stmt = js_ast.Stmt{Loc: stmt.Loc, Data: js_ast.SEmptyShared} } }