Skip to content

Helper evaluate path#645

Merged
boopathi merged 8 commits intomasterfrom
helper-evaluate-path-0
Aug 8, 2017
Merged

Helper evaluate path#645
boopathi merged 8 commits intomasterfrom
helper-evaluate-path-0

Conversation

@boopathi
Copy link
Copy Markdown
Member

@boopathi boopathi commented Aug 7, 2017

No description provided.

@boopathi boopathi added the Tag: Bug Fix Pull Request fixes a bug label Aug 7, 2017
@boopathi boopathi changed the title Helper evaluate path [WIP] Helper evaluate path Aug 7, 2017
@j-f1
Copy link
Copy Markdown
Contributor

j-f1 commented Aug 7, 2017

Can you update the README to explain what this does now?

@vigneshshanmugam
Copy link
Copy Markdown
Member

vigneshshanmugam commented Aug 7, 2017

path.evaluate in babel doesn't handle some of the cases like this

if (v) { var w = 10}
if (w) { console.log(v) } // in diff scope - but still returns { confident: true }, doesn't deopt and results in DCE which is wrong

The helper takes care of this scenario and also for references used before/after bindings and handles if its within same/diff scope.

We need to enable this on simplify and DCE instead of babel evaluate.

@boopathi boopathi changed the title [WIP] Helper evaluate path Helper evaluate path Aug 8, 2017
// https://github.com/babel/babel/blob/master/packages/babel-traverse/src/path/evaluation.js
// modified for Babili use
function evaluateIdentifier(path) {
if (!path.isReferencedIdentifier()) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

check can be removed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This will help in catching bugs where the refId path is replaced with something else. This has been an issue in DCE+mangle previously.

scopePath.skip();
},
ReferencedIdentifier(idPath) {
const binding = idPath.scope.getBinding(idPath.node.name);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we can remove this as well. evaluate identifier takes care of this already.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is required as well. For babel's evaluate to take care of globals. Here we ignore globals. In evaluatePath we depot it. If a referencedId makes it to evaluateId, we have to deopt instead of simply ignoring it. Though that code looks like it's never reached, it will be useful for detecting side-effects from other transformations.

? fnParent.get("body")
: fnParent.get("body").get("body");

const state = {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lets extract this usage before init in to separate function.

`,
`
function foo() {
bar(void 0);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should be referenceError right not void 0

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yes. I'll skip this test for now. This doesn't go through evaluate.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ahh that babel bug? can you put the link to the bug as well?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The transformation is not a babel-bug. It's a bug in one-use-replacements in DCE. The parsing is a babel-bug.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cool

@vigneshshanmugam
Copy link
Copy Markdown
Member

we need to use this evaluate in simplify as well in patterns matching case, this bug still exists #574

@boopathi
Copy link
Copy Markdown
Member Author

boopathi commented Aug 8, 2017

Simplify requires a lot of changes in updating scope. Postponing it for later.

@vigneshshanmugam
Copy link
Copy Markdown
Member

vigneshshanmugam commented Aug 8, 2017

We are doing the same checks/ optimisations already here https://github.com/babel/babili/blob/master/packages/babel-plugin-minify-dead-code-elimination/src/index.js#L314

Some of the cases we discussed like

function () {
  var a = b;
  console.log(b);
  b = 10;
}

are handled to an extent.

@boopathi boopathi merged commit 62982b5 into master Aug 8, 2017
@boopathi boopathi deleted the helper-evaluate-path-0 branch August 8, 2017 22:10
@boopathi
Copy link
Copy Markdown
Member Author

boopathi commented Aug 8, 2017

Yeah. That code doesn't go through evaluate. It's a one time used variable replacement that doesn't have any side-effects. evaluate takes care of finding values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Tag: Bug Fix Pull Request fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants