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

Legacy hoisting for `function arguments() {}` still not correct? #991

Open
anba opened this Issue Sep 6, 2017 · 4 comments

Comments

Projects
None yet
4 participants
@anba
Contributor

anba commented Sep 6, 2017

refs: #815, #889, #891

Per the current spec semantics:

function f(a = () => arguments) {
  print(arguments);   // Print [object Arguments]
  print(a());         // Print [object Arguments]
  { function arguments() {} }
  print(arguments);   // Print function arguments() {}
  print(a());         // Print [object Arguments]
  
  delete arguments;
  print(arguments);   // Print [object Arguments]
  print(a());         // Print [object Arguments]
}
f();

Explanation:

  1. function arguments() {} is an applicable legacy block-scoped function, because it can be replaced with var arguments without causing an early SyntaxError, and arguments is not a parameter name (B.3.3.1 Changes to FunctionDeclarationInstantiation, step 1.a.ii).
  2. B.3.3.1 Changes to FunctionDeclarationInstantiation, step 1.a.ii.2 does not create a binding in varEnvRec, because the function name is "arguments".
  3. 9.2.12 FunctionDeclarationInstantiation, step 28 also doesn't create a binding for "arguments" in varEnvRec.
  4. So when B.3.3.1 Changes to FunctionDeclarationInstantiation, step 1.a.ii.3.f is executed, fenvRec doesn't have a binding for "arguments".
  5. That means 8.1.1.1.5 SetMutableBinding, step 2.b creates a deletable binding.

Maybe we just need something like this in B.3.3.1:

  1. If instantiatedVarNames does not contain F, then
    1. Perform ! varEnvRec.CreateMutableBinding(F, false).
    2. If F is not "arguments" or argumentsObjectNeeded is false, let initialValue be undefined.
    3. Else,
      1. Let initialValue be ! envRec.GetBindingValue(F, false).
    4. Perform varEnvRec.InitializeBinding(F, initialValue).
    5. Append F to instantiatedVarNames.

And two nits in B.3.3.1 Changes to FunctionDeclarationInstantiation:

  • Step 28 -> Step 29
  • Typo: initializedBindings-> instantiatedVarNames.
@anba

This comment has been minimized.

Show comment
Hide comment
@leobalter

This comment has been minimized.

Show comment
Hide comment
@leobalter
Member

leobalter commented Sep 6, 2017

cc @rwaldron ^^

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Sep 13, 2017

Member

I'm not so happy with how complicated it turns out to support legacy hosting of functions named argument. Maybe we should just consistently skip this hoisting, as I guess were the semantics at some point recently.

Member

littledan commented Sep 13, 2017

I'm not so happy with how complicated it turns out to support legacy hosting of functions named argument. Maybe we should just consistently skip this hoisting, as I guess were the semantics at some point recently.

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Sep 13, 2017

Member

@littledan
I'm with you. Legacy FiB supports solely to not break existing code that is interoperable among major browsers. If there is evidence that not hoisting functions named arguments does not result in significant breakage then we should add it the complexity. The rules for binding arguments is already too complicated.

Member

allenwb commented Sep 13, 2017

@littledan
I'm with you. Legacy FiB supports solely to not break existing code that is interoperable among major browsers. If there is evidence that not hoisting functions named arguments does not result in significant breakage then we should add it the complexity. The rules for binding arguments is already too complicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment