From 179433f34571b1e9c0c2c612af47abd6513e8659 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Sun, 6 Dec 2020 00:07:57 -0800 Subject: [PATCH] fix #580: improve DCE after unconditional jumps --- CHANGELOG.md | 11 ++ internal/bundler/bundler_dce_test.go | 137 +++++++++++++++++++ internal/bundler/snapshots/snapshots_dce.txt | 93 +++++++++++++ internal/js_parser/js_parser.go | 53 ++++++- internal/js_parser/js_parser_lower.go | 10 ++ 5 files changed, 302 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8dee1a0166b..722919d76a3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,17 @@ In that specific case, the transformed code could crash when run because the class name is not yet initialized when the static field initializer is run. Only JavaScript code was affected. TypeScript code was not affected. This release fixes this bug. +* Propagate control flow liveness to subsequent statements ([#580](https://github.com/evanw/esbuild/issues/580)) + + This change improves dead-code elimination in the case where unused statements follow an unconditional jump, such as a `return`: + + ```js + if (true) return + if (something) thisIsDeadCode() + ``` + + These unused statements are removed in more cases than in the previous release. Some statements may still be kept that contain hoisted symbols (`var` and `function` statements) because they could potentially impact the code before the conditional jump. + ## 0.8.19 * Handle non-ambiguous multi-path re-exports ([#568](https://github.com/evanw/esbuild/pull/568)) diff --git a/internal/bundler/bundler_dce_test.go b/internal/bundler/bundler_dce_test.go index cf805b26e8d..db6b53efbf8 100644 --- a/internal/bundler/bundler_dce_test.go +++ b/internal/bundler/bundler_dce_test.go @@ -1021,3 +1021,140 @@ func TestDisableTreeShaking(t *testing.T) { }, }) } + +func TestDeadCodeFollowingJump(t *testing.T) { + dce_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.js": ` + function testReturn() { + if (true) return y + z() + if (FAIL) return FAIL + if (x) { var y } + function z() {} + return FAIL + } + + function testThrow() { + if (true) throw y + z() + if (FAIL) return FAIL + if (x) { var y } + function z() {} + return FAIL + } + + function testBreak() { + while (true) { + if (true) { + y + z() + break + } + if (FAIL) return FAIL + if (x) { var y } + function z() {} + return FAIL + } + } + + function testContinue() { + while (true) { + if (true) { + y + z() + continue + } + if (FAIL) return FAIL + if (x) { var y } + function z() {} + return FAIL + } + } + + function testStmts() { + return [a, b, c, d, e, f, g, h, i] + + while (x) { var a } + while (FAIL) { let FAIL } + + do { var b } while (x) + do { let FAIL } while (FAIL) + + for (var c; ;) ; + for (let FAIL; ;) ; + + for (var d in x) ; + for (let FAIL in FAIL) ; + + for (var e of x) ; + for (let FAIL of FAIL) ; + + if (x) { var f } + if (FAIL) { let FAIL } + + if (x) ; else { var g } + if (FAIL) ; else { let FAIL } + + { var h } + { let FAIL } + + x: { var i } + x: { let FAIL } + } + + testReturn() + testThrow() + testBreak() + testContinue() + testStmts() + `, + }, + entryPaths: []string{"/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputFile: "/out.js", + MangleSyntax: true, + }, + }) +} + +func TestRemoveTrailingReturn(t *testing.T) { + dce_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.js": ` + function foo() { + if (a) b() + return + } + function bar() { + if (a) b() + return KEEP_ME + } + export default [ + foo, + bar, + function () { + if (a) b() + return + }, + function () { + if (a) b() + return KEEP_ME + }, + () => { + if (a) b() + return + }, + () => { + if (a) b() + return KEEP_ME + }, + ] + `, + }, + entryPaths: []string{"/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputFile: "/out.js", + MangleSyntax: true, + OutputFormat: config.FormatESModule, + }, + }) +} diff --git a/internal/bundler/snapshots/snapshots_dce.txt b/internal/bundler/snapshots/snapshots_dce.txt index f7b442b06db..2464d043b58 100644 --- a/internal/bundler/snapshots/snapshots_dce.txt +++ b/internal/bundler/snapshots/snapshots_dce.txt @@ -9,6 +9,71 @@ TestDataURLLoaderRemoveUnused // entry.js console.log("unused import"); +================================================================================ +TestDeadCodeFollowingJump +---------- /out.js ---------- +// entry.js +function testReturn() { + return y + z(); + if (x) + var y; + function z() { + } +} +function testThrow() { + throw y + z(); + if (x) + var y; + function z() { + } +} +function testBreak() { + for (; ; ) { + y + z(); + break; + if (x) + var y; + function z() { + } + } +} +function testContinue() { + for (; ; ) { + y + z(); + continue; + if (x) + var y; + function z() { + } + } +} +function testStmts() { + return [a, b, c, d, e, f, g, h, i]; + for (; x; ) + var a; + do + var b; + while (x); + for (var c; ; ) + ; + for (var d in x) + ; + for (var e of x) + ; + if (x) + var f; + if (!x) + var g; + var h; + x: + var i; +} +testReturn(); +testThrow(); +testBreak(); +testContinue(); +testStmts(); + ================================================================================ TestDisableTreeShaking ---------- /out.js ---------- @@ -295,6 +360,34 @@ console.log("hello"); // Users/user/project/src/entry.js console.log("unused import"); +================================================================================ +TestRemoveTrailingReturn +---------- /out.js ---------- +// entry.js +function foo() { + a && b(); +} +function bar() { + return a && b(), KEEP_ME; +} +var entry_default = [ + foo, + bar, + function() { + a && b(); + }, + function() { + return a && b(), KEEP_ME; + }, + () => { + a && b(); + }, + () => (a && b(), KEEP_ME) +]; +export { + entry_default as default +}; + ================================================================================ TestRemoveUnusedImportMeta ---------- /out.js ---------- diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 21b1bfbb16d..be1ba6596da 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -6022,6 +6022,27 @@ func shouldKeepStmtInDeadControlFlow(stmt js_ast.Stmt) bool { s.Decls = identifiers return true + case *js_ast.SIf: + return shouldKeepStmtInDeadControlFlow(s.Yes) || (s.No != nil && shouldKeepStmtInDeadControlFlow(*s.No)) + + case *js_ast.SWhile: + return shouldKeepStmtInDeadControlFlow(s.Body) + + case *js_ast.SDoWhile: + return shouldKeepStmtInDeadControlFlow(s.Body) + + case *js_ast.SFor: + return (s.Init != nil && shouldKeepStmtInDeadControlFlow(*s.Init)) || shouldKeepStmtInDeadControlFlow(s.Body) + + case *js_ast.SForIn: + return shouldKeepStmtInDeadControlFlow(s.Init) || shouldKeepStmtInDeadControlFlow(s.Body) + + case *js_ast.SForOf: + return shouldKeepStmtInDeadControlFlow(s.Init) || shouldKeepStmtInDeadControlFlow(s.Body) + + case *js_ast.SLabel: + return shouldKeepStmtInDeadControlFlow(s.Stmt) + default: // Everything else must be kept return true @@ -6084,21 +6105,46 @@ func (p *parser) visitStmtsAndPrependTempRefs(stmts []js_ast.Stmt, opts prependT } func (p *parser) visitStmts(stmts []js_ast.Stmt) []js_ast.Stmt { + // Save the current control-flow liveness. This represents if we are + // currently inside an "if (false) { ... }" block. + oldIsControlFlowDead := p.isControlFlowDead + // Visit all statements first visited := make([]js_ast.Stmt, 0, len(stmts)) var after []js_ast.Stmt for _, stmt := range stmts { + var target *[]js_ast.Stmt if _, ok := stmt.Data.(*js_ast.SExportEquals); ok { // TypeScript "export = value;" becomes "module.exports = value;". This // must happen at the end after everything is parsed because TypeScript // moves this statement to the end when it generates code. - after = p.visitAndAppendStmt(after, stmt) + target = &after } else { - visited = p.visitAndAppendStmt(visited, stmt) + target = &visited + } + + // This call should only append new statements but never modify old ones + nextStmtIndex := len(*target) + *target = p.visitAndAppendStmt(*target, stmt) + + // If there is a jump-like statement in the middle of this statement + // sequence, control flow is dead for all of the following statements. + // This improves dead-code elimination. + if p.options.mangleSyntax { + for _, stmt := range (*target)[nextStmtIndex:] { + if isJumpStatement(stmt.Data) { + p.isControlFlowDead = true + break + } + } } } visited = append(visited, after...) + // Restore the current control-flow liveness if it was changed inside the + // loop above. This is important because the caller will not restore it. + p.isControlFlowDead = oldIsControlFlowDead + // Stop now if we're not mangling if !p.options.mangleSyntax { return visited @@ -6213,6 +6259,9 @@ func (p *parser) visitStmts(stmts []js_ast.Stmt) []js_ast.Stmt { isControlFlowDead = true + case *js_ast.SBreak, *js_ast.SContinue: + isControlFlowDead = true + case *js_ast.SFor: if len(result) > 0 { prevStmt := result[len(result)-1] diff --git a/internal/js_parser/js_parser_lower.go b/internal/js_parser/js_parser_lower.go index cd8dd77da83..9dd86091095 100644 --- a/internal/js_parser/js_parser_lower.go +++ b/internal/js_parser/js_parser_lower.go @@ -154,6 +154,16 @@ func (p *parser) lowerFunction( hasRestArg *bool, isArrow bool, ) { + // Size optimization: trim an implicit return from the end of the function body + if p.options.mangleSyntax { + if n := len(*bodyStmts); n > 0 { + last := (*bodyStmts)[n-1] + if s, ok := last.Data.(*js_ast.SReturn); ok && s.Value == nil { + *bodyStmts = (*bodyStmts)[:n-1] + } + } + } + // Lower object rest binding patterns in function arguments if p.options.unsupportedJSFeatures.Has(compat.ObjectRestSpread) { var prefixStmts []js_ast.Stmt