Closed Bug 1054759 Opened 10 years ago Closed 8 years ago

Implement ES6 Symbol.unscopables

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: till, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

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
Blocks: 875433
Blocks: 1119779
No longer blocks: es6
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
Attachment #8724733 - Flags: review?(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.
Attachment #8724731 - Flags: review?(shu) → review+
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+
(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
(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.
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.
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)
https://hg.mozilla.org/mozilla-central/rev/b61921a307e9
https://hg.mozilla.org/mozilla-central/rev/e9e74f6bd12a
Status: ASSIGNED → RESOLVED
Closed: 8 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 → ---
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/8461e2ffc339
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1258236
https://hg.mozilla.org/mozilla-central/rev/be489a094b3c
https://hg.mozilla.org/mozilla-central/rev/942b0d89bef0
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.