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

RESOLVED FIXED in mozilla27

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: wingo, Assigned: sankha)

Tracking

unspecified
mozilla27
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Updated

4 years ago
Blocks: 918349
(Assignee)

Comment 1

4 years ago
Created attachment 817639 [details] [diff] [review]
patch v1
Attachment #817639 - Flags: review?(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.
Attachment #817639 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 3

4 years ago
Created attachment 818282 [details] [diff] [review]
patch v2
Assignee: general → sankha93
Attachment #817639 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae664f27e663
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ae664f27e663
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.