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.3 should not unconditionally overwrite global bindings with `undefined` in EvalDeclarationInstantiation #753

Closed
syg opened this issue Dec 14, 2016 · 16 comments

Comments

Projects
None yet
6 participants
@syg
Copy link
Contributor

commented Dec 14, 2016

André Bargull filed the following bug for Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1317373

This seems like a spec bug to me. EDI probably needs extra steps other than CanDeclareGlobalFunction to see if it should call CreateGlobalFunctionBinding.

The above bug reproduced inline for ease:

Test case:

var f = "x";
eval("print(f); { function f(){} }")

Expected: Prints "undefined"
Actual: Prints "x"

V8 and Chakra print "x", JSC prints "function f() { }".

ES2017 spec: B.3.3.3, step 7.a.i.i
https://tc39.github.io/ecma262/#sec-web-compat-evaldeclarationinstantiation

@bakkot

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2016

In particular, according to B.3.3.3 (EvalDeclarationInstantiation):

  • 1.d.ii.4 returns immediately, leaving bindingExists as false
  • varEnvRec is indeed a global environment record, so 1.d.ii.5 is entered
  • The global environment record has no lexical declaration named f, so 1.d.ii.5.a is entered
  • CanDeclareGlobalFunction will see that the existing property named f is a data property and is writable and enumerable, and so returns true: thus fnDefinable will be set to true.
  • 1.d.ii.7.a.i.i then performs CreateGlobalFunctionBinding(F, undefined, true), which sets the value of the f global variable to undefined, overwriting its existing value.
@bakkot

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2016

Contrast the behavior in a function:

function a() {
  var f = 42;
  eval("print(f); { function f(){} }");
}
a();

Per spec this prints 42, because B.3.3.3.1.d.ii.7.a.ii.i avoids clobbering the existing f var binding.

@syg

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2016

FWIW no implementation implements the current spec behavior of overwriting with "undefined".

@littledan

This comment has been minimized.

Copy link
Member

commented Dec 27, 2016

Ugh, what should the semantics be? Will it read the value of the property of the global object and rewrite it (invoking getters at the top of the eval block)? Does any browser do that?

EDIT: Not sure what I was thinking when I wrote this; please ignore this comment.

@bakkot

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2016

It's just occurred to me that this comes up without eval too, in GlobalDeclarationInstantiation:

<script>
var f = 42;
</script>
<script>
console.log(window.f); // per spec, prints 'undefined'
{
  function f(){}
}
</script>

I think the semantics should be, in both cases, that you don't create a new binding if one already exists but you still overwrite it when executing the FunctionDeclaration. As to accessors... personally I'd prefer that no hoisting is performed if the relevant property is a accessors, but there might be cross-browser agreement about what to do.

The two aren't completely symmetric, in that in the global case you perform CreateGlobalFunctionBinding at most once for each name, but in the eval case you perform it once for each block-scoped function of that name, which is observable if the global object is a proxy with a trap for defineOwnProperty. But this probably isn't worth worrying about.

@bakkot

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2016

One more only tangentially related thing: I've just noticed that all of {v8, SpiderMonkey, JSC} agree that

<script>
let f = 42;
</script>
<script>
{
  function f(){}
}
</script>

is a SyntaxError, but

<script>
let f = 42;
</script>
<script>
"use strict";
{
  function f(){}
}
</script>

is fine.

This seems wrong.

I expect this is because the GlobalDeclarationInstantiation case in B.3.3 is lacking the loop in 1.d.ii.4 of Changes to EvalDeclarationInstantiation.

@anba

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2017

One more only tangentially related thing: I've just noticed that all of {v8, SpiderMonkey, JSC} agree that [...]

So that just means they don't implement step 2.d.ii.1 of B.3.3.2?

If envRec.HasLexicalDeclaration(F) is false, then ...

@anba

This comment has been minimized.

Copy link
Collaborator

commented Feb 28, 2017

If we replace the CreateGlobalFunctionBinding calls in B.3.3.2 and B.3.3.3 with CreateGlobalVarBinding, we probably achieve the desired effect of not overwriting an existing binding value, because CreateGlobalVarBinding has an additional HasOwnProperty check.

@littledan

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

FWIW now some test262 tests reflect the state of the specification, e.g., annexB/language/eval-code/indirect/global-if-decl-else-decl-a-eval-global-existing-global-init . What's been said on this thread about what the semantics should be sounds reasonable to me.

@littledan

This comment has been minimized.

Copy link
Member

commented Apr 6, 2017

@anba Your suggestion would have the effect of making the environment record enumerable, whereas it's currently not enumerable, wouldn't it?

@anba

This comment has been minimized.

Copy link
Collaborator

commented Apr 7, 2017

@littledan Can you clarify? https://tc39.github.io/ecma262/#sec-createglobalfunctionbinding also creates enumerable properties, so using https://tc39.github.io/ecma262/#sec-createglobalvarbinding shouldn't make a difference here.

@littledan

This comment has been minimized.

Copy link
Member

commented Apr 11, 2017

