babel / minify

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

simplify's loop rewriting incorrectly removes preceding variable declarations #824

Closed rictic closed 6 years ago

rictic commented 6 years ago

Input Code

let foo;
while (0) {}
console.log(foo);

Actual Output

'use strict';console.log(foo);

Details

Minimal repro in the sandbox here.

Passing simplify: false to babel-preset-minify resolves the issue.

It looks like what's happening is that the loop is rewritten to a for loop, then preceding statements are sometimes moved into the initialization segment of the for loop. But if the initialization segment is empty, or if the entire for loop is removed, then the declarations are lost.

Note that this does not require the loop to be completely deleted, and that more than one preceding statement can be lost. Our original code that prompted this looks roughly like:

Sandbox

const foo = 'foo';
const bar = 'bar';
let baz = qux();
while(baz) {
  baz = qux();
}
console.log(foo + bar);

And we're seeing the declarations for both foo and bar deleted.

vigneshshanmugam commented 6 years ago

I cannot reproduce this issue with babel-preset-minify@0.4.0 and babel-preset-env,

I can see that polymer tools is using a custom plugins list for compiling to es5, Is there any reason you cannot switch to preset env?

Just asking, so that we can drop support for babel-preset-es2015 with new releases in future which has issues with scoping.

justinfagnani commented 6 years ago

re, preset-env: One reason is that there's a bug in the class transform, so we had to pin to an earlier version. Another is that preset-env doesn't appear to let us control which plugins are included very well, and only covers ES2015, so the utility is low for us. We require out toolchain allow for no-compile builds for two current browsers, so we control which plugins are allow so that we don't support JS features that can't be run without compilation. IOW, preset-env is focused on which browsers to compile to, and we want to enforce which browsers we won't compile for, and therefore exclude plugins they might require.

vigneshshanmugam commented 6 years ago

@justinfagnani Thanks for the detailed explanation. That answers my question.

Can you give details on the babel core, babel-preset-2015 and minify version used? I tried different combinations and could not reproduce it somehow. May be i am missing something here. The REPL is using an older version.

simonbuchan commented 6 years ago

@justinfagnani A couple of notes: preset env by default is actually latest, not es2015, and you can control specific plugins to some extent with include and exclude, and if targets includes the browsers you do support, it won't try to support other browsers. For example, what you describe sounds kindof like:

["env", {
  exclude: ['transform-async-to-generator'],
  targets: {
    chrome: 49,
    firefox: 52,
    ios: 10,
  },
}]
Cyp commented 6 years ago

I seem to be able to reproduce this with:

const babel = require('babel-core');
console.log(babel.transform(`
let foo;
while (0) {}
console.log(foo);
`, {presets: ['minify', 'env']}).code);
// Prints: "use strict";console.log(foo);

with package.json:

{
  "name": "babelbug", "version": "1.0.0", "description": "", "main": "index.js", "scripts": {"test": "echo \"Error: no test specified\" && exit 1"}, "author": "", "license": "", "private": true,
  "dependencies": {
    "babel-core": "^6.26.3",
    "babel-preset-env": "^1.6.1",
    "babel-preset-minify": "^0.5.0-alpha.34c3efe9"
  }
}

I don't know whether any other information is relevant/useful.

Same with babel-preset-minify@0.3.0, babel-preset-minify@0.4.0 and babel-preset-minify@0.4.1. I couldn't easily find any configuration which didn't reproduce the issue.

Cyp commented 6 years ago

Testing more, all minify passes disabled gives (correctly):

"use strict";var foo=void 0;while(0){}console.log(foo);

Applying simplify gives (correctly):

"use strict";for(var foo=void 0;0;);console.log(foo);

Applying deadcode to that then gives (incorrectly):

"use strict";console.log(foo);