1054759 - Implement ES6 Symbol.unscopables

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

Requires
- changing HasBinding [1]
- changing GetBindingValue [2]
- adding Array.prototype[@@unscopables] [3]

IIUC, this'd be very useful for DOM event listeners.

[1]: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-object-environment-records-hasbinding-n
[2]: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-object-environment-records-getbindingvalue-n-s
[3]: http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype-@@unscopables

Comment 1

[Jeff Walden [:Waldo]]

Having looked at the current mess that is all our scoping code for bug 589199, I'm convinced we need to add an actual environment record concept to SpiderMonkey.  Doing that work should give a nice basis for understanding what's needed to do this, so I'll take this.

Comment 2

[Jason Orendorff [:jorendorff]]

Stealing bug. I think we can do it in terms of the existing DynamicWithObject stuff.

(But, here's hoping we don't need to care about performance here...)

Comment 4

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2934c33cac3

Comment 5

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa09764d633c

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Reinstate JS_FOR_EACH_WELL_KNOWN_SYMBOL

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Part 2: ES6 Symbol.unscopables

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8724731 [details] [diff] [review]
Part 1: Reinstate JS_FOR_EACH_WELL_KNOWN_SYMBOL

Review of attachment 8724731 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.h
@@ +4730,5 @@
>  };
>  
>  /* For use in loops that iterate over the well-known symbols. */
> +#define JS_PLUS_ONE(name) + 1
> +const size_t WellKnownSymbolLimit = 0 JS_FOR_EACH_WELL_KNOWN_SYMBOL(JS_PLUS_ONE);

According to C++11 7.2.2, the first enumerated value, if not given an explicit value, is 0. So if the compilers are compliant, the WellKnownSymbolLimit is better as an enum value above.

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8724733 [details] [diff] [review]
Part 2: ES6 Symbol.unscopables

Review of attachment 8724733 [details] [diff] [review]:
-----------------------------------------------------------------

Is Array.prototype[@@unscopables] left to a different patch?

I must say your tests are simply marvelous.

::: js/src/tests/ecma_6/LexicalEnvironment/unscopables-mutation-frozen.js
@@ +1,1 @@
> +// When env[@@unscopables].x changes, bindings can appear even if env is inextensible.

>:|

::: js/src/tests/ecma_6/LexicalEnvironment/unscopables-mutation.js
@@ +28,5 @@
> +
> +    // The determination of which binding is assigned happens when the LHS of
> +    // assignment is evaluated, before the RHS. This is observable if we make
> +    // the binding appear or disappear during evaluation of the RHS, before
> +    // assigning.

Oh wow, I wouldn't have thought to test this.

::: js/src/vm/Debugger.cpp
@@ +8514,5 @@
> +            return false;
> +        if (!found) {
> +            args.rval().setUndefined();
> +            return true;
> +        }

Is this needed? This case should fall out automatically below. DSOs of DynamicWithObjects fall through to GetProperty, which respects LookupProperty, which DynamicWithObjects have hooked.

::: js/src/vm/ScopeObject.cpp
@@ +790,5 @@
> +
> +    if (propp) {
> +        bool scopable;
> +        if (!CheckUnscopables(cx, actual, id, &scopable))
> +            return false;

This affects both syntactic and non-syntactic with. I think this is great; chrome code get a private-state mechanism for free for custom scopes.

Comment 10

[Jason Orendorff [:jorendorff]]

(In reply to Shu-yu Guo [:shu] from comment #8)
> > +#define JS_PLUS_ONE(name) + 1
> > +const size_t WellKnownSymbolLimit = 0 JS_FOR_EACH_WELL_KNOWN_SYMBOL(JS_PLUS_ONE);
> 
> According to C++11 7.2.2, the first enumerated value, if not given an
> explicit value, is 0. So if the compilers are compliant, the
> WellKnownSymbolLimit is better as an enum value above.

fixed

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to Shu-yu Guo [:shu] from comment #9)
> ::: js/src/vm/Debugger.cpp
> @@ +8514,5 @@
> > +            return false;
> > +        if (!found) {
> > +            args.rval().setUndefined();
> > +            return true;
> > +        }
> 
> Is this needed? This case should fall out automatically below. DSOs of
> DynamicWithObjects fall through to GetProperty, which respects
> LookupProperty, which DynamicWithObjects have hooked.

Without it, jit-test/tests/debug/Environment-unscopables.js fails on this line:
    assertEq(env.getVariable("x"), undefined);

Error: Assertion failed: got "obj", expected (void 0)

I don't have time to look into it *right* now, but I will before landing.

Comment 12

[Jason Orendorff [:jorendorff]]

OK, the deal is: my patch only touched with_LookupProperty and with_HasProperty, not with_GetProperty.

The reason for that is for correctness in this case:

    with (proxy) { name; }

In the case where proxy.name exists, we should see these traps:

    has("name")              // true
    get(Symbol.unscopables)  // undefined
    get("name")

With the patch as it stands, that's what happens. But if I add a HasProperty check inside of with_GetProperty, in order to enforce @@unscopables there, we get extra traps:

    has("name")              // as before
    get(Symbol.unscopables)
    has("name")              // new calls, from with_GetProperty
    get(Symbol.unscopables)
    get("name")

Therefore with_GetProperty must not enforce @@unscopables, and therefore Debugger::getVariable must.

It's less awful if you consider that the changes in Debugger::getVariable make it more like how GetNameOperation already behaves.

Comment 13

[Jason Orendorff [:jorendorff]]

ni? to make sure :shu sees comment 12, though I'm about to land anyway. I'll follow up if there's a better way.

Comment 14

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b61921a307e9ed7d94e94290dda4672ad2779bd0
Bug 1054759 - Part 1: Reinstate JS_FOR_EACH_WELL_KNOWN_SYMBOL. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9e74f6bd12a8c45bb3e20a0ca573db972ed6345
Bug 1054759 - Part 2: ES6 Symbol.unscopables. r=shu.

Comment 15

[Shu-yu Guo [:shu]]

(In reply to Jason Orendorff [:jorendorff] from comment #13)
> ni? to make sure :shu sees comment 12, though I'm about to land anyway. I'll
> follow up if there's a better way.

Thanks for the explanation. Makes sense.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b61921a307e9
https://hg.mozilla.org/mozilla-central/rev/e9e74f6bd12a

Comment 17

[Shu-yu Guo [:shu]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8461e2ffc339

We need Array.prototype[@@unscopables] for relanding.

Comment 18

[Shu-yu Guo [:shu]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1258140#c4 and https://bugzilla.mozilla.org/show_bug.cgi?id=1258140#c6

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/8461e2ffc339

Comment 20

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be489a094b3ca6665d25d686ad04726ad722647e
Bug 1054759 - Part 1: Reinstate JS_FOR_EACH_WELL_KNOWN_SYMBOL. r=shu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/942b0d89bef069952c753a5a3e036ff60ede1dda
Bug 1054759 - Part 2: ES6 Symbol.unscopables. r=shu.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/be489a094b3c
https://hg.mozilla.org/mozilla-central/rev/942b0d89bef0

Comment 22

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/48#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/unscopables