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

Inline hoisted, post-return declarations properly #391

Merged
merged 7 commits into from Jan 28, 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 @@ -2460,4 +2460,66 @@ describe("dce-plugin", () => {
`);
expect(transform(source)).toBe(expected);
});

it("should not remove vars after return statement", () => {

const source = unpad(`
function f() {
return x;
var x = 1;
}
`);

const expected = unpad(`
function f() {
return void 0;
}
`);

expect(transform(source)).toBe(expected);
});

it("should not remove vars after return statement #2", () => {

const source = unpad(`
var x = 0;
function f1(){
function f2(){
return x;
};
return f2();
var x = 1;
}
`);

const expected = unpad(`
var x = 0;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not ideal, of course. But we could revisit later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually correct. This is not removed because it's a global.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #392

function f1() {
return function () {
return x;
}();
var x = 1;
}
`);

expect(transform(source)).toBe(expected);
});

it("should not remove vars after return statement #3", () => {

const source = unpad(`
function foo() {
bar = x;
var x = 1;
}
`);

const expected = unpad(`
function foo() {
bar = void 0;
}
`);

expect(transform(source)).toBe(expected);
});
});
60 changes: 52 additions & 8 deletions packages/babel-plugin-minify-dead-code-elimination/src/index.js
Expand Up @@ -3,6 +3,24 @@
const some = require("lodash.some");
const { markEvalScopes, isMarked: isEvalScopesMarked, hasEval } = require("babel-helper-mark-eval-scopes");

function prevSiblings(path) {
const parentPath = path.parentPath;
const siblings = [];

let key = parentPath.key;

while ((path = parentPath.getSibling(--key)).type) {
siblings.push(path);
}
return siblings;
}

function forEachAncestor(path, callback) {
while (path = path.parentPath) {
callback(path);
}
}

module.exports = ({ types: t, traverse }) => {
const removeOrVoid = require("babel-helper-remove-or-void")(t);
const shouldRevisit = Symbol("shouldRevisit");
Expand Down Expand Up @@ -238,31 +256,57 @@ module.exports = ({ types: t, traverse }) => {
}

if (binding.references === 1 && binding.kind !== "param" && binding.kind !== "module" && binding.constant) {

let replacement = binding.path.node;
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)) {
replacement = replacement.init;

const _prevSiblings = prevSiblings(replacementPath);

// traverse ancestors of a reference checking if it's before declaration
forEachAncestor(refPath, (ancestor) => {
if (_prevSiblings.indexOf(ancestor) > -1) {
isReferencedBefore = true;
}
});

// deopt if reference is in different scope than binding
// since we don't know if it's sync or async execition
// (i.e. whether value has been assigned to a reference or not)
if (isReferencedBefore && refPath.scope !== binding.scope) {
continue;
}

// simulate hoisting by replacing value
// with undefined if declaration is after reference
replacement = isReferencedBefore
? t.unaryExpression("void", t.numericLiteral(0), true)
: replacement.init;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few lines below this, replacementPath = replacementPath.get("init") - we might have to fix this here too. Else replacement and replacementPath might point to different things.

// Bail out for ArrayPattern and ObjectPattern
// TODO: maybe a more intelligent approach instead of simply bailing out
if (!replacementPath.get("id").isIdentifier()) {
continue;
}
replacementPath = replacementPath.get("init");
}

if (!replacement) {
continue;
}

if (!scope.isPure(replacement, true)) {
if (!scope.isPure(replacement, true) && !isReferencedBefore) {
continue;
}

if (binding.referencePaths.length > 1) {
throw new Error("Expected only one reference");
}

let bail = false;
const refPath = binding.referencePaths[0];

if (replacementPath.isIdentifier()) {
bail = refPath.scope.getBinding(replacement.name) !== scope.getBinding(replacement.name);
Expand Down Expand Up @@ -358,7 +402,7 @@ module.exports = ({ types: t, traverse }) => {
return;
}

// Not last in it's block? (See BlockStatement visitor)
// Not last in its block? (See BlockStatement visitor)
if (path.container.length - 1 !== path.key &&
!canExistAfterCompletion(path.getSibling(path.key + 1)) &&
path.parentPath.isBlockStatement()
Expand Down