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.

Sign up for GitHub

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

Jump to bottom

Fix the reference resolving logic for global. #61

Merged
merged 3 commits into from
May 8, 2015
Merged

Fix the reference resolving logic for global. #61

merged 3 commits into from
May 8, 2015

Conversation

mysticatea
Copy link
Contributor

References of let/const/class should be resolved
statically on the global scope also.

Related: #49 #56

@mysticatea
Fix the reference resolving logic for global.
43d32d5 
- References of `let`/`const`/`class` declarations should be resolved
statically on the global scope also.
- Add very simple test cases.
michaelficarra
michaelficarra reviewed May 7, 2015
View reviewed changes
src/scope.js
// others should be resolved dynamically.
if (this.__shouldStaticallyCloseForGlobal(ref)) {
this.__staticCloseRef(ref);
}
Copy link
Member

Choose a reason for hiding this comment

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

This formatting is odd.

@michaelficarra
Copy link
Member

Very nice! 👍

@mysticatea
Copy link
Contributor Author

Thank you!

I got that variable.defs becomes multiple if there are two or more declarations of same identifier.
I fixed due to it, resolving references statically only when all defs are let/const/class.

I found an issue.
References seem to not be created on default parameters of functions and default values of destructuring assignment...

@Constellation
Copy link
Member

I found an issue.
References seem to not be created on default parameters of functions and default values of destructuring assignment...

Yes. This is because, at that time, AST tree for default parameter was ambiguous (acorn style v.s. esprima style).
But now, it's defined in estree, so we need to implement it :)

@mysticatea
Copy link
Contributor Author

OK, I will remove tests for those from this PR, and try to implement those on other PR later.

@mysticatea
Remove several tests.
3e3b7c4 
- default parameters of functions and default values of destructuring
assignment are not supported yet.  I'll add those later again.
@mysticatea mysticatea changed the title [WIP] Fix the reference resolving logic for global. Fix the reference resolving logic for global. May 8, 2015
@mysticatea
Copy link
Contributor Author

Maybe done.

Note:
When using escope of this PR version, one of eslint's tests is failed.

no-unused-vars const foo = "hello!";function bar(foobar = foo) {  foobar.replace(/!$/, " world!");}

I guess the cause is:

  • The reference of init of const foo had been unresolved.
  • But the reference was changed to a resolved reference by this PR.
  • Since default parameter doesn't make references, now, there are not unresolved references of const foo.
  • As a result, workaround of eslint is not working...

michaelficarra added a commit that referenced this pull request May 8, 2015
@michaelficarra
Merge pull request #61 from mysticatea/fix-references-of-top-level-le…
715850b 
…t-const-class

Fix the reference resolving logic for global.
@michaelficarra michaelficarra merged commit 715850b into estools:master May 8, 2015
@mysticatea mysticatea deleted the fix-references-of-top-level-let-const-class branch May 9, 2015 11:20
@mysticatea mysticatea mentioned this pull request May 10, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mysticatea @michaelficarra @Constellation