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

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Caitlin Potter [:caitp]]

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.

Comment 1

[Caitlin Potter [:caitp]]

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)

Comment 2

[Caitlin Potter [:caitp]]

Er, https://people.mozilla.org/~jorendorff/es6-draft.html#sec-set.prototype.add =)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yes, needs a sprinkling of args.rval.set(args.thisv())

Comment 4

[Till Schneidereit [:till]]

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.

Comment 5

[Sankha Narayan Guria [:sankha]]

Rishab, would you like to work on this?

Comment 6

[Rishab Arora [:spacetime]]

Yes, I'd like to work on this one.

Comment 7

[Till Schneidereit [:till]]

Great! Do you need anything further, or are you good to go and create a patch?

Comment 8

[Rishab Arora [:spacetime]]

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?

Comment 9

[Till Schneidereit [:till]]

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.

Comment 10

[Rishab Arora [:spacetime]]

Attachment: Return object for Map.set and Set.add; Added test; Fixed old tests

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?

Comment 11

[Sankha Narayan Guria [:sankha]]

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.

Comment 12

[Rishab Arora [:spacetime]]

Attachment: 1031632.patch

Makes sense. Fixed in this patch.

Comment 13

[Till Schneidereit [:till]]

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).

Comment 14

[Rishab Arora [:spacetime]]

Attachment: Return object for Map.set, WeakMap.set and Set.add

Comment 15

[Till Schneidereit [: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.

Comment 16

[Rishab Arora [:spacetime]]

Attachment: Return object for Map.set, WeakMap.set and Set.add; Removed old regression test

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

Comment 17

[Till Schneidereit [: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.

Comment 18

[Rishab Arora [:spacetime]]

(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!

Comment 19

[Caitlin Potter [:caitp]]

☆.。.：・゜☆ Good job :spacetime! ☆.。.：・゜☆

Comment 20

[Till Schneidereit [:till]]

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.

Comment 21

[Rishab Arora [:spacetime]]

Thank you, Caitlin!

Thank you, Till! I'll ping you on IRC.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/aabe2de625c6

Comment 23

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/33#JavaScript

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/set
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap/set
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set/add