From f727407dfb06bc207303388b5605bace45be2569 Mon Sep 17 00:00:00 2001 From: Vignesh Shanmugam Date: Wed, 21 Jun 2017 18:23:09 +0200 Subject: [PATCH 1/4] [fix #574] deopt when binding is present in diff scope --- .../__tests__/dead-code-elimination-test.js | 10 ++++++ .../src/index.js | 31 ++++++++++++++----- 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/packages/babel-plugin-minify-dead-code-elimination/__tests__/dead-code-elimination-test.js b/packages/babel-plugin-minify-dead-code-elimination/__tests__/dead-code-elimination-test.js index abecf1de3..c4b5ab9e3 100644 --- a/packages/babel-plugin-minify-dead-code-elimination/__tests__/dead-code-elimination-test.js +++ b/packages/babel-plugin-minify-dead-code-elimination/__tests__/dead-code-elimination-test.js @@ -2462,4 +2462,14 @@ describe("dce-plugin", () => { } ` ); + + thePlugin( + "should deopt when binding is on different scope - issue #574", + ` + function foo(v) { + if (v) var w = 10; + if (w) console.log("hello", v); + } + ` + ); }); diff --git a/packages/babel-plugin-minify-dead-code-elimination/src/index.js b/packages/babel-plugin-minify-dead-code-elimination/src/index.js index 796c08787..ffbb88e0a 100644 --- a/packages/babel-plugin-minify-dead-code-elimination/src/index.js +++ b/packages/babel-plugin-minify-dead-code-elimination/src/index.js @@ -22,6 +22,10 @@ function forEachAncestor(path, callback) { } } +function isDifferentScope(varDeclarationPath, ifStatementPath) { + return varDeclarationPath.parentPath.isDescendant(ifStatementPath.parentPath); +} + module.exports = ({ types: t, traverse }) => { const removeOrVoid = require("babel-helper-remove-or-void")(t); const shouldRevisit = Symbol("shouldRevisit"); @@ -187,11 +191,13 @@ module.exports = ({ types: t, traverse }) => { ) { continue; } else if (binding.path.isVariableDeclarator()) { + const parentPath = binding.path.parentPath.parentPath; if ( - binding.path.parentPath.parentPath && - binding.path.parentPath.parentPath.isForXStatement() + parentPath && + (parentPath.isForXStatement() || parentPath.isIfStatement()) ) { // Can't remove if in a for-in/for-of/for-await statement `for (var x in wat)`. + // Can't remove if (true) {var a = 10}; continue; } } else if (!scope.isPure(binding.path.node)) { @@ -293,14 +299,10 @@ module.exports = ({ types: t, traverse }) => { let replacementPath = binding.path; let isReferencedBefore = false; - if (binding.referencePaths.length > 1) { - throw new Error("Expected only one reference"); - } const refPath = binding.referencePaths[0]; if (t.isVariableDeclarator(replacement)) { const _prevSiblings = prevSiblings(replacementPath); - // traverse ancestors of a reference checking if it's before declaration forEachAncestor(refPath, ancestor => { if (_prevSiblings.indexOf(ancestor) > -1) { @@ -309,7 +311,7 @@ module.exports = ({ types: t, traverse }) => { }); // deopt if reference is in different scope than binding - // since we don't know if it's sync or async execition + // since we don't know if it's sync or async execution // (i.e. whether value has been assigned to a reference or not) if (isReferencedBefore && refPath.scope !== binding.scope) { continue; @@ -774,8 +776,21 @@ module.exports = ({ types: t, traverse }) => { const evalResult = test.evaluate(); const isPure = test.isPure(); - const replacements = []; + const { path: bindingPath } = + path.scope.getBinding(test.node.name) || {}; + // Ref - https://github.com/babel/babili/issues/574 + // deopt if var is declared in other scope + // if (a) { var b = blahl;} if (b) { //something } + if ( + bindingPath && + bindingPath.parentPath.isVariableDeclaration() && + isDifferentScope(bindingPath.parentPath, path) + ) { + return; + } + + const replacements = []; if (evalResult.confident && !isPure && test.isSequenceExpression()) { replacements.push( t.expressionStatement(extractSequenceImpure(test)) From f1819f55729d09aa4819608818079aa12e7d1684 Mon Sep 17 00:00:00 2001 From: Vignesh Shanmugam Date: Thu, 22 Jun 2017 11:01:43 +0200 Subject: [PATCH 2/4] fix binding check --- .../src/index.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/packages/babel-plugin-minify-dead-code-elimination/src/index.js b/packages/babel-plugin-minify-dead-code-elimination/src/index.js index ffbb88e0a..75cdb286f 100644 --- a/packages/babel-plugin-minify-dead-code-elimination/src/index.js +++ b/packages/babel-plugin-minify-dead-code-elimination/src/index.js @@ -776,16 +776,15 @@ module.exports = ({ types: t, traverse }) => { const evalResult = test.evaluate(); const isPure = test.isPure(); - const { path: bindingPath } = - path.scope.getBinding(test.node.name) || {}; + const binding = path.scope.getBinding(test.node.name); // Ref - https://github.com/babel/babili/issues/574 // deopt if var is declared in other scope // if (a) { var b = blahl;} if (b) { //something } if ( - bindingPath && - bindingPath.parentPath.isVariableDeclaration() && - isDifferentScope(bindingPath.parentPath, path) + binding && + binding.path.parentPath.isVariableDeclaration() && + isDifferentScope(binding.path.parentPath, path) ) { return; } From 57fe8ffa8d0afc3583bb3a35925a19834639a4b8 Mon Sep 17 00:00:00 2001 From: Vignesh Shanmugam Date: Mon, 7 Aug 2017 14:14:48 +0200 Subject: [PATCH 3/4] small changes --- .../src/index.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/babel-plugin-minify-dead-code-elimination/src/index.js b/packages/babel-plugin-minify-dead-code-elimination/src/index.js index 75cdb286f..63000da46 100644 --- a/packages/babel-plugin-minify-dead-code-elimination/src/index.js +++ b/packages/babel-plugin-minify-dead-code-elimination/src/index.js @@ -22,10 +22,6 @@ function forEachAncestor(path, callback) { } } -function isDifferentScope(varDeclarationPath, ifStatementPath) { - return varDeclarationPath.parentPath.isDescendant(ifStatementPath.parentPath); -} - module.exports = ({ types: t, traverse }) => { const removeOrVoid = require("babel-helper-remove-or-void")(t); const shouldRevisit = Symbol("shouldRevisit"); @@ -191,13 +187,15 @@ module.exports = ({ types: t, traverse }) => { ) { continue; } else if (binding.path.isVariableDeclarator()) { - const parentPath = binding.path.parentPath.parentPath; + const declaration = binding.path.parentPath; + const maybeBlockParent = declaration.parentPath; if ( - parentPath && - (parentPath.isForXStatement() || parentPath.isIfStatement()) + maybeBlockParent && + maybeBlockParent.isForXStatement({ + left: declaration.node + }) ) { // Can't remove if in a for-in/for-of/for-await statement `for (var x in wat)`. - // Can't remove if (true) {var a = 10}; continue; } } else if (!scope.isPure(binding.path.node)) { @@ -784,7 +782,7 @@ module.exports = ({ types: t, traverse }) => { if ( binding && binding.path.parentPath.isVariableDeclaration() && - isDifferentScope(binding.path.parentPath, path) + binding.path.parentPath.parentPath.isDescendant(path.parentPath) ) { return; } From 483a69a936f54a9886888a580f231851021b877a Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Mon, 7 Aug 2017 15:32:09 +0200 Subject: [PATCH 4/4] Fix detection of different scope --- .../src/index.js | 25 ++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/packages/babel-plugin-minify-dead-code-elimination/src/index.js b/packages/babel-plugin-minify-dead-code-elimination/src/index.js index 63000da46..73c3abf77 100644 --- a/packages/babel-plugin-minify-dead-code-elimination/src/index.js +++ b/packages/babel-plugin-minify-dead-code-elimination/src/index.js @@ -781,10 +781,29 @@ module.exports = ({ types: t, traverse }) => { // if (a) { var b = blahl;} if (b) { //something } if ( binding && - binding.path.parentPath.isVariableDeclaration() && - binding.path.parentPath.parentPath.isDescendant(path.parentPath) + binding.path.parentPath.isVariableDeclaration({ kind: "var" }) ) { - return; + let ifStatementParent = null; + + const fnParent = + binding.path.getFunctionParent() || + binding.path.getProgramParent(); + + forEachAncestor(binding.path.parentPath, parent => { + if (fnParent === parent) return; + if (parent.isIfStatement()) { + ifStatementParent = parent; + } + }); + + if ( + ifStatementParent && + binding.referencePaths.some( + ref => !ref.isDescendant(ifStatementParent) + ) + ) { + return; + } } const replacements = [];