Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Closed
rictic opened this issue Apr 26, 2018 · 6 comments · Fixed by #834
Closed

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

rictic opened this issue Apr 26, 2018 · 6 comments · Fixed by #834
Labels
bug Confirmed bug

Comments

@rictic
Copy link

rictic commented Apr 26, 2018

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
Copy link
Member

vigneshshanmugam commented Apr 30, 2018

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
Copy link

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
Copy link
Member

@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.

@boopathi boopathi added the needs info The details mentioned are NOT clear or NOT enough. Please provide more information. label May 1, 2018
@simonbuchan
Copy link

@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
Copy link

Cyp commented May 5, 2018

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
Copy link

Cyp commented May 8, 2018

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);

@vigneshshanmugam vigneshshanmugam added bug Confirmed bug and removed needs info The details mentioned are NOT clear or NOT enough. Please provide more information. labels May 8, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Confirmed bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants