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

Fix scope of function declaration binding in constant folding #643

Closed
wants to merge 1 commit into from

Conversation

leops
Copy link
Contributor

@leops leops commented Mar 4, 2021

Summary

This is a possibly incomplete fix for #291, it corrects at least one issue with name shadowing but there might be other problems present in the very large code payload presented in the original issue. The exact bug fixed here happens when a function declares an unused variable shadowing its own name:

export function foo() {
    let foo;
}

The current code checks whether that the binding corresponding to the function is referenced in the scope of the FunctionDeclaration node, however in Babel that scope is situated inside the body of the function and not in the outer program. Since the binding is retrieved by name, Babel returns informations related to the inner, unreferenced variable and thus the entire function is removed. This fix changes the biding retrieval to read from the parent scope of the FunctionDefinition, which returns the correct binding.

Test plan

An additional test case has been added to verify the function remains in the final output.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 4, 2021
@motiz88 motiz88 self-requested a review March 25, 2021 10:07
Copy link
Contributor

@motiz88 motiz88 left a comment

Choose a reason for hiding this comment

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

Thank you @leops! This seems correct AFAICT. I appreciate the test, too.

@facebook-github-bot
Copy link
Contributor

@motiz88 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@motiz88 merged this pull request in 150d13e.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants