Skip to content

Commit

Permalink
fix #1791: mark certain "Set" and "Map" as pure
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed Nov 24, 2021
1 parent 1084849 commit d0b0874
Show file tree
Hide file tree
Showing 3 changed files with 116 additions and 0 deletions.
20 changes: 20 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -146,6 +146,26 @@ In addition to the breaking changes above, the following changes are also includ
.example{--some-var: var(--tw-empty, )}/*! Some legal comment */body{background-color:red}
```

* Mark `Set` and `Map` with array arguments as pure ([#1791](https://github.com/evanw/esbuild/issues/1791))

This release introduces special behavior for references to the global `Set` and `Map` constructors that marks them as `/* @__PURE__ */` if they are known to not have any side effects. These constructors evaluate the iterator of whatever is passed to them and the iterator could have side effects, so this is only safe if whatever is passed to them is an array, since the array iterator has no side effects.

Marking a constructor call as `/* @__PURE__ */` means it's safe to remove if the result is unused. This is an existing feature that you can trigger by manually adding a `/* @__PURE__ */` comment before a constructor call. The difference is that this release contains special behavior to automatically mark `Set` and `Map` as pure for you as long as it's safe to do so. As with all constructor calls that are marked `/* @__PURE__ */`, any internal expressions which could cause side effects are still preserved even though the constructor call itself is removed:

```js
// Original code
new Map([
['a', b()],
[c(), new Set(['d', e()])],
]);

// Old output (with --minify)
new Map([["a",b()],[c(),new Set(["d",e()])]]);

// New output (with --minify)
b(),c(),e();
```

## 0.13.15

* Fix `super` in lowered `async` arrow functions ([#1777](https://github.com/evanw/esbuild/issues/1777))
Expand Down
68 changes: 68 additions & 0 deletions internal/js_parser/js_parser.go
View file
Expand Up @@ -12718,6 +12718,8 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
e.Args[i] = p.visitExpr(arg)
}

p.maybeMarkKnownGlobalConstructorAsPure(e)

case *js_ast.EArrow:
asyncArrowNeedsToBeLowered := e.IsAsync && p.options.unsupportedJSFeatures.Has(compat.AsyncAwait)
oldFnOrArrowData := p.fnOrArrowDataVisit
Expand Down Expand Up @@ -12890,6 +12892,72 @@ func (p *parser) warnAboutImportNamespaceCall(target js_ast.Expr, kind importNam
}
}

func (p *parser) maybeMarkKnownGlobalConstructorAsPure(e *js_ast.ENew) {
if id, ok := e.Target.Data.(*js_ast.EIdentifier); ok {
if symbol := p.symbols[id.Ref.InnerIndex]; symbol.Kind == js_ast.SymbolUnbound {
switch symbol.OriginalName {
case "Set":
n := len(e.Args)

if n == 0 {
// "new Set()" is pure
e.CanBeUnwrappedIfUnused = true
break
}

if n == 1 {
switch e.Args[0].Data.(type) {
case *js_ast.EArray, *js_ast.ENull, *js_ast.EUndefined:
// "new Set([a, b, c])" is pure
// "new Set(null)" is pure
// "new Set(void 0)" is pure
e.CanBeUnwrappedIfUnused = true

default:
// "new Set(x)" is impure because the iterator for "x" could have side effects
}
}

case "Map":
n := len(e.Args)

if n == 0 {
// "new Map()" is pure
e.CanBeUnwrappedIfUnused = true
break
}

if n == 1 {
switch arg := e.Args[0].Data.(type) {
case *js_ast.ENull, *js_ast.EUndefined:
// "new Map(null)" is pure
// "new Map(void 0)" is pure
e.CanBeUnwrappedIfUnused = true

case *js_ast.EArray:
allEntriesAreArrays := true
for _, item := range arg.Items {
if _, ok := item.Data.(*js_ast.EArray); !ok {
// "new Map([x])" is impure because "x[0]" could have side effects
allEntriesAreArrays = false
break
}
}

// "new Map([[a, b], [c, d]])" is pure
if allEntriesAreArrays {
e.CanBeUnwrappedIfUnused = true
}

default:
// "new Map(x)" is impure because the iterator for "x" could have side effects
}
}
}
}
}
}

func (p *parser) valueForDefine(loc logger.Loc, defineFunc config.DefineFunc, opts identifierOpts) js_ast.Expr {
expr := js_ast.Expr{Loc: loc, Data: defineFunc(config.DefineArgs{
Loc: loc,
Expand Down
28 changes: 28 additions & 0 deletions internal/js_parser/js_parser_test.go
View file
Expand Up @@ -4839,3 +4839,31 @@ func TestMangleCatch(t *testing.T) {
expectPrintedMangle(t, "try { throw 1 } catch (x) { eval('x') }", "try {\n throw 1;\n} catch (x) {\n eval(\"x\");\n}\n")
expectPrintedMangle(t, "if (y) try { throw 1 } catch (x) {} else eval('x')", "if (y)\n try {\n throw 1;\n } catch {\n }\nelse\n eval(\"x\");\n")
}

func TestAutoPureForSet(t *testing.T) {
expectPrinted(t, "new Set", "/* @__PURE__ */ new Set();\n")
expectPrinted(t, "new Set(null)", "/* @__PURE__ */ new Set(null);\n")
expectPrinted(t, "new Set(undefined)", "/* @__PURE__ */ new Set(void 0);\n")
expectPrinted(t, "new Set([])", "/* @__PURE__ */ new Set([]);\n")
expectPrinted(t, "new Set([x])", "/* @__PURE__ */ new Set([x]);\n")

expectPrinted(t, "new Set(x)", "new Set(x);\n")
expectPrinted(t, "new Set(false)", "new Set(false);\n")
expectPrinted(t, "new Set({})", "new Set({});\n")
expectPrinted(t, "new Set({ x })", "new Set({ x });\n")
}

func TestAutoPureForMap(t *testing.T) {
expectPrinted(t, "new Map", "/* @__PURE__ */ new Map();\n")
expectPrinted(t, "new Map(null)", "/* @__PURE__ */ new Map(null);\n")
expectPrinted(t, "new Map(undefined)", "/* @__PURE__ */ new Map(void 0);\n")
expectPrinted(t, "new Map([])", "/* @__PURE__ */ new Map([]);\n")
expectPrinted(t, "new Map([[]])", "/* @__PURE__ */ new Map([[]]);\n")
expectPrinted(t, "new Map([[], []])", "/* @__PURE__ */ new Map([[], []]);\n")

expectPrinted(t, "new Map(x)", "new Map(x);\n")
expectPrinted(t, "new Map(false)", "new Map(false);\n")
expectPrinted(t, "new Map([x])", "new Map([x]);\n")
expectPrinted(t, "new Map([x, []])", "new Map([x, []]);\n")
expectPrinted(t, "new Map([[], x])", "new Map([[], x]);\n")
}

4 comments on commit d0b0874

@kzc
Copy link
Contributor

@kzc kzc commented on d0b0874 Nov 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the global Set and Map constructors ...
evaluate the iterator of whatever is passed to them and the iterator could have side effects so this is only safe if whatever is passed to them is an array

@evanw String, Map and Set also implement side effect free iterators.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/iterator#description

$ node -p 'new Set(new Set("ABC"))'
Set(3) { 'A', 'B', 'C' }

$ node -p 'new Map(new Map(Object.entries({A:1,B:2})))'
Map(2) { 'A' => 1, 'B' => 2 }

@eligrey
Copy link

@eligrey eligrey commented on d0b0874 Nov 25, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evanw Set, Map, and Array all have mutable iterators by default, so you cannot assume that the iterators are always pure. Please undo this change.

Is it possible to statically detect when built-in default iterators are mutated? This optimization could be gated behind such a detection

@kzc
Copy link
Contributor

@kzc kzc commented on d0b0874 Nov 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set, Map, and Array all have mutable iterators by default, so you cannot assume that the iterators are always pure

All JS bundlers and minifiers make basic assumptions about the code. If someone were to alter any core JS builtin in an incompatible way such as:

$ cat array-iter.js
Array.prototype[Symbol.iterator] = (() => ({
    next() {
        console.log("effect");
        return {
            done: true
        };
    }
}));
new Set([1, 2]);
console.log("end");

$ cat array-iter.js | node
effect
end

$ cat array-iter.js | esbuild --bundle | node
end

then they should disable optimizations altogether:

$ cat array-iter.js | esbuild --bundle --tree-shaking=false | node
effect
end

@evanw
Copy link
Owner Author

@evanw evanw commented on d0b0874 Nov 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evanw Set, Map, and Array all have mutable iterators by default, so you cannot assume that the iterators are always pure. Please undo this change.

In general esbuild assumes that built-ins behave like built-ins. For example, esbuild sometimes makes use of Object.defineProperty which means you must polyfill it yourself if it doesn't exist and the polyfill is assumed to behave according to the specification. I'm not going to change esbuild's output to try to get it to continue to work in a bizarre environment where Object.defineProperty does something random instead of what it's supposed to do. Redefining built-ins to do non-standard stuff is terrible development practice. IMO it's reasonable that your tools are going to break if you do that. The fix is to just not do that.

What use case do you have in mind? What library are you using that redefines built-in iterators to have side effects? What are the side effects, and why are they useful?

Is it possible to statically detect when built-in default iterators are mutated? This optimization could be gated behind such a detection

No, it's not possible. New code can easily be injected into the VM in lots of different ways including via eval, new Function, <script> tags, etc. JavaScript is a dynamic language, not a static language.

Please sign in to comment.