Bug 142829 – [ES6] Implement Symbol.unscopables

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Implement Symbol.unscopables support in with scope.
It allow runtime to expose some generic name methods like `values`.

[Yusuke Suzuki]

Created attachment 249026 [details]
Patch

[Yusuke Suzuki]

Created attachment 249027 [details]
Patch

[Yusuke Suzuki]

Could anyone take a look?

[Joseph Pecoraro]

Comment on attachment 249027 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249027&action=review

Nifty!

> Source/JavaScriptCore/tests/stress/unscopables.js:25
> +        test(__proto__, Array.prototype);  // array's unscopbales.__proto__ is undefined => false.

Typo: unscopbales

> Source/JavaScriptCore/tests/stress/unscopables.js:47
> +        test(toString, "toString");  // object.toString is hidden by unscopables.toString.

Can we have an equivalent test where the unscopables object doesn't have the Object prototype? So: [Symbol.unscopables]: { Cocoa: false, Cappuccino: true, __proto__: null }. In which I think we would expect test(toString) to be Object.prototype.toString within this with block.

[Yusuke Suzuki]

Created attachment 249193 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 249027 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249027&action=review

Added comments and updated the patch.

>> Source/JavaScriptCore/tests/stress/unscopables.js:25
>> +        test(__proto__, Array.prototype);  // array's unscopbales.__proto__ is undefined => false.
> 
> Typo: unscopbales

Oops! Thank you.
Fixed.

>> Source/JavaScriptCore/tests/stress/unscopables.js:47
>> +        test(toString, "toString");  // object.toString is hidden by unscopables.toString.
> 
> Can we have an equivalent test where the unscopables object doesn't have the Object prototype? So: [Symbol.unscopables]: { Cocoa: false, Cappuccino: true, __proto__: null }. In which I think we would expect test(toString) to be Object.prototype.toString within this with block.

Sounds very nice. Added.

[Yusuke Suzuki]

Created attachment 249852 [details]
Patch

[Yusuke Suzuki]

Created attachment 249853 [details]
Patch

[Geoffrey Garen]

Comment on attachment 249853 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249853&action=review

r=me

> Source/JavaScriptCore/runtime/JSScope.cpp:161
> +            if (scope->type() != WithScopeType)
> +                return object;
> +
> +            JSValue unscopables = object->get(exec, exec->propertyNames().unscopablesSymbol);
> +            if (exec->hadException())
> +                return object;
> +            if (!unscopables.isObject())
> +                return object;
> +            JSValue blocked = jsCast<JSObject*>(unscopables)->get(exec, ident);
> +            if (exec->hadException())
> +                return object;
> +            if (!blocked.toBoolean(exec))
> +                return object;
> +            ASSERT_WITH_MESSAGE(!exec->hadException(), "JSValue::toBoolean does not raise any errors.");

It would be nice to factor this code into a static inline helper function named "isUnscopable".

[Yusuke Suzuki]

Committed r182225: <http://trac.webkit.org/changeset/182225>

[Yusuke Suzuki]

Comment on attachment 249853 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=249853&action=review

Thank you for your review! I've landed it with fix.

>> Source/JavaScriptCore/runtime/JSScope.cpp:161
>> +            ASSERT_WITH_MESSAGE(!exec->hadException(), "JSValue::toBoolean does not raise any errors.");
> 
> It would be nice to factor this code into a static inline helper function named "isUnscopable".

OK! I've extracted it to isUnscopable function.