From 0e01f21049e75fb1c19c87f468fdcc96efb621b7 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sun, 13 Mar 2022 17:59:15 -0700 Subject: [PATCH] Drop superfluous `__name()` calls (#2062) --- CHANGELOG.md | 42 ++++++++ internal/bundler/bundler_default_test.go | 65 +++++++++++++ .../bundler/snapshots/snapshots_default.txt | 83 +++++++++++++++- internal/bundler/snapshots/snapshots_ts.txt | 1 - internal/js_ast/js_ast.go | 7 +- internal/js_parser/js_parser.go | 24 ++--- internal/js_printer/js_printer.go | 96 ++++++++++++++----- 7 files changed, 269 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8fa5e906b5b..08550ccc7eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,48 @@ Some JavaScript environments such as Cloudflare Workers or Deno Deploy don't allow `new Function` because they disallow dynamic JavaScript evaluation. Previously esbuild's WebAssembly-based library used this to construct the WebAssembly worker function. With this release, the code is now inlined without using `new Function` so it will be able to run even when this restriction is in place. +* Drop superfluous `__name()` calls ([#2062](https://github.com/evanw/esbuild/pull/2062)) + + When the `--keep-names` option is specified, esbuild inserts calls to a `__name` helper function to ensure that the `.name` property on function and class objects remains consistent even if the function or class name is renamed to avoid a name collision or because name minification is enabled. With this release, esbuild will now try to omit these calls to the `__name` helper function when the name of the function or class object was not renamed during the linking process after all: + + ```js + // Original code + import { foo as foo1 } from 'data:text/javascript,export function foo() { return "foo1" }' + import { foo as foo2 } from 'data:text/javascript,export function foo() { return "foo2" }' + console.log(foo1.name, foo2.name) + + // Old output (with --bundle --keep-names) + (() => { + var __defProp = Object.defineProperty; + var __name = (target, value) => __defProp(target, "name", { value, configurable: true }); + function foo() { + return "foo1"; + } + __name(foo, "foo"); + function foo2() { + return "foo2"; + } + __name(foo2, "foo"); + console.log(foo.name, foo2.name); + })(); + + // New output (with --bundle --keep-names) + (() => { + var __defProp = Object.defineProperty; + var __name = (target, value) => __defProp(target, "name", { value, configurable: true }); + function foo() { + return "foo1"; + } + function foo2() { + return "foo2"; + } + __name(foo2, "foo"); + console.log(foo.name, foo2.name); + })(); + ``` + + Notice how one of the calls to `__name` is now no longer printed. This change was contributed by [@indutny](https://github.com/indutny). + ## 0.14.25 * Reduce minification of CSS transforms to avoid Safari bugs ([#2057](https://github.com/evanw/esbuild/issues/2057)) diff --git a/internal/bundler/bundler_default_test.go b/internal/bundler/bundler_default_test.go index a0246560a02..c264f78074d 100644 --- a/internal/bundler/bundler_default_test.go +++ b/internal/bundler/bundler_default_test.go @@ -4358,6 +4358,71 @@ func TestDefineThis(t *testing.T) { }) } +func TestKeepNamesWithNecessaryHelperFunctionCalls(t *testing.T) { + default_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.js": ` + import { + functionStmt as functionStmt1, + functionExpr as functionExpr1, + classStmt as classStmt1, + classExpr as classExpr1, + functionAnonExpr as functionAnonExpr1, + classAnonExpr as classAnonExpr1, + } from './copy1' + + import { + functionStmt as functionStmt2, + functionExpr as functionExpr2, + classStmt as classStmt2, + classExpr as classExpr2, + functionAnonExpr as functionAnonExpr2, + classAnonExpr as classAnonExpr2, + } from './copy2' + + console.log([ + functionStmt1, functionStmt2, + functionExpr1, functionExpr2, + classStmt1, classStmt2, + classExpr1, classExpr2, + functionAnonExpr1, functionAnonExpr2, + classAnonExpr1, classAnonExpr2, + ]) + `, + + "/copy1.js": ` + export function functionStmt() { return 'copy1' } + export class classStmt { foo = 'copy1' } + export let functionExpr = function fn() { return 'copy1' } + export let classExpr = class cls { foo = 'copy1' } + export let functionAnonExpr = function() { return 'copy1' } + export let classAnonExpr = class { foo = 'copy1' } + + class classStmtSideEffect { static [copy1]() {} } + let classExprSideEffect = class clsSideEffect { static [copy1]() {} } + `, + + "/copy2.js": ` + export function functionStmt() { return 'copy2' } + export class classStmt { foo = 'copy2' } + export let functionExpr = function fn() { return 'copy2' } + export let classExpr = class cls { foo = 'copy2' } + export let functionAnonExpr = function() { return 'copy2' } + export let classAnonExpr = class { foo = 'copy2' } + + class classStmtSideEffect { static [copy2]() {} } + let classExprSideEffect = class clsSideEffect { static [copy2]() {} } + `, + }, + entryPaths: []string{"/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputFile: "/out.js", + KeepNames: true, + }, + }) +} + 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 5ed6ae7bbb9..42da6186108 100644 --- a/internal/bundler/snapshots/snapshots_default.txt +++ b/internal/bundler/snapshots/snapshots_default.txt @@ -1401,19 +1401,95 @@ TestKeepNamesTreeShaking // entry.js function fnStmtKeep() { } -__name(fnStmtKeep, "fnStmtKeep"); x = fnStmtKeep; var fnExprKeep = /* @__PURE__ */ __name(function() { }, "keep"); x = fnExprKeep; var clsStmtKeep = class { }; -__name(clsStmtKeep, "clsStmtKeep"); new clsStmtKeep(); var clsExprKeep = /* @__PURE__ */ __name(class { }, "keep"); new clsExprKeep(); +================================================================================ +TestKeepNamesWithNecessaryHelperFunctionCalls +---------- /out.js ---------- +// copy1.js +function functionStmt() { + return "copy1"; +} +var classStmt = class { + foo = "copy1"; +}; +var functionExpr = function fn() { + return "copy1"; +}; +var classExpr = class cls { + foo = "copy1"; +}; +var functionAnonExpr = function() { + return "copy1"; +}; +var classAnonExpr = class { + foo = "copy1"; +}; +var classStmtSideEffect = class { + static [copy1]() { + } +}; +var classExprSideEffect = class clsSideEffect { + static [copy1]() { + } +}; + +// copy2.js +function functionStmt2() { + return "copy2"; +} +__name(functionStmt2, "functionStmt"); +var classStmt2 = class { + foo = "copy2"; +}; +__name(classStmt2, "classStmt"); +var functionExpr2 = /* @__PURE__ */ __name(function fn2() { + return "copy2"; +}, "fn"); +var classExpr2 = /* @__PURE__ */ __name(class cls2 { + foo = "copy2"; +}, "cls"); +var functionAnonExpr2 = /* @__PURE__ */ __name(function() { + return "copy2"; +}, "functionAnonExpr"); +var classAnonExpr2 = /* @__PURE__ */ __name(class { + foo = "copy2"; +}, "classAnonExpr"); +var classStmtSideEffect2 = class { + static [copy2]() { + } +}; +__name(classStmtSideEffect2, "classStmtSideEffect"); +var classExprSideEffect2 = /* @__PURE__ */ __name(class clsSideEffect2 { + static [copy2]() { + } +}, "clsSideEffect"); + +// entry.js +console.log([ + functionStmt, + functionStmt2, + functionExpr, + functionExpr2, + classStmt, + classStmt2, + classExpr, + classExpr2, + functionAnonExpr, + functionAnonExpr2, + classAnonExpr, + classAnonExpr2 +]); + ================================================================================ TestLegalCommentsAvoidSlashTagEndOfFile ---------- /out/entry.js ---------- @@ -2810,12 +2886,11 @@ export function outer() { let inner = function() { return Math.random(); }; - __name(inner, "inner"); const x = inner(); console.log(x); } } -__name(outer, "outer"), outer(); +outer(); ================================================================================ TestSwitchScopeNoBundle diff --git a/internal/bundler/snapshots/snapshots_ts.txt b/internal/bundler/snapshots/snapshots_ts.txt index 2d75c842191..d8cd84fd9d4 100644 --- a/internal/bundler/snapshots/snapshots_ts.txt +++ b/internal/bundler/snapshots/snapshots_ts.txt @@ -1102,7 +1102,6 @@ TestTypeScriptDecoratorsKeepNames // entry.ts var Foo = class { }; -__name(Foo, "Foo"); Foo = __decorateClass([ decoratorMustComeAfterName ], Foo); diff --git a/internal/js_ast/js_ast.go b/internal/js_ast/js_ast.go index 9c3eed3855e..2e4366b0d18 100644 --- a/internal/js_ast/js_ast.go +++ b/internal/js_ast/js_ast.go @@ -516,6 +516,8 @@ type ECall struct { // call itself is removed due to this annotation, the arguments must remain // if they have side effects. CanBeUnwrappedIfUnused bool + + IsKeepName bool } func (a *ECall) HasSameFlagsAs(b *ECall) bool { @@ -848,11 +850,6 @@ type SLazyExport struct { type SExpr struct { Value Expr - - // This is set to true for automatically-generated expressions that should - // not affect tree shaking. For example, calling a function from the runtime - // that doesn't have externally-visible side effects. - DoesNotAffectTreeShaking bool } type EnumValue struct { diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 8201a340ddc..af992cc6dde 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -7512,7 +7512,6 @@ func (p *parser) mangleStmts(stmts []js_ast.Stmt, kind stmtsKind) []js_ast.Stmt prevStmt := result[len(result)-1] if prevS, ok := prevStmt.Data.(*js_ast.SExpr); ok && !js_ast.IsSuperCall(prevStmt) && !js_ast.IsSuperCall(stmt) { prevS.Value = js_ast.JoinWithComma(prevS.Value, s.Value) - prevS.DoesNotAffectTreeShaking = prevS.DoesNotAffectTreeShaking && s.DoesNotAffectTreeShaking continue } } @@ -8758,21 +8757,20 @@ func (p *parser) keepExprSymbolName(value js_ast.Expr, name string) js_ast.Expr // Make sure tree shaking removes this if the function is never used value.Data.(*js_ast.ECall).CanBeUnwrappedIfUnused = true + value.Data.(*js_ast.ECall).IsKeepName = true return value } func (p *parser) keepStmtSymbolName(loc logger.Loc, ref js_ast.Ref, name string) js_ast.Stmt { p.symbols[ref.InnerIndex].Flags |= js_ast.DidKeepName - return js_ast.Stmt{Loc: loc, Data: &js_ast.SExpr{ - Value: p.callRuntime(loc, "__name", []js_ast.Expr{ - {Loc: loc, Data: &js_ast.EIdentifier{Ref: ref}}, - {Loc: loc, Data: &js_ast.EString{Value: helpers.StringToUTF16(name)}}, - }), + call := p.callRuntime(loc, "__name", []js_ast.Expr{ + {Loc: loc, Data: &js_ast.EIdentifier{Ref: ref}}, + {Loc: loc, Data: &js_ast.EString{Value: helpers.StringToUTF16(name)}}, + }) + call.Data.(*js_ast.ECall).IsKeepName = true - // Make sure tree shaking removes this if the function is never used - DoesNotAffectTreeShaking: true, - }} + return js_ast.Stmt{Loc: loc, Data: &js_ast.SExpr{Value: call}} } func (p *parser) visitAndAppendStmt(stmts []js_ast.Stmt, stmt js_ast.Stmt) []js_ast.Stmt { @@ -14285,12 +14283,6 @@ func (p *parser) stmtsCanBeRemovedIfUnused(stmts []js_ast.Stmt) bool { } case *js_ast.SExpr: - if s.DoesNotAffectTreeShaking { - // Expressions marked with this are automatically generated and have - // no side effects by construction. - break - } - if !p.exprCanBeRemovedIfUnused(s.Value) { return false } @@ -14464,7 +14456,7 @@ func (p *parser) exprCanBeRemovedIfUnused(expr js_ast.Expr) bool { return true case *js_ast.ECall: - canCallBeRemoved := e.CanBeUnwrappedIfUnused + canCallBeRemoved := e.CanBeUnwrappedIfUnused || e.IsKeepName // Consider calls to our runtime "__publicField" function to be free of // side effects for the purpose of expression removal. This allows class diff --git a/internal/js_printer/js_printer.go b/internal/js_printer/js_printer.go index 004f4515a3d..0f9fee0a5d7 100644 --- a/internal/js_printer/js_printer.go +++ b/internal/js_printer/js_printer.go @@ -441,6 +441,8 @@ type printer struct { callTarget js_ast.E extractedLegalComments map[string]bool js []byte + keepNamesAssignTarget js_ast.E + keepNamesRef js_ast.Ref options Options builder sourcemap.ChunkBuilder stmtStart int @@ -1398,6 +1400,10 @@ func (p *printer) simplifyUnusedExpr(expr js_ast.Expr) js_ast.Expr { } case *js_ast.ECall: + if p.isCallExprSuperfluous(e) { + return js_ast.Expr{Loc: expr.Loc} + } + var symbolFlags js_ast.SymbolFlags switch target := e.Target.Data.(type) { case *js_ast.EIdentifier: @@ -1798,6 +1804,11 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla } } + if p.isCallExprSuperfluous(e) { + p.printExpr(e.Args[0], level, flags) + return + } + wrap := level >= js_ast.LNew || (flags&forbidCall) != 0 var targetFlags printExprFlags if e.OptionalChain == js_ast.OptionalChainNone { @@ -2563,6 +2574,44 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla } } +func (p *printer) isCallExprSuperfluous(value *js_ast.ECall) bool { + if !value.IsKeepName { + return false + } + + var ref *js_ast.Ref + switch e := value.Args[0].Data.(type) { + case *js_ast.EIdentifier: + // "__name(foo, 'foo')" + ref = &e.Ref + + case *js_ast.EFunction: + // "__name(function foo() {}, 'foo')" + if e.Fn.Name != nil { + ref = &e.Fn.Name.Ref + } + + case *js_ast.EClass: + // "__name(class foo {}, 'foo')" + if e.Class.Name != nil { + ref = &e.Class.Name.Ref + } + } + + // If this is an anonymous function or class expression but the assign target + // has a name binding, use the name from the name binding instead: + // + // let foo = __name(function() {}, "foo"); + // let bar = __name(class {}, "bar"); + // + if ref == nil && p.keepNamesAssignTarget == value { + ref = &p.keepNamesRef + } + + keptName := value.Args[1].Data.(*js_ast.EString).Value + return ref != nil && helpers.UTF16EqualsString(keptName, p.renamer.NameForSymbol(*ref)) +} + func (p *printer) isUnboundEvalIdentifier(value js_ast.Expr) bool { if id, ok := value.Data.(*js_ast.EIdentifier); ok { // Using the original name here is ok since unbound symbols are not renamed @@ -2777,6 +2826,11 @@ func (p *printer) printDecls(keyword string, decls []js_ast.Decl, flags printExp p.printBinding(decl.Binding) if decl.ValueOrNil.Data != nil { + if id, ok := decl.Binding.Data.(*js_ast.BIdentifier); ok { + p.keepNamesAssignTarget = decl.ValueOrNil.Data + p.keepNamesRef = id.Ref + } + p.printSpace() p.print("=") p.printSpace() @@ -3373,18 +3427,16 @@ func (p *printer) printStmt(stmt js_ast.Stmt, flags printStmtFlags) { update := s.UpdateOrNil // Omit calls to empty functions from the output completely - if p.options.MinifySyntax { - if expr, ok := init.Data.(*js_ast.SExpr); ok { - if value := p.simplifyUnusedExpr(expr.Value); value.Data == nil { - init.Data = nil - } else if value.Data != expr.Value.Data { - init.Data = &js_ast.SExpr{Value: value} - } - } - if update.Data != nil { - update = p.simplifyUnusedExpr(update) + if expr, ok := init.Data.(*js_ast.SExpr); ok { + if value := p.simplifyUnusedExpr(expr.Value); value.Data == nil { + init.Data = nil + } else if value.Data != expr.Value.Data { + init.Data = &js_ast.SExpr{Value: value} } } + if update.Data != nil { + update = p.simplifyUnusedExpr(update) + } p.printIndent() p.printSpaceBeforeIdentifier() @@ -3597,20 +3649,18 @@ func (p *printer) printStmt(stmt js_ast.Stmt, flags printStmtFlags) { value := s.Value // Omit calls to empty functions from the output completely - if p.options.MinifySyntax { - value = p.simplifyUnusedExpr(value) - if value.Data == nil { - // If this statement is not in a block, then we still need to emit something - if (flags & canOmitStatement) == 0 { - // "if (x) empty();" => "if (x) ;" - p.printIndent() - p.print(";") - p.printNewline() - } else { - // "if (x) { empty(); }" => "if (x) {}" - } - break + value = p.simplifyUnusedExpr(value) + if value.Data == nil { + // If this statement is not in a block, then we still need to emit something + if (flags & canOmitStatement) == 0 { + // "if (x) empty();" => "if (x) ;" + p.printIndent() + p.print(";") + p.printNewline() + } else { + // "if (x) { empty(); }" => "if (x) {}" } + break } p.printIndent()