Don't exchange null and undefined with the global object in function.prototype.{call, apply} for ...

Don't exchange null and undefined with the global object in function.prototype.{call, apply} for natives.

This makes us compatible with firefox in throwing an exception when
call is invoked on a builtin with null as the this argument.
Committed: http://code.google.com/p/v8/source/detail?r=7763

[Rico, 2011-04-28 05:57:59]

http://codereview.chromium.org/6902104/diff/1/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

http://codereview.chromium.org/6902104/diff/1/src/ia32/builtins-ia32.cc#newco...
src/ia32/builtins-ia32.cc:608: __ cmp(ebx, Script::TYPE_NATIVE);
We could just test ebx without untagging here, since Script::TYPE_NATIVE is zero
(and we rely on this elsewhere in non code-generating code, like messages.js).

[Lasse Reichstein, 2011-04-28 10:44:38]

LGTM.

http://codereview.chromium.org/6902104/diff/3001/src/array.js
File src/array.js (right):

http://codereview.chromium.org/6902104/diff/3001/src/array.js#newcode378
src/array.js:378: if (IS_NULL_OR_UNDEFINED(this)) {
Is this correct behavior for an undetectable object?
(Can we create an undetectable string and/or object for testing?)

http://codereview.chromium.org/6902104/diff/3001/src/array.js#newcode379
src/array.js:379: throw MakeTypeError("obj_ctor_property_non_object",
["Array.join"]);
The name shouldn't be "Array.prototype.join"?

http://codereview.chromium.org/6902104/diff/3001/src/array.js#newcode995
src/array.js:995: f.call(receiver, current, i, this);
While you are here, make this use %_CallFunction (and similar for the other that
calls).
You may have to handle the receiver being null/undefined too :)

http://codereview.chromium.org/6902104/diff/3001/src/string.js
File src/string.js (right):

http://codereview.chromium.org/6902104/diff/3001/src/string.js#newcode66
src/string.js:66: throw MakeTypeError("obj_ctor_property_non_object",
["charAt"]);
Why "Array.some" but not "String.charAt"?

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.js
File test/mjsunit/function-call.js (right):

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:273: should_throw_on_null_and_undefined[i]);
Indentation (more cases below).

[Lasse Reichstein, 2011-04-28 10:50:01]

http://codereview.chromium.org/6902104/diff/3001/src/array.js
File src/array.js (right):

http://codereview.chromium.org/6902104/diff/3001/src/array.js#newcode378
src/array.js:378: if (IS_NULL_OR_UNDEFINED(this)) {
Just checked Firefox 4. It does not throw when passing document.all (an
undetectable object) as receiver.

[MarkM, 2011-04-28 15:40:45]

http://codereview.chromium.org/6902104/diff/3001/src/array.js
File src/array.js (right):

http://codereview.chromium.org/6902104/diff/3001/src/array.js#newcode378
src/array.js:378: if (IS_NULL_OR_UNDEFINED(this)) {
On 2011/04/28 10:50:01, Lasse Reichstein wrote:
> Just checked Firefox 4. It does not throw when passing document.all (an
> undetectable object) as receiver.

Do we have any undetectable objects other that document.all?

Firefox's strategy for handling document.all differs from WebKits. WebKit treats
the value as a special kind of value, whereas FF treats the lookup of the "all"
property of document as a special context sensitive kind of lookup. The lookup
is sensitive to the syntactic context from which it is looked up, but once
looked up yields either a normal value or a normal undefined. AFAIK, FF does not
have special undetectable objects as first class values.

See https://bugzilla.mozilla.org/show_bug.cgi?id=521670#c6

[Lasse Reichstein, 2011-04-28 16:41:11]

Ah, yes. I also tested a Webkit Nightly, and they also don't throw on
document.all:
 
 var o = document.all;
 alert(typeof o);  // undefined, it's still undetectable
 Array.prototype.pop.call(o)

[Lasse Reichstein, 2011-04-28 16:44:02]

But then, it doesn't throw on "undefined" either.

[MarkM, 2011-04-28 18:58:21]

http://codereview.chromium.org/6902104/diff/3001/src/array.js
File src/array.js (right):

http://codereview.chromium.org/6902104/diff/3001/src/array.js#newcode379
src/array.js:379: throw MakeTypeError("obj_ctor_property_non_object",
["Array.join"]);
For all these generics, I wonder about the string "obj_ctor_property_non_object"
as not reflecting what the actual error is. After all

    > typeof [].map.call("foo", function(c,i,a) { return a; })[0]
    object


already works, as it should, by coercing its this to an object as specified by
ToObject. This shows that the problem isn't that the this is a "non_object" but
that it's null or undefined. (Or, given document.all, perhaps "indistinguishable
from null or undefined")

[Lasse Reichstein, 2011-04-28 19:13:19]

Since Safari isn't already doing something, we are free to do what we want with
document.all.
I think the right thing is to treat it as an object, since it's used in an
object context, not a test context. I.e., we should not throw on an undetectable
object/string as receiver (we should treat it as a valid argument to ToObject).

[MarkM, 2011-04-28 21:16:54]

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.js
File test/mjsunit/function-call.js (right):

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:35: // Array.prototype.toString,
Why are these commented out?

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:160: assertTrue(/called on non-object/.test(e) ||
/Cannot convert/.test(e));
If we changed the test here to

    assertTrue(e instanceof TypeError);

as it is for the non-generics below, then we could treat this as a normative
test eventually to be migrated into Sputnik and/or test262. OTOH, I can see why
you'd want to test that you're getting the diagnostic you expect.

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:164:
should_throw_on_null_and_undefined[i].call(undefined);
If should_throw_on_null_and_undefined[i] happens to be a function that expects
more arguments and throws if these are absent, then calling it without the rest
of its expected arguments is not a reliable test that you're getting the error
for the right reason.

[Lasse Reichstein, 2011-04-29 06:16:12]

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.js
File test/mjsunit/function-call.js (right):

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:80: [Function.prototype.toString,
How about Function.prototype.call and Function.prototype.apply? :)

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:149: Array.prototype.reduceRight];
Try testing String.prototype.replace the same way. Let's, for now, assume that
we should be passing undefined as receiver (I'll ask es5-discuss).

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:160: assertTrue(/called on non-object/.test(e) ||
/Cannot convert/.test(e));
It helps if the same algorithm can throw more than one TypeError. In that case,
we should check that it's the first test in the ES algorithm to fail that is
actually thrown.

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:164:
should_throw_on_null_and_undefined[i].call(undefined);
A comment should suffice. The general rule, which applies to all of these
functions, is that supplying too few arguments means that the remaining
parameters will take the undefined value. All of the functions here will test
their receiver for being convertible to object as the first thing, or have it as
a precondition for the non-generic methods.

Having meaningless and unusable arguments that would by themselves make the
method fail, is actually a good thing, since it also tests that the
object-coercible test is performed first.

[Rico, 2011-05-03 08:47:20]

http://codereview.chromium.org/6902104/diff/3001/src/array.js
File src/array.js (right):

http://codereview.chromium.org/6902104/diff/3001/src/array.js#newcode378
src/array.js:378: if (IS_NULL_OR_UNDEFINED(this)) {
On 2011/04/28 15:40:45, MarkM wrote:
> On 2011/04/28 10:50:01, Lasse Reichstein wrote:
> > Just checked Firefox 4. It does not throw when passing document.all (an
> > undetectable object) as receiver.
> 
> Do we have any undetectable objects other that document.all?
> 
> Firefox's strategy for handling document.all differs from WebKits. WebKit
treats
> the value as a special kind of value, whereas FF treats the lookup of the
"all"
> property of document as a special context sensitive kind of lookup. The lookup
> is sensitive to the syntactic context from which it is looked up, but once
> looked up yields either a normal value or a normal undefined. AFAIK, FF does
not
> have special undetectable objects as first class values.
> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=521670#c6
Added checks for the undetectable object in all cases

http://codereview.chromium.org/6902104/diff/3001/src/array.js#newcode379
src/array.js:379: throw MakeTypeError("obj_ctor_property_non_object",
["Array.join"]);
On 2011/04/28 10:44:38, Lasse Reichstein wrote:
> The name shouldn't be "Array.prototype.join"?
Changed

http://codereview.chromium.org/6902104/diff/3001/src/array.js#newcode379
src/array.js:379: throw MakeTypeError("obj_ctor_property_non_object",
["Array.join"]);
On 2011/04/28 18:58:21, MarkM wrote:
> For all these generics, I wonder about the string
"obj_ctor_property_non_object"
> as not reflecting what the actual error is. After all
> 
>     > typeof [].map.call("foo", function(c,i,a) { return a; })[0]
>     object
> 
> 
> already works, as it should, by coercing its this to an object as specified by
> ToObject. This shows that the problem isn't that the this is a "non_object"
but
> that it's null or undefined. (Or, given document.all, perhaps
"indistinguishable
> from null or undefined")
Changed to "FUNCTIONNAME called on null or undefined"

http://codereview.chromium.org/6902104/diff/3001/src/array.js#newcode995
src/array.js:995: f.call(receiver, current, i, this);
On 2011/04/28 10:44:38, Lasse Reichstein wrote:
> While you are here, make this use %_CallFunction (and similar for the other
that
> calls).
> You may have to handle the receiver being null/undefined too :)
I don't want to make this cl even larger, lets do this in another one

http://codereview.chromium.org/6902104/diff/3001/src/string.js
File src/string.js (right):

http://codereview.chromium.org/6902104/diff/3001/src/string.js#newcode66
src/string.js:66: throw MakeTypeError("obj_ctor_property_non_object",
["charAt"]);
On 2011/04/28 10:44:38, Lasse Reichstein wrote:
> Why "Array.some" but not "String.charAt"?

Done.

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.js
File test/mjsunit/function-call.js (right):

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:35: // Array.prototype.toString,
On 2011/04/28 21:16:54, MarkM wrote:
> Why are these commented out?

Done (and fixed in Array.prototype.toString/toLocaleString which should be
generic)

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:80: [Function.prototype.toString,
On 2011/04/29 06:16:12, Lasse Reichstein wrote:
> How about Function.prototype.call and Function.prototype.apply? :)

Done.

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:149: Array.prototype.reduceRight];
On 2011/04/29 06:16:12, Lasse Reichstein wrote:
> Try testing String.prototype.replace the same way. Let's, for now, assume that
> we should be passing undefined as receiver (I'll ask es5-discuss).
We do not use null as argument to call when in String.prototype.replace as far
as I can see

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:160: assertTrue(/called on non-object/.test(e) ||
/Cannot convert/.test(e));
On 2011/04/29 06:16:12, Lasse Reichstein wrote:
> It helps if the same algorithm can throw more than one TypeError. In that
case,
> we should check that it's the first test in the ES algorithm to fail that is
> actually thrown.
We normally do test the output of the error message to ensure that we actually
get what we expected

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:164:
should_throw_on_null_and_undefined[i].call(undefined);
On 2011/04/29 06:16:12, Lasse Reichstein wrote:
> A comment should suffice. The general rule, which applies to all of these
> functions, is that supplying too few arguments means that the remaining
> parameters will take the undefined value. All of the functions here will test
> their receiver for being convertible to object as the first thing, or have it
as
> a precondition for the non-generic methods.
> 
> Having meaningless and unusable arguments that would by themselves make the
> method fail, is actually a good thing, since it also tests that the
> object-coercible test is performed first.

Comment inserted

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:273: should_throw_on_null_and_undefined[i]);
On 2011/04/28 10:44:38, Lasse Reichstein wrote:
> Indentation (more cases below).

Done.

[Lasse Reichstein, 2011-05-03 09:01:02]

LGTM

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.js
File test/mjsunit/function-call.js (right):

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:149: Array.prototype.reduceRight];
No, but I think we should use undefined for strict functions, and not always the
global receiver.

