Closed Bug 1108930 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

(Keywords: dev-doc-complete)

Attachments

(7 files)

(similar to bug 980962)

Before fixing bug 1062075 and bug 1083752,
need to add "new" to all of those calls in tree.
Blocks: 1062075, 1083752
filed bug 1108965 for gaia.
filed 1109453 for addon-sdk.
will work on this after bug 1114752 (addon sdk uplift) is fixed.
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
Attachment #8558566 - Flags: review?(evilpies)
Attachment #8558567 - Flags: review?(evilpies)
Attachment #8558568 - Flags: review?(evilpies)
Attachment #8558570 - Flags: review?(felipc)
Attachment #8558572 - Flags: review?(nfitzgerald)
Attachment #8558570 - Flags: review?(felipc) → review+
Attachment #8558572 - Flags: review?(nfitzgerald) → review+
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...
Attachment #8558566 - Flags: review?(evilpies) → review+
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.
Attachment #8558567 - Flags: review?(evilpies) → review+
Attachment #8558568 - Flags: review?(evilpies) → review+
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.
Attachment #8558574 - Flags: review?(evilpies) → review+
Attachment #8558573 - Flags: review?(mrbkap) → review+
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
Flags: needinfo?(evilpies)
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");
Flags: needinfo?(evilpies)
Blocks: 1129605
Blocks: 1129609
You need to log in before you can comment on or make changes to this bug.