fix: handle block-level function declaration (#10046)

[vitorveiga]

Q	A
Fixed Issues?	Fixes #10046, resolves #10050
Patch: Bug Fix?	Yes
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?	No
License	MIT

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 627fdcb:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/32208/

[vitorveiga]

Hello.

@JLHwung did you have a chance to review this PR?

Best Regards

[JLHwung]

I think it suffices to check if the following conditions are both true.

1. binding.kind is "hoisted"
2. key is not defined in parent scope

const binding = scope.getOwnBinding(key);
if (binding) {
  if (binding.kind === "hoisted" && !scope.parent.hasOwnBinding(key)) {
    continue;
  }
  scope.rename(ref.name);
}

The first condition is actually a shortcut for (binding && binding.path && !binding.path.isFunctionDeclaration()). As of the second condition, we don't have to check isVar because the fact that it is one of letRefs implies there must be a let/const binding in current scope chain. Since the binding.kind is "hoisted", it must be defined in upper scope so we can simply check whether an upper scope own a binding named as key.

[vitorveiga]

Thanks for your review :)

Oh ok.. didn't know that binding.kind === 'hoisted' is in fact a shortcut for checking if its a function declaration.
On the other hand, I think we should keep the isVar condition because we need to rename the variable if the upper scope has a const or let variable with the same name. However, if its a var we should skip the renaming operation.

The conditions were updated!

[JLHwung]

I think we should keep the isVar condition because we need to rename the variable if the upper scope has a const or let variable with the same name. However, if its a var we should skip the renaming operation.

Note that the renaming is nested in for (const key of letRefs.keys()) loop. In this loop, if !parentBinding is false, which means key is owned in parent scope, then isVar(parentBinding.path.parent) must be false otherwise it contradicts with the fact that key is of letRefs.

(You can also click on "Details" to see my implementation in #11801 (review))

[vitorveiga]

Note that the implementation in #11801, fails on the unit-test issue-10046-collision-var due to the variable declaration on the program node.

In that particular unit test, the !parentBinding is false and without isVar condition the rename will happen turning the function declaration into to var _run = function() {}.

if !parentBinding is false, which means key is owned in parent scope, then isVar(parentBinding.path.parent) must be false otherwise it contradicts with the fact that key is of letRefs.

Maybe there is some implementation bug on letRefs map, but it seems that isVar(parentBinding.path.parent) is true on that use case.

[vitorveiga]

Hello @JLHwung did you have a chance to read my last comment? Thanks

[nicolo-ribaudo]

This PR caused some regressions: https://app.circleci.com/pipelines/github/babel/babel/4967/workflows/27043ada-f8d2-482e-810b-2593a17cf087/jobs/35381