Map.prototype.set, WeakMap.prototype.set and Set.prototype.add should be chainable

RESOLVED FIXED in mozilla33

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: caitpotter88, Assigned: spacetime, Mentored)

Tracking

({dev-doc-complete})

Trunk
mozilla33
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=c++][DocArea=JS])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.153 Safari/537.36

Steps to reproduce:

Trivial example: http://jsfiddle.net/n4p9s/

Just run the example!


Actual results:

We get a TypeError because the value returned from `(new Map()).set('foo', 'BAR')` is undefined.


Expected results:

Per https://people.mozilla.org/~jorendorff/es6-draft.html#sec-map.prototype.set, the 'set' should return `M` (this), and be chainable. So, the chained '.set()' call should return the Map object.

It's a minor inconvenience, not a huge deal, but you know.
(Reporter)

Comment 1

5 years ago
Updated test case: http://jsfiddle.net/e8mL2/

Also includes broken Set.prototype.add test, which should chain (per https://people.mozilla.org/~jorendorff/es6-draft.html#sec-set.prototype.entries)

This is also an issue in v8 (https://code.google.com/p/v8/issues/detail?id=3410)
(Reporter)

Updated

5 years ago
Summary: Map.prototype.set ( key , value ) should be chainable → Map.prototype.set ( key , value ) and Set.prototype.add should be chainable
Yes, needs a sprinkling of args.rval.set(args.thisv())
Status: UNCONFIRMED → NEW
Component: General → JavaScript: Standard Library
Ever confirmed: true
Pretty straight-forward to do. Good first bug because working on this means doing some very light C++ coding and adding some small JS tests, making for a nice first dive into SpiderMonkey.
Mentor: till
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [good first bug][c++]
Whiteboard: [good first bug][c++] → [good first bug][lang=c++]
Rishab, would you like to work on this?
Flags: needinfo?(ra.rishab)
(Assignee)

Comment 6

5 years ago
Yes, I'd like to work on this one.
Flags: needinfo?(ra.rishab)
Great! Do you need anything further, or are you good to go and create a patch?
(Assignee)

Comment 8

5 years ago
I'm going through the documentation. Just to confirm, my first steps would be to go set up everything using https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey and write a test?
Correct, that is a pretty good starting point.

If you haven't set up your environment at all, you have to check out the source as the very first step. Then, run `./mach mercurial-setup` from the checkout's root directory to set everything up in such a way that you can produce useful patches.

Feel free to ask as many questions as you need here or in the #jsapi channel on irc.mozilla.org.
(Assignee)

Comment 10

5 years ago
Some of the older tests failed since they asserted 'set' and 'add' return undefined. Removed assertions. Is there a better way? Also, could you help find a better fix for testCrossCompartmentTransparency.js?
Attachment #8449448 - Flags: review?(till)
Comment on attachment 8449448 [details] [diff] [review]
Return object for Map.set and Set.add; Added test; Fixed old tests

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

::: js/src/jit-test/tests/collections/Map-delete.js
@@ +7,5 @@
>  assertEq(m.delete(key), false);
>  assertEq(m.has(key), false);
>  
>  // when the key is present
> +m.set(key, 'x');

Why not do a somthing like:

    assertEq(m.set(key, 'x'), m);

That would test if we are returning the right value or not. Similarly for the other cases in rest of the files.
(Assignee)

Comment 12

5 years ago
Posted patch 1031632.patch (obsolete) — Splinter Review
Makes sense. Fixed in this patch.
Attachment #8449448 - Attachment is obsolete: true
Attachment #8449448 - Flags: review?(till)
Attachment #8449469 - Flags: review?(till)
Attachment #8449469 - Flags: feedback?(sankha93)
Assignee: nobody → ra.rishab
Comment on attachment 8449469 [details] [diff] [review]
1031632.patch

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

Yup, this is fine. I'm clearing the review flag instead of granting the review because I just realized that the same change should be done for WeakMap. Can you do that and add a test for it, too?

::: js/src/jit-test/tests/collections/bug-1031632.js
@@ +1,1 @@
> +// Bug 1031632 - Map.prototype.set ( key , value ) and Set.prototype.add

Please split this file into tow, Map-set-returns-this.js and Set-add-returns-this.js - it's nice to be able to roughly understand what a test does by just reading the filename. Keep the comment in both files, though.

@@ +1,4 @@
> +// Bug 1031632 - Map.prototype.set ( key , value ) and Set.prototype.add
> +// should be chainable
> +
> +load(libdir + "asserts.js");

You shouldn't need to load this - jit-tests only run in the shell, and that has assertEq built-in.

@@ +2,5 @@
> +// should be chainable
> +
> +load(libdir + "asserts.js");
> +
> +var a = (new Map()).set('foo', 'BAR').get('foo');

I know that more checks are contained in other test files, but please add basic tests for the return value:
var m = new Map();
assertEq(m.set('foo', 'BAR'), m);

and the same for Set (and WeakMap).
Attachment #8449469 - Flags: review?(till)
(Assignee)

Comment 14

5 years ago
Attachment #8449469 - Attachment is obsolete: true
Attachment #8449469 - Flags: feedback?(sankha93)
Attachment #8450270 - Flags: review?(till)
Comment on attachment 8450270 [details] [diff] [review]
Return object for Map.set, WeakMap.set and Set.add

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

Looks great. I commented on one issue that I fixed locally.

I also changed the commit message: it should contain the bug number and a description of what the patch does, not *why* it does it.

Pushed to try and will land it on inbound if everything is green:
https://tbpl.mozilla.org/?tree=Try&rev=de17213e5013

::: js/src/jsweakmap.cpp
@@ +419,5 @@
>      RootedValue value(cx, (args.length() > 1) ? args[1] : UndefinedValue());
>      Rooted<JSObject*> thisObj(cx, &args.thisv().toObject());
>      Rooted<WeakMapObject*> map(cx, &thisObj->as<WeakMapObject>());
>  
> +    args.rval().set(args.thisv());

This needs to be the last statement in the function, because otherwise you'll set the return value even if the call to SetWeakMapEntryInternal fails. It should be something like this:

if (!SetWeakMapEntryInternal(cx, map, key, value))
    return false;
args.rval().set(args.thisv());
return true;

I'll make that change locally, though, so no need to attach a new version.
Attachment #8450270 - Flags: review?(till) → review+
(Assignee)

Comment 16

5 years ago
I've incorporated the feedback from your last comment and removed an old regression test from Bug 697515 (https://bugzil.la/697515)

I also took the liberty to push it to try:
https://tbpl.mozilla.org/?tree=Try&rev=3ea713e6f6ac
Attachment #8450270 - Attachment is obsolete: true
Attachment #8450377 - Flags: review?(till)
Comment on attachment 8450377 [details] [diff] [review]
Return object for Map.set, WeakMap.set and Set.add; Removed old regression test

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

Yup, still fine. :)

Just for next time: an r+ means that you don't have to ask for a review again. Also, with a fix this small, a second try run isn't required. It doesn't hurt, but then again our infrastructure isn't exactly under-utilized ;)

Will land once the try run has finished.
Attachment #8450377 - Flags: review?(till) → review+
(Assignee)

Comment 18

5 years ago
(In reply to Till Schneidereit [:till] from comment #17)
> Comment on attachment 8450377 [details] [diff] [review]
> Return object for Map.set, WeakMap.set and Set.add; Removed old regression
> test
> 
> Review of attachment 8450377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yup, still fine. :)
> 
> Just for next time: an r+ means that you don't have to ask for a review
> again. Also, with a fix this small, a second try run isn't required. It
> doesn't hurt, but then again our infrastructure isn't exactly under-utilized
> ;)
> 
> Will land once the try run has finished.

Got it :) Thanks!
(Reporter)

Comment 19

5 years ago
☆.。.:・゜☆ Good job :spacetime! ☆.。.:・゜☆
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aabe2de625c6

Congratulations, Rishab!

Do you know what you'd like to look into next? Ping me on IRC if you want suggestions or feedback.
Status: NEW → ASSIGNED
Summary: Map.prototype.set ( key , value ) and Set.prototype.add should be chainable → Map.prototype.set, WeakMap.prototype.set and Set.prototype.add should be chainable
(Assignee)

Comment 21

5 years ago
Thank you, Caitlin!

Thank you, Till! I'll ping you on IRC.
https://hg.mozilla.org/mozilla-central/rev/aabe2de625c6
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Keywords: dev-doc-needed
Whiteboard: [good first bug][lang=c++] → [good first bug][lang=c++][DocArea=JS]
You need to log in before you can comment on or make changes to this bug.