Closed
Bug 885788
Opened 11 years ago
Closed 11 years ago
Implement ES6 Object.setPrototypeOf
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla31
Tracking | Status | |
---|---|---|
relnote-firefox | --- | 31+ |
People
(Reporter: bbenvie, Assigned: sankha)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: dev-doc-complete, feature, Whiteboard: [qa-])
Attachments
(1 file, 3 obsolete files)
6.91 KB,
patch
|
sankha
:
review+
|
Details | Diff | Splinter Review |
Signature: Object.setPrototypeOf(O, proto)
This is pretty much identical to the `Object.prototype.__proto__` setter.
See ES6 spec (May 2013 draft) section 15.2.3.2.
Updated•11 years ago
|
Keywords: dev-doc-needed
Comment 2•11 years ago
|
||
We should implement this. It seems easy enough and v8 just landed it: http://code.google.com/p/v8/source/detail?r=18685.
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8364329 -
Flags: review?(jorendorff)
Comment 4•11 years ago
|
||
Comment on attachment 8364329 [details] [diff] [review]
patch v1
Review of attachment 8364329 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/builtin/Object.cpp
@@ +545,5 @@
> + JSObject::isExtensible(cx, obj, &extensible);
> + if (!extensible) {
> + JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_OBJECT_NOT_EXTENSIBLE, "object");
> + return false;
> + }
Hmm. I don't think it's necessary to check isExtensible here. Doesn't JSObject::setProto do this for us?
@@ +554,5 @@
> +
> + if (!success) {
> + args.rval().setUndefined();
> + return false;
> + }
The spec says, "7. If status is false, then throw a TypeError exception." Returning undefined would definitely be a bug.
I think this pretty much only happens if the object is inextensible, so just move the error-reporting code you added above down here.
::: js/src/jit-test/tests/basic/setPrototypeOf.js
@@ +1,4 @@
> +load(libdir + 'asserts.js');
> +
> +function getObjects() {
> + function func(){};
Nit: no semicolon here.
@@ +9,5 @@
> + [1, 2, 3],
> + new Date(),
> + new Number(1),
> + new Boolean(true),
> + new String('str')];
Throw Object.create(null) in here, please.
@@ +28,5 @@
> +}
> +
> +// check if Object.setPrototypeOf works with coercible values
> +for(var i = 0; i < coercibleValues.length; i++) {
> + var value = coercibleValues[i];
You can write:
for (var value of coercibleValues) {
@@ +31,5 @@
> +for(var i = 0; i < coercibleValues.length; i++) {
> + var value = coercibleValues[i];
> + assertThrowsInstanceOf(function() {
> + Object.getPrototypeOf(value);
> + }, TypeError, "");
This is redundant -- you assert the same thing a few lines below.
@@ +53,5 @@
> +var objects = getObjects();
> +for (var i = 0; i < objects.length; i++) {
> + var object = objects[i];
> + for (var j = 0; j < valuesWithoutNull.length; j++) {
> + var proto = valuesWithoutNull[j];
for (var object of getObjects()) {
for (var proto of valuesWithoutNull) {
....
Attachment #8364329 -
Flags: review?(jorendorff)
Assignee | ||
Comment 5•11 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #4)
> Hmm. I don't think it's necessary to check isExtensible here. Doesn't
> JSObject::setProto do this for us?
setProto checks if the object is extensible or not but doesn't throw a TypeError exception (which I think is correct according to the spec's [[SetPrototypeOf]] (V) section.
But V8's implementation checks for a TypeError when trying to use Object.setPrototypeOf on inextensible object. Is that correct?
Flags: needinfo?(jorendorff)
Comment 6•11 years ago
|
||
Comment on attachment 8364329 [details] [diff] [review]
patch v1
Review of attachment 8364329 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/builtin/Object.cpp
@@ +526,5 @@
> +static bool
> +obj_setPrototypeOf(JSContext *cx, unsigned argc, Value *vp) {
> + CallArgs args = CallArgsFromVp(argc, vp);
> +
> + if (args[0].isNullOrUndefined()) {
You can't access these without making sure that args.length() >= 2.
@@ +537,5 @@
> + JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_NOT_AN_OBJECT);
> + return false;
> + }
> +
> + RootedObject obj(cx, ToObject(cx, args[0]));
The spec says something about:
4. If Type(O) is not Object, then return O.
In any case this is missing a null check.
You should really use the usual // Step annotation.
@@ +541,5 @@
> + RootedObject obj(cx, ToObject(cx, args[0]));
> + RootedObject newProto(cx, args[1].toObjectOrNull());
> +
> + bool extensible;
> + JSObject::isExtensible(cx, obj, &extensible);
if (!JSObject::isExtensible(cx, obj, &extensible))
return false;
Comment 7•11 years ago
|
||
Sankha, the first bug evilpie noted is testable, so please add tests:
assertThrowsInstanceOf(() => Object.setPrototypeOf(), TypeError);
assertThrowsInstanceOf(() => Object.setPrototypeOf({}), TypeError);
(In reply to Tom Schuster [:evilpie] from comment #6)
> > + RootedObject obj(cx, ToObject(cx, args[0]));
>
> The spec says something about:
> 4. If Type(O) is not Object, then return O.
>
> In any case this is missing a null check.
Right. This is an OOM case so it's not easily testable.
Flags: needinfo?(jorendorff)
Comment 8•11 years ago
|
||
(In reply to Sankha Narayan Guria [:sankha93] from comment #5)
> (In reply to Jason Orendorff [:jorendorff] from comment #4)
> > Hmm. I don't think it's necessary to check isExtensible here. Doesn't
> > JSObject::setProto do this for us?
>
> setProto checks if the object is extensible or not but doesn't throw a
> TypeError exception (which I think is correct according to the spec's
> [[SetPrototypeOf]] (V) section.
Yes, that's correct. setProto will set succeeded to false. This is equivalent to [[SetPrototypeOf]] returning false in step 5.
According to the specification, we must check in step 7 whether this happened, and if it did, throw a TypeError.
> But V8's implementation checks for a TypeError when trying to use
> Object.setPrototypeOf on inextensible object. Is that correct?
I'm sure what V8 is doing is right, but let's talk about testable behavior and the specification, not V8.
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #8364329 -
Attachment is obsolete: true
Attachment #8365150 -
Flags: review?(jorendorff)
Comment 10•11 years ago
|
||
Comment on attachment 8365150 [details] [diff] [review]
patch v2
Review of attachment 8365150 [details] [diff] [review]:
-----------------------------------------------------------------
Nice work. The "Step" comments really do help make the code more obviously correct.
r=me with the following changes.
::: js/src/builtin/Object.cpp
@@ +528,5 @@
> + CallArgs args = CallArgsFromVp(argc, vp);
> +
> + if (args.length() < 2) {
> + JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_MORE_ARGS_NEEDED,
> + "Object.setPrototypeOf", "1", "s");
Please change "s" to the empty string "".
This is because with the "s", the message would be "requires more than 1 arguments" which is ungrammatical in English. (Don't ask me to explain why "more than 1" requires a singular noun. It's magic.)
@@ +552,5 @@
> + return true;
> + }
> +
> + /* Step 5-6. */
> + RootedObject obj(cx, ToObject(cx, args[0]));
Since step 4 established that args[0].isObject(), please just do &args[0].toObject() here.
JS::ToObject() is more complex; it handles primitive values (converting them to wrapper objects) and it can fail by running out of memory, which args[0].toObject() can't.
::: js/src/js.msg
@@ +223,5 @@
> MSG_DEF(JSMSG_NEGATIVE_REPETITION_COUNT, 170, 0, JSEXN_RANGEERR, "repeat count must be non-negative")
> MSG_DEF(JSMSG_INVALID_FOR_OF_INIT, 171, 0, JSEXN_SYNTAXERR, "for-of loop variable declaration may not have an initializer")
> MSG_DEF(JSMSG_INVALID_MAP_ITERABLE, 172, 0, JSEXN_TYPEERR, "iterable for map should have array-like objects")
> MSG_DEF(JSMSG_NOT_A_CODEPOINT, 173, 1, JSEXN_RANGEERR, "{0} is not a valid code point")
> +MSG_DEF(JSMSG_NOT_AN_OBJECT, 174, 0, JSEXN_TYPEERR, "Object prototype may only be an Object or null")
Instead of adding a new error message, please use JSMSG_NOT_EXPECTED_TYPE.
That error message takes 3 arguments:
"Object.setPrototypeOf",
"an object or null",
InformalValueTypeName(args[1])
Attachment #8365150 -
Flags: review?(jorendorff) → review+
Comment 11•11 years ago
|
||
Review commentary:
<jwalden> jorendorff: for the Object.setPrototypeOf thing, we should make that code call GlobalObject::warnOnceAboutPrototypeMutation
<jwalden> jorendorff: which currently doesn't warn at all, but will once I land a patch in the next few days
<jorendorff> jwalden: or make setProto do so
<jwalden> jorendorff: we use setProto in Gecko for dumbness, and we use it inside the engine, I think, as well, for { __proto__: null }
<jwalden> jorendorff: so I think we can't do it there just yet
<jorendorff> mmkay
<jorendorff> sankha93: jwalden has a review comment for you!
*jwalden would kind of like to make { __proto__: ... } warn, but thinks it probably should be treated differently, at least initially, because there's much less use of the actual setter than of the name in object literals
<jwalden> :-)
I'll be flipping that method to actually warn in a very few days, in bug 948227, after I land the second half of bug 948583. That's delayed while I fix regressions from the first half of that bug, but that should happen Really Soon, and definitely before uplift. So for now just add the
RootedObject setPrototypeOf(cx, &args.callee());
if (!GlobalObject::warnOnceAboutPrototypeMutation(cx, setPrototypeOf))
return false;
to the method and don't worry that it doesn't actually perform a warning.
Assignee | ||
Comment 12•11 years ago
|
||
fine?Addressed Jason's comments in this. Jeff can you see if the changes you suggested is fine?
Attachment #8365150 -
Attachment is obsolete: true
Attachment #8365535 -
Flags: feedback?(jwalden+bmo)
Assignee | ||
Comment 13•11 years ago
|
||
Oops! The text got scrambled. It should have been:
Addressed Jason's comments in this. Jeff can you see if the change you suggested is fine?
Comment 14•11 years ago
|
||
Comment on attachment 8365535 [details] [diff] [review]
patch v3
Review of attachment 8365535 [details] [diff] [review]:
-----------------------------------------------------------------
Vague skimming, didn't really look closely at behavior.
::: js/src/builtin/Object.cpp
@@ +523,5 @@
> return true;
> }
>
> +static bool
> +obj_setPrototypeOf(JSContext *cx, unsigned argc, Value *vp) {
{ on new line
@@ +554,5 @@
> + }
> +
> + RootedObject setPrototypeOf(cx, &args.callee());
> + if (!GlobalObject::warnOnceAboutPrototypeMutation(cx, setPrototypeOf))
> + return false;
Please move this all the way up to the top, just after the |CallArgs args| bit. We want to warn regardless whether any intermediate error-tests cause execution to halt, because such behavior likely indicates a bug in the user's code.
Attachment #8365535 -
Flags: feedback?(jwalden+bmo) → feedback+
Assignee | ||
Comment 15•11 years ago
|
||
Carrying over r+ from previous comments.
Assignee: general → sankha93
Attachment #8365535 -
Attachment is obsolete: true
Attachment #8379747 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 16•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 17•11 years ago
|
||
Comment 18•11 years ago
|
||
Looks like the asserts were only happening in the rootanalysis builds.
Comment 19•11 years ago
|
||
Oh sorry I just pushed, this because I confused the normal SM builds with the hazards builds. Maybe we are lucky.
https://hg.mozilla.org/integration/mozilla-inbound/rev/893100c94698
Comment 20•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 21•11 years ago
|
||
Tagging as [qa-] since this has in-testsuite coverage. Please needinfo me if this needs QA attention before we release.
Whiteboard: [qa-]
Comment 22•11 years ago
|
||
ES6's Object.setPrototypeOf is probably worth relnoting.
relnote-firefox:
--- → ?
Comment 23•11 years ago
|
||
Thanks. Added in the release notes for 31.
I will add a link to the documentation once it is available.
Comment 24•11 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #23)
> Thanks. Added in the release notes for 31.
> I will add a link to the documentation once it is available.
Reference:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/setPrototypeOf
Firefox 31 for developers:
https://developer.mozilla.org/en-US/Firefox/Releases/31#JavaScript
Keywords: dev-doc-needed → dev-doc-complete
Comment 25•11 years ago
|
||
Thanks. Release note updated.
You need to log in
before you can comment on or make changes to this bug.
Description
•