babel / minify

:scissors: An ES6+ aware minifier based on the Babel toolchain (beta)
https://babeljs.io/repl
MIT License
4.4k stars 225 forks source link

Sometimes scopes are unsafely removed, breaking code #935

Open AshleyScirra opened 5 years ago

AshleyScirra commented 5 years ago

Describe the bug

babel-minify sometimes eliminates a scope (i.e. pairs of braces, e.g. { ... }) when it is unsafe to do so, resulting in broken code.

To Reproduce

{
    const foo = 1;
}

Actual Output

const a=1;

Note the enclosing scope was removed. This defeats the purpose of using the scope to prevent namespace pollution in the outer scope.

Expected Output

{
    const a=1;
}

Note the enclosing scope is preserved. This avoids the const declaration using its name in the outer scope.

Another way to demonstrate how this is an incorrect optimisation is the following code:

{
    const foo = 1;
}

{
    const bar = 2;
}

This fails to minify, because babel-minify renames both const declarations to a, removes both enclosing scopes, and then crashes with Duplicate declaration "a".

Configuration

Demo URL:

https://babeljs.io/en/repl#?babili=true&browsers=&build=&builtIns=false&spec=false&loose=false&code_lz=N4KABBYMYPYHYGcAuYBmMZgLxgIwG4QBfEEUSaeZMAIwEMAnbMAJkKKA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=false&presets=babili&prettier=false&targets=&version=6.26.0&envVersion=

Possible solution

It looks like the only safe option is to entirely avoid variable declarations in scopes. This is very difficult if you are using classic mode scripts and use a file-level scope to avoid polluting the global namespace.

AshleyScirra commented 5 years ago

With a bit more investigation, it looks like adding an extra statement to the scope prevents the bug happening. We came across this with real-world code like this:

{
    const NAMESPACE = self.foo = class Foo {
        constructor() { /* something using NAMESPACE */ }
    }
}

This uses a scope with a single const declaration inside (triggering the bug), but also declares a global variable with a class declaration. The scope gets removed though, so when this pattern is used more than once, the NAMESPACE declaration is duplicated in the outer scope.