312385 - String.indexOf(arg, pattern) should convert arg to string

Status: VERIFIED INVALID

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

String.indexOf(null, 'l') == -1
String.indexOf(String(null), 'l') == 2

String.indexOf(undefined, 'l') == 11
String.indexOf(String(undefined), 'l') == 2

see js1_6/String/regress-306591.js

Comment 1

[Brendan Eich [:brendan]]

This was intentional, to match Function.prototype.call and .apply.  Mistake?

/be

Comment 2

[Bob Clary [:bc] (inactive)]

shaver, mrbkap: Care to make the call whether this is invalid or not?

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #0)
> String.indexOf(null, 'l') == -1

null is the nominal |this| paramter of String.prototype.indexOf, called from the generic dispatcher implementing String.indexOf.  Per ECMA-262, when null is the base object of a Reference being invoked, i.e. the nominal |this| parameter, it is replaced with the global object (which one?  ECMA doesn't talk about multiple, but static scope is the only way to go, so it's the last object on the scope chain reachable from the callee).

The native String.prototype.indexOf function dutifully converts the global object in the JS shell to a string: "[object global]", and returns the index of the first 'l', namely 9.

> String.indexOf(String(null), 'l') == 2
> 
> String.indexOf(undefined, 'l') == 11
> String.indexOf(String(undefined), 'l') == 2

Similar comments apply to these cases.  I think this is INVALID, but we are now extending ECMA-262 with these generic functions.  They take the general form C.f(t, a) for |this| parameter |t| and arguments |a| in class |C| with function |f|, and call C.prototype.f.apply(t, a).  Note that 

js> String.indexOf(null, 'l') === String.prototype.indexOf.apply(null, ['l'])
true
js> String.indexOf(undefined, 'l') === String.prototype.indexOf.apply(undefined, ['l'])
true

This is the key: the generics are defined in terms of apply.  If you test all identities constructed based on this derivation and find a mismatch, report it here.  If not, then INVALID.

/be

Comment 4

[Bob Clary [:bc] (inactive)]

Attachment: js1_6/Regress/regress-312385.js

Are the failures in this bogus?

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> Created an attachment (id=207694) [edit]
> js1_6/Regress/regress-312385.js
> 
> Are the failures in this bogus?

No, but my use of === in comment 3 was bogus.  Let's look at some of the results in detail:

FAILED!: String.split(null) == String.prototype.split.apply(null)
FAILED!: Expected value 'true', Actual value 'false'

split returns an Array instance, in this case with one element, the global object. JS1 is broken in that no two distinct objects compare == or ===, but we would like [1] == [1], [1,2,3] < [1,2,4], etc.  You'll want an "isomorphic Array" comparison function, that wants matching length and for each element, the same value or object reference (or recursively, to be general, isomorphic elements).

Same comments for concat.

The new-in-1.6 Array extras show a bug in error reporting:

FAILED!: Array.forEach(null) == Array.prototype.forEach.apply(null)
FAILED!: Type mismatch, expected type boolean, actual type string
FAILED!: Expected value 'true', Actual value 'TypeError: Array.forEach is not a
function'

You can't call forEach zero arguments (null |this| is replaced with the global object of course, but there is no second argument to the Array.forEach call, and no array of actual arguments to the .apply call, so no function to call for each element.

So my comment 3 identity has to be amended again to allow for minimum required arguments.

The bug evident above where TypeError: Array.forEach is not a function is thrown shows the wrong stack value generator being decompiled.  It's not the callee, Array.forEach, that's not a function (it is), it is the missing argument (after the null |this| param).  I'll try to hack a patch and attach it here.

/be

Comment 6

[Brendan Eich [:brendan]]

The error reporting bug is not peculiar to the generic (Array.forEach) case:

js> [].forEach()
typein:1: TypeError: [].forEach is not a function
js> Array.forEach([])
typein:2: TypeError: Array.forEach is not a function
js> Array.forEach([], 42)
typein:3: TypeError: number is not a function
js> Array.forEach([], 'argh')
typein:4: TypeError: string is not a function
js> [].forEach('argh')
typein:5: TypeError: string is not a function

/be

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: js1_6/Regress/regress-312385-01.js v2

Testing return values of generics v2. Excludes forEach and uses array compares via toSource when appropriate. Fails in push, pop, shift, splice since they attempt to modify the "array". I think these should just be removed the way forEach was.

I'll do a -02 for the error cases when I understand the proper error messages.

Comment 8

[Brendan Eich [:brendan]]

This slipped through the cracks.  Need to sort it out.  Taking, will consult with shaver and others.

/be

Comment 9

[Bob Clary [:bc] (inactive)]

Danger Will Robinson! Danger! /me waves arms spasmodically

Comment 10

[Bob Clary [:bc] (inactive)]

Please nominate for spidermonkey 1.6 again when this is ready.

Comment 11

[Bob Clary [:bc] (inactive)]

Comment on attachment 207738 [details]
js1_6/Regress/regress-312385-01.js v2

Checking in regress-312385-01.js;
/cvsroot/mozilla/js/tests/js1_6/Regress/regress-312385-01.js,v  <--  regress-312385-01.js
initial revision: 1.1

this test will fail until the background issues are decided.

Comment 12

[Brendan Eich [:brendan]]

Blake has kindly offered to look into this one.

/be

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

I'm going to call this bug INVALID. The test, as written, does not do the right thing -- it fills the global object with random properties (consider, you're calling unshift on the same object twice in a row and comparing the result, where unshift returns length of the object, which increases with each call).

I also don't see why this is the desired behavior, our current behavior matches .call and.apply -- and in the case where you're e.g., using Array.shift on some DOM list, this bug seems to want us to copy the DOM list into an array proper, which seems wasteful.

Bob, am I misunderstanding?

Comment 14

[Bob Clary [:bc] (inactive)]

(In reply to comment #13)

I think so. I guess I have been really asking for the answer to the question in comment 1 whether we should have used <http://bclary.com/2004/11/07/#a-15.5.4.7> and passed String(null) as the |this| object or used <http://bclary.com/2004/11/07/#a-15.3.4.3> and passed String(global object) as the |this| object. I suppose 15.3.4.3 is "pure water" according to ecma 262-3, and ...

Firefox & Opera & Safari: 
String.prototype.indexOf.apply(null, ['l']) == -1
IE:
String.prototype.indexOf.apply(null, ['l']) == 2

I didn't have a Mac when I filed this, and with us, Opera and Safari outvoting IE 3 to 1, I guess that answers that question and this bug as filed originally is INVALID.

Apart from the 'TypeError: setting a property that has only a getter' errors in the tests for this bug, all of the tests pass. I think removing the causes of TypeError will allow me to rest.

Comment 15

[Bob Clary [:bc] (inactive)]

Checking in regress-312385-01.js;
/cvsroot/mozilla/js/tests/js1_6/Regress/regress-312385-01.js,v  <--  regress-312385-01.js
new revision: 1.2; previous revision: 1.1
done