Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Incorrect transformation on React project with both simplify and dead code elimination enabled #385

Closed
zsparal opened this issue Jan 22, 2017 · 4 comments
Labels
bug Confirmed bug

Comments

@zsparal
Copy link

zsparal commented Jan 22, 2017

I couldn't make a minimal repro, but compiling my project with these two plugins enabled leads to the following error message (it's worth mentioning that babili is run on single fairly large file created by Webpack):

Uncaught TypeError: inst.render is not a function        ReactCompositeComponent.js:811

The issue doesn't manifest with either of the plugins disabled. I narrowed the bug to createPrevExpressionEater("if") in simplify whose result is removed by the if (evaluateTest === false) block in the IfStatement part of dead-code-elimination.

If there's any diagnostic I can run to be more helpful I'd be happy to do so.

@jakepusateri
Copy link

I'm also seeing something along these lines as well, while trying to minify https://github.com/facebook/react/blob/master/src/isomorphic/modern/class/ReactComponent.js in the context of a webpack module. It ends up causing the error:

Uncaught TypeError: Cannot call a class as a function

for me. I was able to get a more minimal repro of:

Input

function ReactComponent() {
}

ReactComponent.prototype.isReactComponent = {};

if (false) {
  const anyVar = 1;
}

Expected

function ReactComponent(){}ReactComponent.prototype.isReactComponent={};

Actual

function ReactComponent(){};

It appears to work correctly in the REPL at the moment, but not on babel-preset-babili@0.0.10 and a babel version of 6.22.2 (babel-core 6.22.1)

@boopathi boopathi added the bug Confirmed bug label Jan 22, 2017
@boopathi
Copy link
Member

Strange that if (0) { const foo = bar; } is causing the previous statement to be removed.

@zsparal
Copy link
Author

zsparal commented Jan 22, 2017

The problem seems to be that dead code elimination ignores the side effects of sequence expressions.
simplify transforms the code provided by @jakepusateri to this:

function ReactComponent() {
}

if (ReactComponent.prototype.isReactComponent = {}, false) {
  const anyVar = 1;
}

which dead code elimination undestands as:

if (false) {
  const anyVar = 1;
}

I'm not terribly familiar with the Babel AST but in light of dead code elimination this transformation only seems safe to me when the node being moved into the if is pure. In fact, replacing this block in createPrevExpressionEater in simplify:

var prev = path.getSibling(path.key - 1);
if (!prev.isExpressionStatement()) {
  return;
}

with

var prev = path.getSibling(path.key - 1);
if (!prev.isExpressionStatement() || !prev.isPure()) {
  return;
}

solves the minimal repro case for me (couldn't test it on my project yet).

@boopathi
Copy link
Member

boopathi commented Jan 22, 2017

Yes! Exactly.

The order of the transformations again. 0.0.9 didn't have DCE in exit. Now it's in Program.exit. So multipleSequenceExpressions takes place first and then DCE.

isPure check (which is necessary) should fix the bug. But it will result in ...; if (sideEffects, !1) {} ... code, instead of simply - ...; sideEffects; .... I'm trying out ways to eliminate this.

boopathi added a commit that referenced this issue Jan 22, 2017
+ (Fix #385)
+ Add isPure check to IfStatement visitor in DCE
+ Move IfStatement visitor from Single Pass program.exit to DCEPlugin.visitor - so that it executes for all IfStatements as babel traverse happens and NOT during programPath.traverse. This eliminates the need for a second pass for things like the one mentioned in #385.
boopathi added a commit that referenced this issue Jan 25, 2017
+ (Fix #385)
+ Add isPure check to IfStatement visitor in DCE
+ Move IfStatement visitor from Single Pass program.exit to DCEPlugin.visitor - so that it executes for all IfStatements as babel traverse happens and NOT during programPath.traverse. This eliminates the need for a second pass for things like the one mentioned in #385.
kangax pushed a commit that referenced this issue Jan 25, 2017
* DCE: Deopt impure statements in If.test

+ (Fix #385)
+ Add isPure check to IfStatement visitor in DCE
+ Move IfStatement visitor from Single Pass program.exit to DCEPlugin.visitor - so that it executes for all IfStatements as babel traverse happens and NOT during programPath.traverse. This eliminates the need for a second pass for things like the one mentioned in #385.

* Add tests

* Moar optimizations for side-effecty if statements

* Add dependency jsesc for constant-folding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug
Projects
None yet
Development

No branches or pull requests

3 participants