Skip to content

Commit

Permalink
fix #2445: fixes for spread arguments and --minify
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed Aug 8, 2022
1 parent b2b5a60 commit 20878ce
Show file tree
Hide file tree
Showing 8 changed files with 274 additions and 14 deletions.
24 changes: 24 additions & 0 deletions CHANGELOG.md
View file
@@ -1,5 +1,29 @@
# Changelog

## Unreleased

* Fix optimizations for calls containing spread arguments ([#2445](https://github.com/evanw/esbuild/issues/2445))

This release fixes the handling of spread arguments in the optimization of `/* @__PURE__ */` comments, empty functions, and identity functions:

```js
// Original code
function empty() {}
function identity(x) { return x }
/* @__PURE__ */ a(...x)
/* @__PURE__ */ new b(...x)
empty(...x)
identity(...x)

// Old output (with --minify --tree-shaking=true)
...x;...x;...x;...x;

// New output (with --minify --tree-shaking=true)
function identity(n){return n}[...x];[...x];[...x];identity(...x);
```

Previously esbuild assumed arguments with side effects could be directly inlined. This is almost always true except for spread arguments, which are not syntactically valid on their own and which have the side effect of causing iteration, which might have further side effects. Now esbuild will wrap these elements in an unused array so that they are syntactically valid and so that the iteration side effects are preserved.

## 0.14.53

This release fixes a minor issue with the previous release: I had to rename the package `esbuild-linux-loong64` to `@esbuild/linux-loong64` in the contributed PR because someone registered the package name before I could claim it, and I missed a spot. Hopefully everything is working after this release. I plan to change all platform-specific package names to use the `@esbuild/` scope at some point to avoid this problem in the future.
Expand Down
142 changes: 142 additions & 0 deletions internal/bundler/bundler_dce_test.go
View file
Expand Up @@ -3271,3 +3271,145 @@ func TestMultipleDeclarationTreeShakingMinifySyntax(t *testing.T) {
},
})
}

// Pure call removal should still run iterators, which can have side effects
func TestPureCallsWithSpread(t *testing.T) {
dce_suite.expectBundled(t, bundled{
files: map[string]string{
"/entry.js": `
/* @__PURE__ */ foo(...args);
/* @__PURE__ */ new foo(...args);
`,
},
entryPaths: []string{"/entry.js"},
options: config.Options{
Mode: config.ModeBundle,
AbsOutputFile: "/out.js",
MinifySyntax: true,
},
})
}

func TestTopLevelFunctionInliningWithSpread(t *testing.T) {
dce_suite.expectBundled(t, bundled{
files: map[string]string{
"/entry.js": `
function empty1() {}
function empty2() {}
function empty3() {}
function identity1(x) { return x }
function identity2(x) { return x }
function identity3(x) { return x }
empty1()
empty2(args)
empty3(...args)
identity1()
identity2(args)
identity3(...args)
`,

"/inner.js": `
export function empty1() {}
export function empty2() {}
export function empty3() {}
export function identity1(x) { return x }
export function identity2(x) { return x }
export function identity3(x) { return x }
`,

"/entry-outer.js": `
import {
empty1,
empty2,
empty3,
identity1,
identity2,
identity3,
} from './inner.js'
empty1()
empty2(args)
empty3(...args)
identity1()
identity2(args)
identity3(...args)
`,
},
entryPaths: []string{"/entry.js", "/entry-outer.js"},
options: config.Options{
Mode: config.ModeBundle,
AbsOutputDir: "/out",
MinifySyntax: true,
},
})
}

func TestNestedFunctionInliningWithSpread(t *testing.T) {
dce_suite.expectBundled(t, bundled{
files: map[string]string{
"/entry.js": `
function empty1() {}
function empty2() {}
function empty3() {}
function identity1(x) { return x }
function identity2(x) { return x }
function identity3(x) { return x }
check(
empty1(),
empty2(args),
empty3(...args),
identity1(),
identity2(args),
identity3(...args),
)
`,

"/inner.js": `
export function empty1() {}
export function empty2() {}
export function empty3() {}
export function identity1(x) { return x }
export function identity2(x) { return x }
export function identity3(x) { return x }
`,

"/entry-outer.js": `
import {
empty1,
empty2,
empty3,
identity1,
identity2,
identity3,
} from './inner.js'
check(
empty1(),
empty2(args),
empty3(...args),
identity1(),
identity2(args),
identity3(...args),
)
`,
},
entryPaths: []string{"/entry.js", "/entry-outer.js"},
options: config.Options{
Mode: config.ModeBundle,
AbsOutputDir: "/out",
MinifySyntax: true,
},
})
}
4 changes: 2 additions & 2 deletions internal/bundler/linker.go
View file
Expand Up @@ -1566,8 +1566,8 @@ func (c *linkerContext) scanImportsAndExports() {
// Every call will be inlined
continue
} else if (flags & (js_ast.IsIdentityFunction | js_ast.CouldPotentiallyBeMutated)) == js_ast.IsIdentityFunction {
// Every single-argument call will be inlined
callUse.CallCountEstimate -= callUse.SingleArgCallCountEstimate
// Every single-argument call will be inlined as long as it's not a spread
callUse.CallCountEstimate -= callUse.SingleArgNonSpreadCallCountEstimate
if callUse.CallCountEstimate == 0 {
continue
}
Expand Down
77 changes: 77 additions & 0 deletions internal/bundler/snapshots/snapshots_dce.txt
View file
Expand Up @@ -1043,6 +1043,44 @@ function x() {
return 3;
}

================================================================================
TestNestedFunctionInliningWithSpread
---------- /out/entry.js ----------
// entry.js
function identity1(x) {
return x;
}
function identity3(x) {
return x;
}
check(
void 0,
(args, void 0),
([...args], void 0),
identity1(),
args,
identity3(...args)
);

---------- /out/entry-outer.js ----------
// inner.js
function identity1(x) {
return x;
}
function identity3(x) {
return x;
}

// entry-outer.js
check(
void 0,
(args, void 0),
([...args], void 0),
identity1(),
args,
identity3(...args)
);

================================================================================
TestPackageJsonSideEffectsArrayGlob
---------- /out.js ----------
Expand Down Expand Up @@ -1455,6 +1493,13 @@ console.log("hello");
// Users/user/project/src/entry.js
console.log("unused import");

================================================================================
TestPureCallsWithSpread
---------- /out.js ----------
// entry.js
[...args];
[...args];

================================================================================
TestRemoveTrailingReturn
---------- /out.js ----------
Expand Down Expand Up @@ -1545,6 +1590,38 @@ TestTextLoaderRemoveUnused
// entry.js
console.log("unused import");

================================================================================
TestTopLevelFunctionInliningWithSpread
---------- /out/entry.js ----------
// entry.js
function identity1(x) {
return x;
}
function identity3(x) {
return x;
}
args;
[...args];
identity1();
args;
identity3(...args);

---------- /out/entry-outer.js ----------
// inner.js
function identity1(x) {
return x;
}
function identity3(x) {
return x;
}

// entry-outer.js
args;
[...args];
identity1();
args;
identity3(...args);

================================================================================
TestTreeShakingBinaryOperators
---------- /out.js ----------
Expand Down
4 changes: 2 additions & 2 deletions internal/js_ast/js_ast.go
View file
Expand Up @@ -2100,8 +2100,8 @@ type SymbolUse struct {
}

type SymbolCallUse struct {
CallCountEstimate uint32
SingleArgCallCountEstimate uint32
CallCountEstimate uint32
SingleArgNonSpreadCallCountEstimate uint32
}

// Returns the canonical ref that represents the ref for the provided symbol.
Expand Down
6 changes: 6 additions & 0 deletions internal/js_ast/js_ast_helpers.go
View file
Expand Up @@ -676,6 +676,9 @@ func SimplifyUnusedExpr(expr Expr, unsupportedFeatures compat.JSFeature, isUnbou
if e.CanBeUnwrappedIfUnused {
var result Expr
for _, arg := range e.Args {
if _, ok := arg.Data.(*ESpread); ok {
arg.Data = &EArray{Items: []Expr{arg}, IsSingleLine: true}
}
result = JoinWithComma(result, SimplifyUnusedExpr(arg, unsupportedFeatures, isUnbound))
}
return result
Expand Down Expand Up @@ -732,6 +735,9 @@ func SimplifyUnusedExpr(expr Expr, unsupportedFeatures compat.JSFeature, isUnbou
if e.CanBeUnwrappedIfUnused {
var result Expr
for _, arg := range e.Args {
if _, ok := arg.Data.(*ESpread); ok {
arg.Data = &EArray{Items: []Expr{arg}, IsSingleLine: true}
}
result = JoinWithComma(result, SimplifyUnusedExpr(arg, unsupportedFeatures, isUnbound))
}
return result
Expand Down
10 changes: 5 additions & 5 deletions internal/js_parser/js_parser.go
View file
Expand Up @@ -14048,7 +14048,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
case *js_ast.EImportIdentifier:
// If this function is inlined, allow it to be tree-shaken
if p.options.minifySyntax && !p.isControlFlowDead {
p.convertSymbolUseToCall(t.Ref, len(e.Args) == 1)
p.convertSymbolUseToCall(t.Ref, len(e.Args) == 1 && !hasSpread)
}

case *js_ast.EIdentifier:
Expand Down Expand Up @@ -14095,7 +14095,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO

// If this function is inlined, allow it to be tree-shaken
if p.options.minifySyntax && !p.isControlFlowDead {
p.convertSymbolUseToCall(t.Ref, len(e.Args) == 1)
p.convertSymbolUseToCall(t.Ref, len(e.Args) == 1 && !hasSpread)
}

case *js_ast.EDot:
Expand Down Expand Up @@ -14385,7 +14385,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
return expr, exprOut{}
}

func (p *parser) convertSymbolUseToCall(ref js_ast.Ref, isSingleArgCall bool) {
func (p *parser) convertSymbolUseToCall(ref js_ast.Ref, isSingleNonSpreadArgCall bool) {
// Remove the normal symbol use
use := p.symbolUses[ref]
use.CountEstimate--
Expand All @@ -14401,8 +14401,8 @@ func (p *parser) convertSymbolUseToCall(ref js_ast.Ref, isSingleArgCall bool) {
}
callUse := p.symbolCallUses[ref]
callUse.CallCountEstimate++
if isSingleArgCall {
callUse.SingleArgCallCountEstimate++
if isSingleNonSpreadArgCall {
callUse.SingleArgNonSpreadCallCountEstimate++
}
p.symbolCallUses[ref] = callUse
}
Expand Down

0 comments on commit 20878ce

Please sign in to comment.