918341 - new Map(iterable) should check that iterator values are objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andy Wingo [:wingo]]

From http://people.mozilla.org/~jorendorff/es6-draft.html#sec-23.1.1, when building a map from an iterable, we should throw if the values are not objects.

Comment 1

[Sankha Narayan Guria [:sankha]]

Attachment: patch v1

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 817639 [details] [diff] [review]
patch v1

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

Thanks! r=me with the comments addressed.

::: js/src/builtin/MapObject.cpp
@@ +1194,5 @@
>              if (!iter.next(&pairVal, &done))
>                  return false;
>              if (done)
>                  break;
> +            if (JSVAL_IS_PRIMITIVE(pairVal) || JSVAL_IS_NULL(pairVal)) {

Instead of this, please write:
    if (!pairVal.isObject()) {

@@ +1203,3 @@
>              pairObj = ToObject(cx, pairVal);
>              if (!pairObj)
>                  return false;

Please change this ToObject call to pairVal.toObject(), which is cheaper and can't fail. (This is possible because we just confirmed that pairVal really is an object.)

::: js/src/jit-test/tests/collections/Map-constructor-5.js
@@ +10,5 @@
> +
> +assertThrowsInstanceOf(function () { Map([1, 2, 3]); }, TypeError);
> +assertThrowsInstanceOf(function () {
> +	let s = new Set([1, 2, "abc"]);
> +	Map(s.values());

just `new Map(s);` please.  The values method is useless.

Comment 3

[Sankha Narayan Guria [:sankha]]

Attachment: patch v2

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae664f27e663

Comment 5

[Carsten Book [:Tomcat]]

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