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

if_return - deopt when ref loses scope #408

Merged
merged 3 commits into from Feb 27, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -1625,12 +1625,14 @@ describe("simplify-plugin", () => {
}
`);

// TODO:
// Fix indenting
const expected = unpad(`
function x() {
if (bar) {
var x = foo;
if (foo && y) throw y;
}
var x = foo;
if (foo && y) throw y;
}
}
`);

Expand Down Expand Up @@ -2566,15 +2568,17 @@ describe("simplify-plugin", () => {
}
}
`);
// TODO:
// Fix indenting
const expected = unpad(`
function foo() {
function bar() {
baz(), bar();
}

if (bar(), !x) {
const { a } = b;
}
const { a } = b;
}
}
`);
expect(transform(source)).toBe(expected);
Expand Down
57 changes: 51 additions & 6 deletions packages/babel-plugin-minify-simplify/src/index.js
Expand Up @@ -547,7 +547,16 @@ module.exports = ({ types: t }) => {
const ids = Object.keys(prev.getBindingIdentifiers());

idloop: for (let i = 0; i < ids.length; i++) {
const refs = prev.scope.bindings[ids[i]].referencePaths;
const binding = prev.scope.bindings[ids[i]];
// TODO
// Temporary Fix
// if there is no binding, we assume it is referenced outside
// and deopt to avoid bugs
if (!binding) {
referencedOutsideLoop = true;
break idloop;
}
const refs = binding.referencePaths;
for (let j = 0; j < refs.length; j++) {
if (!isAncestor(path, refs[j])) {
referencedOutsideLoop = true;
Expand Down Expand Up @@ -1471,8 +1480,44 @@ module.exports = ({ types: t }) => {
function genericEarlyExitTransform(path) {
const { node } = path;

const statements = path.container.slice(path.key + 1)
.filter((stmt) => !t.isFunctionDeclaration(stmt));
const statements = path
.parentPath
.get(path.listKey)
.slice(path.key + 1)
.filter((stmt) => !stmt.isFunctionDeclaration());

// deopt for any block scoped bindings
// issue#399
const deopt = !statements.every((stmt) => {
if (!(
stmt.isVariableDeclaration({ kind: "let" })
|| stmt.isVariableDeclaration({ kind: "const" })
)) {
return true;
}
const ids = Object.keys(stmt.getBindingIdentifiers());
for (const id of ids) {
const binding = path.scope.getBinding(id);
// TODO
// Temporary Fix
// if there is no binding, we assume it is referenced outside
// and deopt to avoid bugs
if (!binding) {
return false;
}
const refs = [...binding.referencePaths, ...binding.constantViolations];
for (const ref of refs) {
if (!ref.isIdentifier()) return false;
if (ref.getFunctionParent().scope !== path.scope) return false;
}
}
return true;
});

if (deopt) {
path.visit();
return false;
}

if (!statements.length) {
path.replaceWith(t.expressionStatement(node.test));
Expand All @@ -1490,15 +1535,15 @@ module.exports = ({ types: t }) => {
node.test = t.unaryExpression("!", node.test, true);
}

path.get("consequent").replaceWith(t.blockStatement(statements.map((stmt) => t.clone(stmt.node))));

let l = statements.length;
while (l-- > 0) {
if (!t.isFunctionDeclaration(statements[l])) {
if (!statements[l].isFunctionDeclaration()) {
path.getSibling(path.key + 1).remove();
}
}

node.consequent = t.blockStatement(statements);

// this should take care of removing the block
path.visit();
}
Expand Down