@anba Not sure what I was thinking of; of course they're made enumerable in both cases. Staring at the spec longer, it seems like the only distinction might be that different Proxy traps are invoked. However, the distinction should be unobservable on the web. If we did care about that, an alternative to calling CreateGlobalVarBinding would be to allow ~empty~ to be passed to CreateGlobalFunctionBinding, and leave out the [[Value]] field in the record passed to CreatePropertyDescriptorOrThrow if that's the case. That should be mostly equivalent to what you're suggesting, right?

@anba

This comment has been minimized.

Copy link
Collaborator

commented Apr 11, 2017

Do you mean something like this?

  1. Let existingProp be ? globalObject.[[GetOwnProperty]](N).
  2. If existingProp is undefined or existingProp.[[Configurable]] is true, then
    1. Let desc be the PropertyDescriptor{[[Value]]: V, [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: D}.
    2. If V is not empty, then
      1. Let desc be the PropertyDescriptor{[[Value]]: V, [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: D}.
    3. Else,
      1. Let desc be the PropertyDescriptor{[[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: D}.
  3. Else,
    1. Let desc be the PropertyDescriptor{[[Value]]: V }.
    2. If V is not empty, then
      1. Let desc be the PropertyDescriptor{[[Value]]: V }.
    3. Else,
      1. Let desc be the PropertyDescriptor{ }.
  4. Perform ? DefinePropertyOrThrow(globalObject, N, desc).
  5. Record that the binding for N in ObjRec has been initialized.
  6. Perform ? Set(globalObject, N, V, false).
  7. If V is not empty, then
    1. Perform ? Set(globalObject, N, V, false).

This seems a bit too complicated to me, I'd rather prefer a different abstract operation in this case.


That should be mostly equivalent to what you're suggesting, right?

No. CreateGlobalFunctionBinding overrides existing property attributes whereas CreateGlobalVarBinding keeps the current attributes:

var global = this;
Object.defineProperty(global, "f", {configurable: true, writable: false});
eval("{ function f() {} }");
// Current spec: {"writable":true,"enumerable":true,"configurable":true}
print(JSON.stringify(Object.getOwnPropertyDescriptor(global, "f")));
// Current spec: function f() {}
print(global.f);

When CreateGlobalFunctionBinding is used, the global property will be changed to {[[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: true}, whereas when CreateGlobalVarBinding is used, the property will keep the original attributes, that means it will be {[[Writable]]: false, [[Enumerable]]: false, [[Configurable]]: true}.


Considering this example, we may also want to change CanDeclareGlobalFunction to CanDeclareGlobalVar (if we want to align the spec with current engine behaviour).

var global = this;
Object.defineProperty(global, "f", {writable: true, enumerable: false, configurable: false});
eval("{ function f() {} }");

// Current spec: {"writable":true,"enumerable":false,"configurable":false}
print(JSON.stringify(Object.getOwnPropertyDescriptor(global, "f")));
// Current spec: undefined (b/c CanDeclareGlobalFunction returned false!)
print(global.f);

littledan added a commit to littledan/ecma262 that referenced this issue Apr 12, 2017

Normative: Create global properties for sloppy function hosting as vars
This patch removes a case where a synthetic *undefined* was visible
as a result of legacy sloppy-mode function hoisting.

This patch addresses tc39#753

littledan added a commit to littledan/test262 that referenced this issue Apr 12, 2017

Add templated tests for new proposed Annex B semantics
These tests are againt a proposed fix for
tc39/ecma262#753

They seem to pass in V8, JSC and SpiderMonkey, though ChakraCore
hews slightly closer to the previous specification.
@littledan

This comment has been minimized.

Copy link
Member

commented Apr 12, 2017

@anba OK, thanks for explaining and bearing with me on this. I wrote a spec patch and test262 tests for this. It seems to be the semantics from three engines (minus ChakraCore). Any thoughts?

littledan added a commit to littledan/test262 that referenced this issue Mar 7, 2018

Add templated tests for new proposed Annex B semantics
These tests are againt a proposed fix for
tc39/ecma262#753

They seem to pass in V8, JSC and SpiderMonkey, though ChakraCore
hews slightly closer to the previous specification.

rwaldron added a commit to tc39/test262 that referenced this issue May 11, 2018

Add templated tests for new proposed Annex B semantics
These tests are againt a proposed fix for
tc39/ecma262#753

They seem to pass in V8, JSC and SpiderMonkey, though ChakraCore
hews slightly closer to the previous specification.
@rwaldron

This comment has been minimized.

Copy link
Contributor

commented May 11, 2018

Spec tests landed: tc39/test262#969

ljharb added a commit to littledan/ecma262 that referenced this issue Jul 18, 2018

Normative: Create global properties for sloppy function hosting as vars
This patch removes a case where a synthetic *undefined* was visible
as a result of legacy sloppy-mode function hoisting.

This patch addresses tc39#753
@ljharb

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

Closed in #888.

@ljharb ljharb closed this Jul 18, 2018

chicoxyzzy added a commit to chicoxyzzy/test262 that referenced this issue May 14, 2019

Add templated tests for new proposed Annex B semantics
These tests are againt a proposed fix for
tc39/ecma262#753

They seem to pass in V8, JSC and SpiderMonkey, though ChakraCore
hews slightly closer to the previous specification.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.