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

Add mark eval scopes helper and deopt DCE fn unused params #371

Merged
merged 6 commits into from
Jan 19, 2017
Merged

Conversation

boopathi
Copy link
Member

@boopathi boopathi commented Jan 10, 2017

@boopathi
Copy link
Member Author

The helper attaches to the scope object directly instead of returning the list of scopes that contain eval. This is to avoid traversing the program twice - mangler, dce.

@boopathi
Copy link
Member Author

Also, this now only deopts eval scopes for only a few transforms under the visitor Scope. Have to look into other places in DCE that require deopt for eval.

`);

const expected = unpad(`
function a(b, c, d) {
Copy link
Member

Choose a reason for hiding this comment

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

Will this work with nested functions too? i.e. making sure it doesn't mangle top-level vars either.

Copy link
Member Author

Choose a reason for hiding this comment

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

added more tests

@@ -0,0 +1,52 @@
"use strict";

const EVAL_SCOPE_MARKER = "evalInScope";
Copy link
Member

Choose a reason for hiding this comment

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

Should we use Symbol instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. Will change. I was using scope.evalInScope directly.

Copy link
Member

@kangax kangax left a comment

Choose a reason for hiding this comment

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

I feel like this is something that should be done on Babel level? So that you can just ask scope.hasDirectEval or something along those lines.

/cc @hzoo

@boopathi
Copy link
Member Author

I feel like this is something that should be done on Babel level

I think so too, but if we keep it in babel, it should be CORRECT rather than optimised for a few use cases. Like now, we don't recompute between multiple calls of hasEval()/isUnsafeScope() - as we assume that we don't create new scopes. If we do, we should manually recompute these things by calling markEvalScopes() again.

If we implement this in babel, every call of hasEval/isUnsafeScope should trigger scope.traverse to find if it contains eval. And there are other cases like - eval is removed.

@kangax
Copy link
Member

kangax commented Jan 13, 2017

Yeah, this brings us again to the usual issues of keeping track of proper state and revalidation. Just like we don't update bindings or certain path information right now after changing nodes.

@@ -710,6 +715,10 @@ module.exports = ({ types: t, traverse }) => {
traverse.clearCache();
path.scope.crawl();

if (!isEvalScopesMarked(path.scope)) {
markEvalScopes(path);
Copy link
Member

Choose a reason for hiding this comment

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

I'd love to merge this for now.

Just one suggestion — let's move isEvalScopesMarked into markEvalScopes (checking it within and returning if scope is already marked). That way API becomes simpler (markEvalScopes(path)), there's less vars to exports, etc.

Another thing to consider is that markEvalScopes operates on path, and hasEval operates on scope. In an ideal world, that means these methods belong to those objects — path.markScopesWithEval, scope.hasDirectEvals.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't put isMarked inside markEvalScopes because if something is changed after marking - we need to remark.

Copy link
Member Author

Choose a reason for hiding this comment

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

that means these methods belong to those objects — path.markScopesWithEval, scope.hasDirectEvals

Yes!!

Copy link
Member

Choose a reason for hiding this comment

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

Do you want to implement them in Babel instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @hzoo what do you think ?

Copy link
Member

@kangax kangax left a comment

Choose a reason for hiding this comment

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

Please see inline

@boopathi boopathi added the Tag: Bug Fix Pull Request fixes a bug label Jan 19, 2017
@kangax
Copy link
Member

kangax commented Jan 19, 2017

Merging for now. Later, we can implement this in Babel instead.

@kangax kangax merged commit 2b98b98 into master Jan 19, 2017
@kangax kangax deleted the dce-eval-0 branch January 19, 2017 21:02
@kangax
Copy link
Member

kangax commented Jan 19, 2017

@boopathi thanks for a speedy fix!

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
2 participants