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

Don't resolve shadowed arguments variables from functions #14036

Conversation

The-x-Theorist
Copy link
Contributor

@The-x-Theorist The-x-Theorist commented Dec 9, 2021

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

Function's implicit argument binding now won't fail because the arguments identifier in function will be renamed differently than the arguments identifier in any another scope.

@babel-bot
Copy link
Collaborator

babel-bot commented Dec 9, 2021

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

@nicolo-ribaudo
Copy link
Member

The fix should be inside the renamer. You can edit this code:

if (
!path.scope.bindingIdentifierEquals(
state.oldName,
state.binding.identifier,
)
) {

to also skip if state.oldName === "arguments" && path.isFunction() && !path.isArrowFunctionExpression().

@nicolo-ribaudo
Copy link
Member

Or even better, if it works, in getBinding (so that bindingIdentifierEquals then returns false):

getBinding(name: string): Binding | undefined {
let scope: Scope = this;
let previousPath;
do {
const binding = scope.getOwnBinding(name);
if (binding) {
// Check if a pattern is a part of parameter expressions.
// Note: for performance reason we skip checking previousPath.parentPath.isFunction()
// because `scope.path` is validated as scope in packages/babel-types/src/validators/isScope.js
// That is, if a scope path is pattern, its parent must be Function/CatchClause
// Spec 9.2.10.28: The closure created by this expression should not have visibility of
// declarations in the function body. If the binding is not a `param`-kind (as function parameters)
// or `local`-kind (as id in function expression),
// then it must be defined inside the function body, thus it should be skipped
if (
previousPath?.isPattern() &&
binding.kind !== "param" &&
binding.kind !== "local"
) {
// do nothing
} else {
return binding;
}
}
previousPath = scope.path;
} while ((scope = scope.parent));
}

If !binding && name === "arguments" && scope.path.isFunction() && !scope.path.isArrowFunctionExpression(), we can break the loop and return undefined. This is because in this code:

var arguments = [];
function f() {
  return arguments;
}

the return arguments does not refer to var arguments.

@The-x-Theorist
Copy link
Contributor Author

@nicolo-ribaudo Okay, I got I will make the changes just as you described in the next commit.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thanks!

@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Dec 11, 2021
@@ -0,0 +1,4 @@
{
"sourceType": "unambiguous",
"presets": ["env"]
Copy link
Member

Choose a reason for hiding this comment

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

The Babel 8 tests are failing because you didn't specify any target here. Could you move this test in the arrow functions plugin, and enable the plugin instead of the preset?
Also, please specify "sourceType": "script" since this is specific to loose mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I can do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the same as you said.

@@ -1,7 +1,7 @@
var _arguments2 = 1;

function fn() {
var _arguments = _arguments2;
var _arguments = arguments;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for self and others: this is correct, the test was previously broken — fn should not see the outer arguments, because it was not an arrow function in input.js

@nicolo-ribaudo nicolo-ribaudo changed the title Fix: Function's implicit argument binding fail Don't resolve shadowed arguments variables from functions Dec 13, 2021
@nicolo-ribaudo nicolo-ribaudo merged commit add64e8 into babel:main Dec 13, 2021
@The-x-Theorist The-x-Theorist deleted the function-implicit-argument-binding-fail branch December 14, 2021 05:39
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 16, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Function's Implicit Arguments Binding Fail
5 participants