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

Check binding exists #261

Merged
merged 1 commit into from Jan 17, 2017
Merged

Check binding exists #261

merged 1 commit into from Jan 17, 2017

Conversation

kangax
Copy link
Member

@kangax kangax commented Nov 8, 2016

Ran into this in one of our files when NOT using es2015 transform. I'll try to make a test case but don't have one for now.

@boopathi
Copy link
Member

boopathi commented Nov 8, 2016

Related: babel/babel#4815 . So we should probably include a scope.crawl before running DCE.

I guess a for statement using let in the init and an arrow function (referencing this let binding) inside the for loop might fail. (this is true for es2015. misread the desc)

@boopathi boopathi added the Tag: Bug Fix Pull Request fixes a bug label Nov 9, 2016
@boopathi
Copy link
Member

boopathi commented Nov 14, 2016

I had similar issues in mangler as well, where a BindingIdentifier has no binding - and I reduced it to this ... without clearing the traverse cache and re-crawling the entire scope that will throw.

@kangax
Copy link
Member Author

kangax commented Nov 15, 2016

@boopathi so what do you think we should do here? Drop this PR in favor of #252?

@boopathi
Copy link
Member

No. Suggesting we should do traverse.clearCache and program.scope.crawl() before anything in DCE, and check if that fixes the bug.

This check really is a work around where we have a BindingIdentifier(here a function parameter) and ignore it when we do not have a binding in the scope.

@boopathi
Copy link
Member

I didn't come across any other case where a binding is missing at obvious places. So, a snippet that reproduces this problem should be helpful to debug if we missed anything.

@kangax
Copy link
Member Author

kangax commented Nov 25, 2016

I just ran into this again, while running over Nuclide code base. It seems to only happen when there's other plugins that transform code before Babili (https://github.com/facebook/nuclide/blob/master/pkg/nuclide-node-transpiler/lib/NodeTranspiler.js#L43-L75); transforming source code itself didn't exhibit the issue...

@kangax kangax merged commit 8169f8e into master Jan 17, 2017
@kangax kangax deleted the check_for_binding branch January 17, 2017 22:15
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.

None yet

2 participants