Array.prototype.toLocaleString does not respect deletion of Object.prototype.toLocaleString

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Richard Gibson]

The algorithm at https://tc39.es/ecma402/#sup-array.prototype.tolocalestring requires looking up a "toLocaleString" property on each non-undefined non-null element of the receiver array, invoking it as a method, and (absent an exception) passing the result through ToString. However, JSC appears to have a hidden but always-present default method.

The following statement list should throw an exception, but does not:
  delete Object.prototype.toLocaleString;
  [{}].toLocaleString();

Also reported to test262 for coverage: https://github.com/tc39/test262/issues/3298

[Yusuke Suzuki]

Instead of this, we will implement Intl.ListFormat based new implementation.

[Radar WebKit Bug Importer]

<rdar://problem/85310560>

[Yusuke Suzuki]

*** Bug 232724 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Intl.ListFormat-based implementation is deferred. For now, let's implement the current Array.prototype.toLocaleString in ECMA 402.

[Yusuke Suzuki]

Created attachment 448233 [details]
Patch

[Yusuke Suzuki]

Created attachment 448234 [details]
Patch

[Yusuke Suzuki]

Created attachment 448236 [details]
Patch

[Yusuke Suzuki]

Created attachment 448243 [details]
Patch

[Alexey Shvayka]

Comment on attachment 448243 [details]
Patch

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

Sweet, r=me with nits.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:696
> +    if (UNLIKELY(arguments.hasOverflowed())) {

Given the default inline capacity of 8, can this be simplified to `ASSERT(!arguments.hasOverflowed())`?

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:760
> +        JSValue element = thisObject->get(globalObject, k);

I appreciate following the spec 1:1, but since we are already a bit off with handling of 0th element, maybe we could do `thisObject->getIndex(globalObject, k)` here?
Also, getIndex() is currently used in trunk.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:809
> +        JSValue element = thisObject->get(globalObject, k);

Since we are changing this line, maybe we could do `thisObject->getIndex(globalObject, k)`?

[Yusuke Suzuki]

Comment on attachment 448243 [details]
Patch

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

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:696
>> +    if (UNLIKELY(arguments.hasOverflowed())) {
> 
> Given the default inline capacity of 8, can this be simplified to `ASSERT(!arguments.hasOverflowed())`?

Sounds good!

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:760
>> +        JSValue element = thisObject->get(globalObject, k);
> 
> I appreciate following the spec 1:1, but since we are already a bit off with handling of 0th element, maybe we could do `thisObject->getIndex(globalObject, k)` here?
> Also, getIndex() is currently used in trunk.

Nice, fixed.

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:809
>> +        JSValue element = thisObject->get(globalObject, k);
> 
> Since we are changing this line, maybe we could do `thisObject->getIndex(globalObject, k)`?

Nice, fixed.

[Yusuke Suzuki]

Committed r287560 (245695@trunk): <https://commits.webkit.org/245695@trunk>

[Yusuke Suzuki]

Committed r287561 (245696@trunk): <https://commits.webkit.org/245696@trunk>