Handle optional iterable argument in WeakMap constructor

RESOLVED FIXED in mozilla36



5 years ago
4 years ago


(Reporter: anba, Assigned: arai)



Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



5 years ago
Same issue as in bug 726223, only this time for WeakMaps.

Simple test case:
key = {};
wm = new WeakMap([[key, 123]]);
value = wm.get(key);
assertEq(value, 123);

Comment 1

5 years ago
Implemented ES6 steps 5-7 and 11-12, except `null` in step 6,

Also, added almost same testcases as Map-constructor-*.
There are so much differences between them, so I added separetely.

Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7efe84f31791

For steps 2-4 and 8-9, I leave it to bug 1062075 and bug 804279,
and `null` in step 6 to bug 1092538.
(Both could cause compatibility problem, more likely than this patch)

Then, in step 7a, it gets "set" property of this-value, to call it while iterating,
it means that if we alter `WeakMap.prototype.set` to another function,
WeakMap constructor should call it, right? (what jit-test/tests/collections/WeakMap-constructor-set.js tests)
Similar thing is noted in bug 804279, for generic.
If this is true for non-generic call, other collections's constructor should also be fixed.
If not, i'll remove that part from this patch, and leave it to bug 804279.
Attachment #8516452 - Flags: review?(jorendorff)

Comment 2

5 years ago
Sorry, I overlooked second example in bug 804279 comment #0.
It's just what I asked about step 7a in comment #1.
I would propose that we should just implement this like all the other Map/Set/WeakSet constructors right now and totally ignore calling any method instead of just doing JS::SetWeakMapEntry. I don't see any fault however with handling null/undefined correctly from the start.

Comment 4

5 years ago
Okay, thank you for suggestion.
I'll remove step7a things (and any other differences between other containers) for now.
Then try to fix bug 1092538 as a next step.

Comment 5

5 years ago
Comment on attachment 8516452 [details] [diff] [review]
Handle optional iterable argument in WeakMap constructor.

Clearing r? for now.
I'll post updated patch shortly.
Attachment #8516452 - Flags: review?(jorendorff)

Comment 6

5 years ago
Removed step7a, will be fixed as bug 804279.
Also, use SetWeakMapEntry instead of SetWeakMapEntryInternal as WeakSet does.

Green on try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f1fdfb5b74c0
Attachment #8516452 - Attachment is obsolete: true
Attachment #8521385 - Flags: review?(jorendorff)
Comment on attachment 8521385 [details] [diff] [review]
Handle optional iterable argument in WeakMap constructor.

Review of attachment 8521385 [details] [diff] [review]:

Looks really good.

::: js/src/builtin/MapObject.cpp
@@ +1250,5 @@
>              if (done)
>                  break;
>              if (!pairVal.isObject()) {
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr,
> +                                     JSMSG_INVALID_MAP_ITERABLE, "map");


::: js/src/jsweakmap.cpp
@@ +548,5 @@
> +
> +            // Step 12f.
> +            if (!pairVal.isObject()) {
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr,
> +                                     JSMSG_INVALID_MAP_ITERABLE, "weakmap");

Attachment #8521385 - Flags: review?(jorendorff) → review?(evilpies)
Attachment #8521385 - Flags: review?(evilpies) → review+
Thanks arai, nice work! I landed this for you, here:

Comment 9

5 years ago
Thank you!
Assignee: nobody → arai_a
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.