http://codereview.chromium.org/6902104/diff/9001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:160: assertTrue(/called on non-object/.test(e) ||
/Cannot convert/.test(e));
We call all functions with no parameters, which means that essential parameters
will have the undefined value.
The test for whether the "this" value is null or undefined is always performed
before access to the other parameters, so even if the undefined value is an
invalid argument value, it mustn't change the result of the test.

http://codereview.chromium.org/6902104/diff/9001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:238: assertUnreachable();
How about explicitly converting e to a string before doing regexp test on it.
Can we test the e.type property directly?

[Rico, 2011-05-03 12:41:02]

Please have another look:
Addressed comments
Changed Array.prototype.toString and toLocaleString to only work on arrays
(filled bug: http://code.google.com/p/v8/issues/detail?id=1361)
Removed not passing tests from es5conform and sputnik
Added test exception to mozilla status (double checked that firefox nightly is
also throwing on this)

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.js
File test/mjsunit/function-call.js (right):

http://codereview.chromium.org/6902104/diff/3001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:149: Array.prototype.reduceRight];
On 2011/05/03 09:01:02, Lasse Reichstein wrote:
> No, but I think we should use undefined for strict functions, and not always
the
> global receiver.
Bug filled for this and Array.prototype.sort
http://code.google.com/p/v8/issues/detail?id=1360

http://codereview.chromium.org/6902104/diff/9001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:160: assertTrue(/called on non-object/.test(e) ||
/Cannot convert/.test(e));
On 2011/05/03 09:01:03, Lasse Reichstein wrote:
> We call all functions with no parameters, which means that essential
parameters
> will have the undefined value.
> The test for whether the "this" value is null or undefined is always performed
> before access to the other parameters, so even if the undefined value is an
> invalid argument value, it mustn't change the result of the test.

Done.

http://codereview.chromium.org/6902104/diff/9001/test/mjsunit/function-call.j...
test/mjsunit/function-call.js:238: assertUnreachable();
On 2011/05/03 09:01:03, Lasse Reichstein wrote:
> How about explicitly converting e to a string before doing regexp test on it.
> Can we test the e.type property directly?

Done.

[Lasse Reichstein, 2011-05-03 13:06:52]

LGTM

http://codereview.chromium.org/6902104/diff/11003/src/array.js
File src/array.js (right):

http://codereview.chromium.org/6902104/diff/11003/src/array.js#newcode366
src/array.js:366: if (!IS_ARRAY(this)) {
Remove the test above, then.

http://codereview.chromium.org/6902104/diff/11003/src/array.js#newcode378
src/array.js:378: if (!IS_ARRAY(this)) {
Ditto

[Rico, 2011-05-03 13:15:38]

http://codereview.chromium.org/6902104/diff/11003/src/array.js
File src/array.js (right):

http://codereview.chromium.org/6902104/diff/11003/src/array.js#newcode366
src/array.js:366: if (!IS_ARRAY(this)) {
On 2011/05/03 13:06:52, Lasse Reichstein wrote:
> Remove the test above, then. 

Done.

http://codereview.chromium.org/6902104/diff/11003/src/array.js#newcode378
src/array.js:378: if (!IS_ARRAY(this)) {
On 2011/05/03 13:06:52, Lasse Reichstein wrote:
> Ditto

Done.