1108930 - Fix in-tree consumers that call Map/Set/WeakMap constructors without "new"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

(similar to bug 980962)

Before fixing bug 1062075 and bug 1083752,
need to add "new" to all of those calls in tree.

Comment 1

[Tooru Fujisawa [:arai]]

running try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=08a0908f8706

Comment 2

[Tooru Fujisawa [:arai]]

filed bug 1108965 for gaia.

Comment 3

[Tooru Fujisawa [:arai]]

filed 1109453 for addon-sdk.

Comment 4

[Tooru Fujisawa [:arai]]

will work on this after bug 1114752 (addon sdk uplift) is fixed.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Call Map with new.

Prepared a patch (Part 7) to show warning when Map/Set/WeakMap are called without `new`, like other ES6 build-ins,
and before it, I'd like to fix in-tree consumers (Part 1-6).

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9197229f6060

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Call Set with new.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Call WeakMap with new.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Call Map with new in browser.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Call Map and Set with new in devtools.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Call WeakMap with new in xpconnect.

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 7: Warn when Map/Set/WeakMap are called without new.

Comment 12

[Tom S [:evilpie]]

Comment on attachment 8558566 [details] [diff] [review]
Part 1: Call Map with new.

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

::: js/src/jit-test/tests/collections/Map-constructor-generator-3.js
@@ +1,3 @@
>  // The argument to Map may be a generator-iterator that produces no values.
>  
> +assertEq(new Map(x for (x of [])).size, 0);

Can you write var m = new Map()
assert(..size, 0)

I find combining |new| with other expression confusing.

@@ +4,5 @@
>  
>  function none() {
>      if (0) yield 0;
>  }
> +assertEq(new Map(none()).size, 0);

And here.

::: js/src/jit-test/tests/collections/Set-iterator-proxies-2.js
@@ +12,5 @@
>  assertIteratorNext(iterator_fn.call(setw), "x");
>  
>  var next_fn = Set()[Symbol.iterator]().next;
>  assertThrowsInstanceOf(function () { next_fn.call({}); }, TypeError);
> +assertThrowsInstanceOf(function () { next_fn.call(new Map()[Symbol.iterator]()); }, TypeError);

(new Map())[Symbol..

::: js/src/jit-test/tests/collections/iterator-proto-2.js
@@ +2,5 @@
>  
>  load(libdir + "iteration.js");
>  
>  var aproto = Object.getPrototypeOf(Array()[Symbol.iterator]());
> +var mproto = Object.getPrototypeOf(new Map()[Symbol.iterator]());

(new Map())[Symbol...

Comment 13

[Tom S [:evilpie]]

Comment on attachment 8558567 [details] [diff] [review]
Part 2: Call Set with new.

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

::: js/src/jit-test/tests/collections/Map-iterator-proxies-2.js
@@ +13,5 @@
>  assertEqArray(iterator_fn.call(mapw).next().value, ["x", 1]);
>  
>  var next_fn = new Map()[Symbol.iterator]().next;
>  assertThrowsInstanceOf(function () { next_fn.call({}); }, TypeError);
> +assertThrowsInstanceOf(function () { next_fn.call(new Set()[Symbol.iterator]()); }, TypeError);

(new Set)

I just gave up on this point as most of the code already did something like new Map().size. Apparently just I don't like this style.

Comment 14

[Tom S [:evilpie]]

Comment on attachment 8558574 [details] [diff] [review]
Part 7: Warn when Map/Set/WeakMap are called without new.

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

You could actually write a test for the warnings by using options("werror")

::: js/src/jsweakmap.cpp
@@ +526,5 @@
>      RootedObject obj(cx, NewBuiltinClassInstance(cx, &WeakMapObject::class_));
>      if (!obj)
>          return false;
>  
> +    // ES6 23.3.1.1 steps 2-4.

Isn't this Step 1.? (in rev31)
If NewTarget is null, throw a TypeError exception.

Comment 15

[Tooru Fujisawa [:arai]]

Thank you all for reviewing :)

(In reply to Tom Schuster [:evilpie] from comment #14)
> Comment on attachment 8558574 [details] [diff] [review]
> Part 7: Warn when Map/Set/WeakMap are called without new.
> 
> Review of attachment 8558574 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You could actually write a test for the warnings by using options("werror")

JSMSG_BUILTIN_CTOR_NO_NEW is defined as JSEXN_NONE, and it cannot be caught. Is it okay to make it JSEXN_TYPEERR? (it's also used by ArrayBuffer and TypedArray)

> ::: js/src/jsweakmap.cpp
> @@ +526,5 @@
> >      RootedObject obj(cx, NewBuiltinClassInstance(cx, &WeakMapObject::class_));
> >      if (!obj)
> >          return false;
> >  
> > +    // ES6 23.3.1.1 steps 2-4.
> 
> Isn't this Step 1.? (in rev31)
> If NewTarget is null, throw a TypeError exception.

Yeah, I was reading old one, thank you for letting me know the change :D

Comment 16

[Tom S [:evilpie]]

Oh right, now I remember \o/. I wrote a patch for something else, and they way I did it was:

options("werrror");
assertEq(evaluate("WeakMap()", {catchTermination: true}), "terminated");

Comment 17

[Tooru Fujisawa [:arai]]

Thanks again :D
Added werror tests to Map/Set/WeakMap.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a24ebc65b965
https://hg.mozilla.org/integration/mozilla-inbound/rev/754f851ff0c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d0cca6c17e
https://hg.mozilla.org/integration/mozilla-inbound/rev/65c74fc6e769
https://hg.mozilla.org/integration/mozilla-inbound/rev/94276cdcc5ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/798dc45a5610
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c896a4f15ae

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a24ebc65b965
https://hg.mozilla.org/mozilla-central/rev/754f851ff0c8
https://hg.mozilla.org/mozilla-central/rev/e0d0cca6c17e
https://hg.mozilla.org/mozilla-central/rev/65c74fc6e769
https://hg.mozilla.org/mozilla-central/rev/94276cdcc5ff
https://hg.mozilla.org/mozilla-central/rev/798dc45a5610
https://hg.mozilla.org/mozilla-central/rev/2c896a4f15ae

Comment 19

[Kohei Yoshino]

Added to the compat doc:
https://developer.mozilla.org/en-US/Firefox/Releases/38/Site_Compatibility