Implement ES6 Symbol.unscopables

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: till, Assigned: jorendorff)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments)

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.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Reporter

Updated

5 years ago
Blocks: 875433
Blocks: 1119779
No longer blocks: es6
Assignee

Comment 2

4 years ago
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...)
Assignee: jwalden+bmo → jorendorff
Duplicate of this bug: 1232303

Comment 8

3 years ago
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.
Attachment #8724731 - Flags: review?(shu) → review+

Comment 9

3 years ago
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.
Attachment #8724733 - Flags: review?(shu) → review+
Assignee

Comment 10

3 years ago
(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
Assignee

Comment 11

3 years ago
(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.
Assignee

Comment 12

3 years ago
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.
Assignee

Comment 13

3 years ago
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.
Flags: needinfo?(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.
Flags: needinfo?(shu)

Comment 16

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b61921a307e9
https://hg.mozilla.org/mozilla-central/rev/e9e74f6bd12a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1258140
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8461e2ffc339

We need Array.prototype[@@unscopables] for relanding.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 19

3 years ago
backoutbugherder
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/8461e2ffc339
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1258236

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/be489a094b3c
https://hg.mozilla.org/mozilla-central/rev/942b0d89bef0
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.