Closed Bug 1054756 Opened 10 years ago Closed 9 years ago

Implement ES6 Symbol.toPrimitive

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: till, Assigned: jorendorff)

References

(
URL
)

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(5 files, 2 obsolete files)

, part 1 - Support symbol-keyed properties in JSXrayTraits::resolveOwnProperty()
14.23 KB, patch
jandem
: review+
Details | Diff | Splinter Review
, part 2 - Move ToPrimitive slow paths into a non-inline function
6.64 KB, patch
jandem
: review+
Details | Diff | Splinter Review
, part 3 - Implement Symbol.toPrimitive. Replace existing convert hooks with methods
80.21 KB, patch
jandem
: review+
Details | Diff | Splinter Review
, part 4 - Remove BaseProxyHandler::defaultValue
18.75 KB, patch
jandem
: review+
Details | Diff | Splinter Review
, part 5 - Remove Class::convert. try: -b do -p linux64,linux64-st-an,linux64-br-haz,macosx64,win32,android-api-9,emulator,linux64_gecko -u all -t none
74.39 KB, patch
jandem
: review+
Details | Diff | Splinter Review 
The basic implementation should be straight-forward, but I'm fairly sure that we'd want to have JIT optimizations for the common case before landing this. (Where the common case is an invocation of the ToPrimitive abstract operation with an input that's either not an object or doesn't have @@toPrimitive defined.
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
The above patches do not contain any new tests, and they can't land until bug 1069416 anyway.

Passes shell tests. This is a net reduction in lines of code, even without part 3, which will be "delete the convert hook and JS_ConvertStub".

(In reply to Till Schneidereit [:till] from comment #0)
> The basic implementation should be straight-forward, but I'm fairly sure
> that we'd want to have JIT optimizations for the common case before landing
> this. (Where the common case is an invocation of the ToPrimitive abstract
> operation with an input that's either not an object or doesn't have
> @@toPrimitive defined.

Till, which benchmark do you think this will regress? We seem to have a Baseline IC case for (str + obj), ICBinaryArith_StringObjectConcat; is there a benchmark that does that a lot?

(If this won't regress anything, then it won't hurt to fix the behavior in this bug and the performance later.)
Flags: needinfo?(till)
(In reply to Jason Orendorff [:jorendorff] from comment #3)
> Till, which benchmark do you think this will regress? We seem to have a
> Baseline IC case for (str + obj), ICBinaryArith_StringObjectConcat; is there
> a benchmark that does that a lot?

I'm not aware of a benchmark that would be regressed by this, no. I would assume, though, that there's an abundance of real world-code that does ToPrimitive on objects, and that we shouldn't slow down. If you think that that's not something to worry about, then I agree that we can do performance work later.
Flags: needinfo?(till)
Whiteboard: [DocArea=JS]
Blocks: 1119779
No longer blocks: es6
Attachment #8515460 - Attachment is obsolete: true
Attachment #8515461 - Attachment is obsolete: true
JSClass::convert is no longer used after this, but to minimize the noise, it will be deleted in a separate patch. However all non-nullptr convert hook implementations must be replaced with [@@toPrimitive] methods in this patch to avoid changing the behavior.

The changes in XrayWrapper.cpp fix a pre-existing bug: when an Xray wrapper tries to emit the "Silently denied access" warning, if id is a symbol, the existing code triggers an error trying to convert it to a string for the warning message. Implementing Symbol.toPrimitive revealed this bug; the fix is straightforward.
Attachment #8658962 - Flags: review?(jdemooij)
Comment on attachment 8658958 [details] [diff] [review]
, part 1 - Support symbol-keyed properties in JSXrayTraits::resolveOwnProperty()

Review of attachment 8658958 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ +3606,5 @@
> +    GetterOp gop;
> +    SetterOp sop;
> +    if (flags & JSFUN_STUB_GSOPS) {
> +        // JSFUN_STUB_GSOPS is a request flag only, not stored in fun->flags or
> +        // the defined property's attributes.

Do you think we could remove the JSFUN_STUB_GSOPS code from js::DefineFunction now, since we're handling that here now? Or are there other callers that rely on it?

It'll require auditing all DefineFunction/JS_DefineFunction callers so probably not necessary here.

::: js/src/jsfun.cpp
@@ +2265,2 @@
>      else
> +        fun = NewNativeFunction(cx, native, nargs, atom, allocKind, GenericObject);

Nit: GenericObject is the default for these two NewNative* calls, so you could remove it here. If you think it's clearer to be explicit that's also fine.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ -521,5 @@
> -        RootedFunction fun(cx);
> -        if (fsMatch->selfHostedName) {
> -            fun = JS::GetSelfHostedFunction(cx, fsMatch->selfHostedName, id, fsMatch->nargs);
> -        } else {
> -            fun = JS_NewFunctionById(cx, fsMatch->call.op, fsMatch->nargs, 0, id);

Looks like we can remove JS_NewFunctionById now as this was the only caller.
Attachment #8658958 - Flags: review?(jdemooij) → review+
Attachment #8658959 - Flags: review?(jdemooij) → review+
Comment on attachment 8658962 [details] [diff] [review]
, part 3 - Implement Symbol.toPrimitive. Replace existing convert hooks with methods

Review of attachment 8658962 [details] [diff] [review]:
-----------------------------------------------------------------

Really nice we can get rid of the convert hook now.

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +1408,5 @@
> +      vp.setObject(*obj);
> +      return true;
> +    }
> +  }
> +  

Nit: trailing whitespace

::: dom/workers/WorkerScope.cpp
@@ -748,5 @@
> -    vp.setObject(*obj);
> -    return true;
> -  }
> -
> -  return JS::OrdinaryToPrimitive(cx, obj, type, vp);

It's okay to just remove workerdebuggersandbox_convert because the default implementation (without the hook) will do the same thing?

::: js/src/js.msg
@@ +53,5 @@
>  MSG_DEF(JSMSG_CANT_TRUNCATE_ARRAY,     0, JSEXN_TYPEERR, "can't delete non-configurable array element")
>  MSG_DEF(JSMSG_NOT_FUNCTION,            1, JSEXN_TYPEERR, "{0} is not a function")
>  MSG_DEF(JSMSG_NOT_CONSTRUCTOR,         1, JSEXN_TYPEERR, "{0} is not a constructor")
>  MSG_DEF(JSMSG_CANT_CONVERT_TO,         2, JSEXN_TYPEERR, "can't convert {0} to {1}")
> +MSG_DEF(JSMSG_TOPRIMITIVE_NOT_CALLABLE, 2, JSEXN_TYPEERR, "can't convert {0} to {1}: its [Symbol.toPrimtive] property is not a function")

Nit: toPrimitive, typo

::: js/src/jsapi.h
@@ +1888,5 @@
> + *
> + * Implements: ES6 7.1.1 ToPrimitive(input, [PreferredType]).
> + */
> +extern JS_PUBLIC_API(bool)
> +JS_DefaultValue(JSContext* cx, JS::HandleObject obj, JSType hint,

Shouldn't we rename JS_DefaultValue to JS::ToPrimitive, JS::ObjectToPrimitive or something similar?

::: js/src/tests/ecma_6/Object/toPrimitive.js
@@ +26,5 @@
> +// It is called even through proxies.
> +var proxy = new Proxy(obj, {});
> +expectedThis = proxy;
> +expectedHint = "default";
> +assertEq("ES" + proxy, "ES2015");

Heh.
Attachment #8658962 - Flags: review?(jdemooij) → review+
Attachment #8658963 - Flags: review?(jdemooij) → review+
Attachment #8658964 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #21)
> Do you think we could remove the JSFUN_STUB_GSOPS code from
> js::DefineFunction now, since we're handling that here now? Or are there
> other callers that rely on it?

Right, this does require an audit. I'll look at it soonish, since I want to take slightly more careful inventory of the things we want to delete from our codebase.

> Looks like we can remove JS_NewFunctionById now as this was the only caller.

Done!

> part 3 - Implement Symbol.toPrimitive. Replace existing convert hooks with methods
[...]
> > -  return JS::OrdinaryToPrimitive(cx, obj, type, vp);
> 
> It's okay to just remove workerdebuggersandbox_convert because the default
> implementation (without the hook) will do the same thing?

That's what it looks like to me. Who knows why this code was here. :-\

> > +extern JS_PUBLIC_API(bool)
> > +JS_DefaultValue(JSContext* cx, JS::HandleObject obj, JSType hint,
> 
> Shouldn't we rename JS_DefaultValue to JS::ToPrimitive,
> JS::ObjectToPrimitive or something similar?

Yeah, I think I'll be able to fix this in bug 1191570, but I haven't run all the tests yet.
> Yeah, I think I'll be able to fix this in bug 1191570, but I haven't run all the tests yet.

Sorry, that was incorrect. Filed follow-up bug 1206168 with patch for this suggestion.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc9fef646a97505196b39f3719b15157dba66af6
Bug 1054756, part 1 - Support symbol-keyed properties in JSXrayTraits::resolveOwnProperty(). r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c250abf4fd1778496b8b0e8b58ff8b6554fe9ba2
Bug 1054756, part 2 - Move ToPrimitive slow paths into a non-inline function. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/13128a88f2b91f31b6f79963768218c3997db41e
Bug 1054756, part 3 - Implement Symbol.toPrimitive. Replace existing convert hooks with methods. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/105433ce195b39f10f9f0b939c8786a0aff6a70f
Bug 1054756, part 4 - Remove BaseProxyHandler::defaultValue. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c93d1044b7e66f9c9c368ebfcd9c9da3d481081
Bug 1054756, part 5 - Remove Class::convert.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f55348a322863d8b66516e24cf7bbcb590e510e
Bug 1054756, part 1 - Support symbol-keyed properties in JSXrayTraits::resolveOwnProperty(). r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/88ed8edc7c6c2c9db428d8322168b036a218e160
Bug 1054756, part 2 - Move ToPrimitive slow paths into a non-inline function. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/00042f058f8b5a7832f2a759d98ea91af8f35437
Bug 1054756, part 3 - Implement Symbol.toPrimitive. Replace existing convert hooks with methods. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5b806d8a29cb304968ccac08d24ab3953b5a85
Bug 1054756, part 4 - Remove BaseProxyHandler::defaultValue. r=jandem.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7fa97ef8e68ec59ac7c4fd69735c5edbc0a9444
Bug 1054756, part 5 - Remove Class::convert.
Depends on: 1210570
Fantastic work on the documentation! Thanks :arai!
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: