Calling Map etc without new should throw TypeError

Calling Map etc without new should throw TypeError

Even though we do not yet allow Map, Set, WeakMap and WeakSet to be
subclassed we need to ensure that we do not allow them to be [[Call]]ed
to allow them to be subclassed in the future.

BUG=v8:2819
R=dslomov@chromium.org, mstarzinger@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=16006

[Dmitry Lomov (no reviews), 2013-08-01 08:06:40]

lgtm

[Michael Starzinger, 2013-08-01 08:06:52]

IIUC the Map method should be callable (even as non-constructor) if the receiver
object has a Map instance type. To be more precise, the following should not
throw.

Map.call(%NewObject(Map));

[Dmitry Lomov (no reviews), 2013-08-01 08:19:40]

On 2013/08/01 08:06:52, Michael Starzinger wrote:
> IIUC the Map method should be callable (even as non-constructor) if the
receiver
> object has a Map instance type. To be more precise, the following should not
> throw.
> 
> Map.call(%NewObject(Map));

Yes this is needed for proper subclassing, but I would leave that for another
CL. This Cl makes us consistent with what we do for other constructors
(ArrayBuffers, typed arrays).
To be even more precise, the above should not throw for _uninitialized_ object
with Map instance type, and that is a bit harder to check.

[Michael Starzinger, 2013-08-01 08:32:09]

OK, as discussed offline with Dmitry: I can be convinced to land this change and
leave the "actual fix" up to a follow-up CL. But just so that we are on the same
side here: With this CL we are going from the current wrong behavior to another
(new) wrong behavior. Judging whether the new wrong behavior is better or worse
than the old is outside the scope of my comment.

LGTM (in light that Dmitry agreed to work on the "actual fix" as a follow-up).

[Dmitry Lomov (no reviews), 2013-08-01 09:12:03]

Discussed offline, but putting this here for the record: the new behavior is
better because it is future-proof - the user cannot write code that will break.

[Dmitry Lomov (no reviews), 2013-08-01 09:18:34]

Committed patchset #1 manually as r16006 (presubmit successful).

[danno, 2013-08-01 16:17:13]

NOT lgtm. Why the change for an experimental feature that we don't make any
promises about? This is needless code churn that I see no benefit from. I am
very tempted to roll this back... and in the future please don't don't make
changes to experimental features on purpose that change one incorrect behavior
to another. If you want to change the semantic, please fix it and make it spec
conform.

[danno, 2013-08-02 08:12:47]

OK, sorry, my bad. This was clarified in an offline thread, the context of the
issues wasn't initially clear to me. Lgtm, too. :-)