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
Changes from 2 commits
81cd100
3404d3d
5b735d1
9d49be4
eddd59f
80dd806
ffefb6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,15 @@ | |
const some = require("lodash.some"); | ||
const { markEvalScopes, isMarked: isEvalScopesMarked, hasEval } = require("babel-helper-mark-eval-scopes"); | ||
|
||
function forEachPrevSibling(path, callback) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Considering that DCE has a lot of cases of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filed babel/babel#5223 |
||
const parentPath = path.parentPath; | ||
let key = parentPath.key; | ||
|
||
while ((path = parentPath.getSibling(--key)).type) { | ||
callback(path); | ||
} | ||
} | ||
|
||
module.exports = ({ types: t, traverse }) => { | ||
const removeOrVoid = require("babel-helper-remove-or-void")(t); | ||
const shouldRevisit = Symbol("shouldRevisit"); | ||
|
@@ -238,10 +247,25 @@ 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 isAfterReturn = false; | ||
|
||
if (t.isVariableDeclarator(replacement)) { | ||
replacement = replacement.init; | ||
|
||
forEachPrevSibling(replacementPath, (path) => { | ||
if (t.isReturnStatement(path)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this will fail for cases like - function foo() {
bar = x;
var x = 1;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. But then we have a much "deeper" problem: function foo() {
(function() {
alert(function(){ return x }());
})();
var x = 1;
} This brings us again to CFG and knowing when an identifier is referenced before declaration (having to traverse entire tree before declaration). I think in this case it would be simpler to just hoist vars before doing DCE. So this: function foo() {
bar = x;
var x = 1;
} would first become: function foo() {
var x;
bar = x;
x = 1;
} which could then be transformed to: function foo() {
bar = undefined;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Case 1 can be solved without a CFG ... function foo() {
(() => {
return x // x - ReferencedPath
})() // CallExpression SiblingPath
var x = 1;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, for each binding reference we can traverse a subtree upwards and check if parent is a previous sibling. We also need to check that identifier is not already defined: function foo() {
((x) => {
return x // x - ReferencedPath but is a local var
})(2) // CallExpression SiblingPath
var x = 1;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! But, this case we are dealing with is "one-time-used-constant" - binding.references === 1 and binding.constant, and we already have the path to the reference ( |
||
isAfterReturn = true; | ||
} | ||
}); | ||
|
||
// simulate hoisting by replacing value | ||
// with undefined if declaration is after return | ||
replacement = isAfterReturn | ||
? t.identifier("undefined") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, thanks |
||
: replacement.init; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a few lines below this, |
||
// Bail out for ArrayPattern and ObjectPattern | ||
// TODO: maybe a more intelligent approach instead of simply bailing out | ||
if (!replacementPath.get("id").isIdentifier()) { | ||
|
@@ -253,7 +277,7 @@ module.exports = ({ types: t, traverse }) => { | |
continue; | ||
} | ||
|
||
if (!scope.isPure(replacement, true)) { | ||
if (!scope.isPure(replacement, true) && !isAfterReturn) { | ||
continue; | ||
} | ||
|
||
|
@@ -358,7 +382,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() | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #392