Bug 170882 - test262: test262/test/built-ins/Object/prototype/toLocaleString/primitive_this_value.js
Summary: test262: test262/test/built-ins/Object/prototype/toLocaleString/primitive_thi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-15 22:09 PDT by Joseph Pecoraro
Modified: 2017-04-16 19:59 PDT (History)
7 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (8.03 KB, patch)
2017-04-15 22:13 PDT, Joseph Pecoraro
sbarati: review-
sbarati: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (8.72 KB, patch)
2017-04-16 18:50 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-04-15 22:09:18 PDT
test262/test/built-ins/Object/prototype/toLocaleString/primitive_this_value.js

Summary:
In strict mode retains the primitive valueless of a this object.
In invoking toString the primitive value is passed not the ToObject version of it.
So the end result is both the toString accessor and invocation the this object is
the primitive value not an object.

Boiled down to:

    "use strict";
    Boolean.prototype.toString = function() { return typeof this; }
    assert( true.toLocaleString() === "boolean" )

Notes:
- Chrome and Firefox both pass
- Safari produced "object"

Spec:
19.1.3.5 Object.prototype.toLocaleString ( [ reserved1 [ , reserved2 ] ] )
https://tc39.github.io/ecma262/#_ref_3826

> 1. Let O be the this value.
> 2. Return ? Invoke(O, "toString").

7.3.18 Invoke ( V, P [ , argumentsList ] )
https://tc39.github.io/ecma262/#sec-invoke

> ...
> 3. Let func be ? GetV(V, P).
> 4. Return ? Call(func, V, argumentsList).

7.3.2 GetV ( V, P )
https://tc39.github.io/ecma262/#sec-getv

> ...
> 2. Let O be ? ToObject(V).
> 3. Return ? O.[[Get]](P, V).

7.3.12 Call ( F, V [ , argumentsList ] )
https://tc39.github.io/ecma262/#sec-call

> 1. If argumentsList was not passed, set argumentsList to a new empty List.
> 2. If IsCallable(F) is false, throw a TypeError exception.
> 3. Return ? F.[[Call]](V, argumentsList).

The key parts of this are:

  - In toLocaleString, O is the this value (not a ToObject of it)
  - GetV uses [[Get]] passing along the value (not the ToObject) to use as the receiver when getting an accessor
  - Call is also passed the value (not the ToObject) to use as the this object in the call

This must be one of the weirdest corners of JavaScript.
Comment 1 Joseph Pecoraro 2017-04-15 22:13:52 PDT
Created attachment 307217 [details]
[PATCH] Proposed Fix

I didn't add a new test for this because I considered it too obscure. test262 covers it, and I think that should be enough.
Comment 2 Saam Barati 2017-04-16 10:05:15 PDT
Comment on attachment 307217 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:294
> +    JSValue toString = object->getV(exec, vm.propertyNames->toString, thisValue);

We usually do this another way:
We already have a way of distinguishing between |this| and receiver. The first argument to PropertySlot constructor is |this|. Then, you can call getPropertySlot on the receiver. That's what I'd do here instead of making this new function, which looks slightly wrong since it sets |this| only after doing the lookup. You can write code like this:

PropertySlot slot(thisValue, ...Get);
JSValue toString =  object->getPropertySlot(exec, slot, toStringIdent) ? slot.getValue(exec, toStringIdent) : jsUndefined();
Comment 3 Joseph Pecoraro 2017-04-16 13:40:10 PDT
Comment on attachment 307217 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:294
>> +    JSValue toString = object->getV(exec, vm.propertyNames->toString, thisValue);
> 
> We usually do this another way:
> We already have a way of distinguishing between |this| and receiver. The first argument to PropertySlot constructor is |this|. Then, you can call getPropertySlot on the receiver. That's what I'd do here instead of making this new function, which looks slightly wrong since it sets |this| only after doing the lookup. You can write code like this:
> 
> PropertySlot slot(thisValue, ...Get);
> JSValue toString =  object->getPropertySlot(exec, slot, toStringIdent) ? slot.getValue(exec, toStringIdent) : jsUndefined();

Much cleaner!
Comment 4 Joseph Pecoraro 2017-04-16 18:50:57 PDT
Created attachment 307252 [details]
[PATCH] Proposed Fix
Comment 5 WebKit Commit Bot 2017-04-16 19:59:43 PDT
Comment on attachment 307252 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 307252

Committed r215405: <http://trac.webkit.org/changeset/215405>
Comment 6 WebKit Commit Bot 2017-04-16 19:59:45 PDT
All reviewed patches have been landed.  Closing bug.