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

Annex B.3.3.1 intention with arguments parameter shadowing #815

Closed
syg opened this issue Feb 14, 2017 · 3 comments
Closed

Annex B.3.3.1 intention with arguments parameter shadowing #815

syg opened this issue Feb 14, 2017 · 3 comments

Comments

@syg
Copy link
Contributor

syg commented Feb 14, 2017

(Credit to @anba for discovering this in Firefox's bug 1339123)

Consider the following two programs:

(function (a = arguments) {
  print(arguments);
  var arguments;
}());

and

(function (a = arguments) {
  print(arguments);
  { function arguments() { } }
}());

According to the current spec text (ES2017), both programs should print [object Arguments].
According to ES2015, the second program prints undefined.

PR #677 from @jswalden resulted in the semantics change, though he confirmed over IRC any semantics changes from the PR were unintended.

What is the intended behavior here? I am of the opinion both programs should print [object Arguments]. That is, that the current spec is correct, and ES2015 had an inconsistency bug that has since been fixed.

For completeness, SpiderMonkey prints [object Arguments] for both. V8 prints undefined for both. I don't have JSC and Chakra handy, but @anba says they both print [object Arguments] for both tests.

@syg
Copy link
Contributor Author

syg commented Feb 14, 2017

The related question from the actual Firefox bug is the following.

(function (a = arguments) {
  { function arguments() { } }
  print(arguments);
}());

Should this program print [object Arguments] or the function? Current spec text says [object Arguments], i.e., Annex B semantics for FIB does not apply. ES2015 says the function, i.e. Annex B semantics does apply.

Edit: for completeness, SpiderMonkey and V8 both print the function, i.e., apply Annex B semantics.

@littledan
Copy link
Member

@syg I agree with your opinion here.

To spell out some more analysis (which you probably already did, but just to verify that my understanding matches yours), looks like adding arguments to parameterNames is done as the strategy for specifying that if you do var arguments in the body of a function, then arguments still refers to the arguments object. On the other hand, the general design rule for arguments is that, apart from this case, it should be easy to override and trigger it not being created (see all the logic for calculating argumentsObjectNeeded; maybe @allenwb has some more context on the design goals for arguments), so it feels to me like Annex B 3.3 should apply.

We could implement this by making two separate Lists, one for the "real" parameterNames which would be referenced in Annex B 3.3, and one for what's used in step 26-27 for determining which var declarations to actually do something in response to. Even without this semantic issue, I think that would've made sense as a helpful editorial change, as step 21 reads and uses the parametersName list before adding arguments, and then 26-27 uses it after, which is unnecessarily confusing.

@anba
Copy link
Contributor

anba commented Feb 14, 2017

(Credit to @anba for discovering this in Firefox's bug 1339123)

Which was only discovered thanks to the test cases added in WebKit/WebKit@9dd0775. 😄

littledan added a commit to littledan/ecma262 that referenced this issue Apr 12, 2017
An editorial change accidentally blocked this hoisting; this patch
restores it. Corresponding tests already pass on JSC and ChakraCore.

Closes tc39#815
littledan added a commit to littledan/test262 that referenced this issue Apr 12, 2017
leobalter pushed a commit to tc39/test262 that referenced this issue Apr 13, 2017
Ref tc39/ecma262#815
Ref tc39/ecma262#889

This is testing the current semantics of the specs, rather than the semantics in the proposed referenced issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants