Closed Bug 885788 Opened 11 years ago Closed 11 years ago

Implement ES6 Object.setPrototypeOf

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

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)

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.
We should implement this. It seems easy enough and v8 just landed it: http://code.google.com/p/v8/source/detail?r=18685.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #8364329 - Flags: review?(jorendorff)
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)
(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 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;
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)
(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.
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8364329 - Attachment is obsolete: true
Attachment #8365150 - Flags: review?(jorendorff)
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+
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.
Attached patch patch v3 (obsolete) — Splinter Review
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)
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 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+
Attached patch patch v4Splinter Review
Carrying over r+ from previous comments.
Assignee: general → sankha93
Attachment #8365535 - Attachment is obsolete: true
Attachment #8379747 - Flags: review+
Keywords: checkin-needed
Keywords: feature
Looks like the asserts were only happening in the rootanalysis builds.
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Tagging as [qa-] since this has in-testsuite coverage. Please needinfo me if this needs QA attention before we release.
Whiteboard: [qa-]
Depends on: 992566
ES6's Object.setPrototypeOf is probably worth relnoting.
relnote-firefox: --- → ?
Thanks. Added in the release notes for 31. I will add a link to the documentation once it is available.
(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
Thanks. Release note updated.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: