If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement ES6 Object.setPrototypeOf

RESOLVED FIXED in mozilla31

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: benvie, Assigned: sankha)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete, feature})

Trunk
mozilla31
dev-doc-complete, feature
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 31+)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Duplicate of this bug: 872453
Keywords: dev-doc-needed
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

4 years ago
Created attachment 8364329 [details] [diff] [review]
patch v1
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)
(Assignee)

Comment 5

4 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 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.
(Assignee)

Comment 9

4 years ago
Created attachment 8365150 [details] [diff] [review]
patch v2
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.
(Assignee)

Comment 12

4 years ago
Created attachment 8365535 [details] [diff] [review]
patch v3

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

4 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 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

4 years ago
Created attachment 8379747 [details] [diff] [review]
patch v4

Carrying over r+ from previous comments.
Assignee: general → sankha93
Attachment #8365535 - Attachment is obsolete: true
Attachment #8379747 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/00d8b2e04b15
Flags: in-testsuite+
Keywords: checkin-needed
Keywords: feature
Backed out for jit-test asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab170f0289da

https://tbpl.mozilla.org/php/getParsedLog.php?id=35053719&tree=Mozilla-Inbound
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
https://hg.mozilla.org/mozilla-central/rev/893100c94698
Status: NEW → RESOLVED
Last Resolved: 4 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-]

Updated

4 years ago
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.
relnote-firefox: ? → 31+
(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
Thanks. Release note updated.
You need to log in before you can comment on or make changes to this bug.