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

VERIFIED INVALID

Status

()

defect
VERIFIED INVALID
14 years ago
13 years ago

People

(Reporter: bc, Assigned: mrbkap)

Tracking

({js1.6})

Trunk
mozilla1.9alpha1
x86
Windows XP
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
This was intentional, to match Function.prototype.call and .apply.  Mistake?

/be
Flags: testcase+
shaver, mrbkap: Care to make the call whether this is invalid or not?
(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
Posted file js1_6/Regress/regress-312385.js (obsolete) —
Are the failures in this bogus?
Attachment #207694 - Flags: review?(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
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
Attachment #207694 - Attachment is obsolete: true
Attachment #207694 - Flags: review?(brendan)
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.
Attachment #207738 - Flags: review?(brendan)
This slipped through the cracks.  Need to sort it out.  Taking, will consult with shaver and others.

/be
Assignee: general → brendan
Danger Will Robinson! Danger! /me waves arms spasmodically
Please nominate for spidermonkey 1.6 again when this is ready.
No longer blocks: js1.6rc1
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.
Attachment #207738 - Flags: review?(brendan)
Blake has kindly offered to look into this one.

/be
Assignee: brendan → mrbkap
Target Milestone: --- → mozilla1.9alpha
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?
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
(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.
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.