Bug 12963 – Inconsistencies in JS array extras with Mozilla implementation

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Jeff Walden (remove +bwo to email)]

I recently made the effort to fully quantify the exact details of Mozilla's support for its array extensions: some, every, forEach, map, filter, and (lastI|i)ndexOf.  Exact JavaScript definitions of these methods which fully match the native Mozilla implementations are now posted on the Mozilla Developer Center:

http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Objects:Array:indexOf
http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Objects:Array:lastIndexOf
http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Objects:Array:some
http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Objects:Array:filter
http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Objects:Array:every
http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Objects:Array:map
http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Objects:Array:forEach

Safari's done a good job of matching the Mozilla implementations, for the most part (including not replicating a bug Mozilla had up until the latest update).  I've found a couple things you do differently, tho, and I'm posting a patch for them.  (Feel free to compare against the above URLs for yourself as a double-check for any other latent issues.)  The issues discovered are as follows:

- forEach returns this, not undefined
- all the methods should ignore holes, not treat them as undefined (I think
  every/forEach/some already do this?)

I do not have a Mac and thus haven't tested the to-be-posted patch or the current implementation's behavior.

[Jeff Walden (remove +bwo to email)]

Created attachment 13470 [details]
Patch

Picking the reviewer for the original extras implementation, let me know if someone else should do this...

[Jeff Walden (remove +bwo to email)]

I also have ECMAScript-262 ed. 3 style specs, since much of the code in some of the examples replicates specification-defined operators like ToNumber.  I currently have these specs at <http://web.mit.edu/jwalden/www/array-extras.js>.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> I also have ECMAScript-262 ed. 3 style specs, since much of the code in some of
> the examples replicates specification-defined operators like ToNumber.  I
> currently have these specs at <http://web.mit.edu/jwalden/www/array-extras.js>.

Thanks for the patch, Jeff!  There are a few things needed before this may be landed:

- Create or update existing layout tests that fail before the patch is applied, and pass after the patch is applied.  I know you don't have a Mac, but these particular layout tests should run in Mozilla/Firefox (not all layout tests will work this way), and you should be able to add tests for the changed behavior that may be verified later by someone with a Mac.  If there are any Mac computer labs at MIT, you should be able to write/update these tests quite easily:

LayoutTests/fast/js/array-every.html
LayoutTests/fast/js/array-float-delete.html
LayoutTests/fast/js/array-foreach.html
LayoutTests/fast/js/array-index-immediate-types.html
LayoutTests/fast/js/array-indexof.html
LayoutTests/fast/js/array-join-bug-11524.html
LayoutTests/fast/js/array-lastIndexOf.html
LayoutTests/fast/js/array-some.html
LayoutTests/fast/js/array-tostring-ignore-separator.html

- A changelog entry (generated by prepare-ChangeLog in the WebKitTools/Scripts directory, then edited to include bug information and a description of the fix).

- A patch created by the WebKitTools/Scripts/svn-create-patch script (after the above changes) that includes the ChangeLog entries for JavaScriptCore and LayoutTests, the layout test changes, and the code changes.

Thanks again!

[Darin Adler]

Comment on attachment 13470 [details]
Patch

Thanks. This help is quite welcome!

It's hard to read this patch because it has so much context! I'm really used to the plain old "-u".

The code change looks fine, but we need some test cases and a change log before we can review+ this.

[Jeff Walden (remove +bwo to email)]

Created attachment 13476 [details]
Full patch (but haven't attempted to run the tests yet)

(In reply to comment #4)
> It's hard to read this patch because it has so much context! I'm really used
> to the plain old "-u".

Not overdoing context is good, but since -p was useless I wanted enough to show the names of the methods associated with each change.

> The code change looks fine, but we need some test cases and a change log
> before we can review+ this.

I whipped up some tests/changes earlier tonight as a break from classwork.  I'm pretty sure the changes are correct, except possibly with respect to amounts of (trailing) whitespace (ugh).  I should have access to a Mac tomorrow night, so hopefully I'll be able to build/verify the tests then if no one else has (hence no review flag yet).

By the way, these methods are all tested in Mozilla's JS test suite, which is being reorganized to separate Moz-specific behaviors from ECMA-262 canon to aid use by other implementations.  (There are nascent plans to try to make some sort of generalized ECMA-262 test suite, but that's a while away for now.)  Except for forEach returning undefined, I'd bet we have everything else tested -- resyncing wouldn't be a bad idea if you have the time, as a further correctness check.

[Jeff Walden (remove +bwo to email)]

I wrongly expected the |getProperty()| function in JavaScriptCore returned NULL only for non-existent properties and not for properties with value set to |undefined|, so as written the patch doesn't pass the tests in it (even ignoring the whitespace and formatting flubs in it).  I need to use a getProperty/hasProperty pair to fix the problem.

Hopefully I'll have time to fix that by the end of the week, but I doubt it'll be any sooner.

[Jeff Walden (remove +bwo to email)]

Created attachment 13786 [details]
Complete patch

Returning false for an array index whose element is undefined seems to be a bug, because hasProperty just returns the result of getPropertySlot.

This patch includes tests, passes all of them, and regresses no existing tests.

[Darin Adler]

Comment on attachment 13786 [details]
Complete patch

r=me

[Jeff Walden (remove +bwo to email)]

http://trac.webkit.org/projects/webkit/changeset/20569