Bug 31689 - RegExp#exec's returned Array-like object behaves differently from regular Arrays
Summary: RegExp#exec's returned Array-like object behaves differently from regular Arrays
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-19 14:40 PST by J Smith
Modified: 2009-11-24 20:29 PST (History)
5 users (show)

See Also:


Attachments
RegExpMatchesArray::fillArrayInstance patch (555 bytes, patch)
2009-11-19 14:40 PST, J Smith
no flags Details | Formatted Diff | Diff
Patch for RegExpMatchesArray (3.91 KB, patch)
2009-11-24 07:52 PST, J Smith
no flags Details | Formatted Diff | Diff
Patch for RegExpMatchesArray with corrections (4.18 KB, patch)
2009-11-24 12:02 PST, J Smith
eric: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch for RegExpMatchesArray with expected test results (5.50 KB, patch)
2009-11-24 13:18 PST, J Smith
eric: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch for RegExpMatchesArray with corrected tabs (5.57 KB, patch)
2009-11-24 18:15 PST, J Smith
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description J Smith 2009-11-19 14:40:28 PST
Created attachment 43524 [details]
RegExpMatchesArray::fillArrayInstance patch

RegExpMatchesArray objects returned from RegExp#exec behave differently than regular Arrays when using the "in" operator and the Array#forEach method. 

The RegExp#exec method returns an Array of captured matches from within the tested String. For optional captures, "undefined" is inserted into the Array as a missing match. The returned Array seems to be correct when you try to access the values directly using Array notation but using "in" or forEach will skip over any "undefined" values as if they aren't there.

For instance:

m = /(a)(_)?.+(c)(_)?.+/.exec("abcd");
console.log(m);
console.log(2 in m);
m.forEach(function(x) {
  console.log(x);
});

Should produce:

["abcd", "a", undefined, "c", undefined]
true
abcd
a
undefined
c
undefined

However, WebKit produces:

["abcd", "a", undefined, "c", undefined]
false
abcd
a
c

Using a regular Array similar to the one produced by RegExp#exec produces the expected result, so there is some inconsistency here.

The expected behaviour is based on what is produced by the Rhino, KJS and V8 engines. (In V8, the undefined values are inexplicably labeled "null" in their console output but checking with the strict equality operator shows that the undefined values are indeed undefined and not null. In IE8... well, the results are stranger still, as it acts in the exact opposite manner that WebKit does and produces empty Strings instead of undefineds or nulls to boot!)

A quick fix (small patch attached for the sake of convenience) appears to be to add an extra condition to JavaScriptCore/runtime/RegExpConstructor.cpp in RegExpMatchesArray::fillArrayInstance at line 135 to make sure that those undefined values are inserted properly:

131     for (unsigned i = 0; i <= lastNumSubpatterns; ++i) {
132         int start = d->lastOvector()[2 * i];
133         if (start >= 0)
134             JSArray::put(exec, i, jsSubstring(exec, d->lastInput, start, d->lastOvector()[2 * i + 1] - start));
135         else
136             JSArray::put(exec, i, jsUndefined());
137     }
Comment 1 Alexey Proskuryakov 2009-11-19 17:10:04 PST
Thank you for the bug report, and for the proposed fix.

Would you be willing to submit a patch for review, as described in <http://webkit.org/coding/contributing.html>?
Comment 2 J Smith 2009-11-20 08:46:49 PST
Not a problem. Should any new layout tests be added for the fix? This is my first WebKit patch so I'm not entirely sure of the procedure...
Comment 3 Alexey Proskuryakov 2009-11-20 14:52:12 PST
Good question! Yes, a layout test needs to be added to LayoutTests/fast/js. To make one, add a test script to LayoutTests/fast/js/script-tests, and then use make-script-test-wrappers script to generate an HTML wrapper. Expected results are generated by run-webkit-tests itself.

Looks like a link in <http://webkit.org/coding/contributing.html> promises to explain this, but <http://webkit.org/quality/testwriting.html> fails to do that.
Comment 4 J Smith 2009-11-24 07:47:48 PST
Okay, I've created a patch, tests and such included. Hopefully everything is correct and in the right places. Just let me know how it looks and if everything is okay...
Comment 5 J Smith 2009-11-24 07:52:47 PST
Created attachment 43769 [details]
Patch for RegExpMatchesArray
Comment 6 Eric Seidel 2009-11-24 10:45:09 PST
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree explains more about test writing, but it has long been on my list of TODOs to enhance both that page and the webkit.org/quality/testwriting.html page.
Comment 7 Eric Seidel 2009-11-24 10:47:58 PST
Ideally JavaScriptCore/runtime/RegExpConstructor.cpp would be updated with your Copyright line.  And the ChangeLog would include your full name (unless your first name is actually 'J').  Overall the patch seems good to me.  I'm not sure I understand the test case fully, but I would have to stare at it more.

Technically I think that the test case violates the webkit style guidlines ({ on a new line after a function name), although we aren't very clear about our JavaScript styling.  Thank you very much for the patch!
Comment 8 J Smith 2009-11-24 11:53:59 PST
Is there anything like the "WebKit Group" or something the copyright could be assigned to? I have no problems assigning copyright over for a two-liner, especially if it makes licensing easier...

The test case itself just ensures that the RegExpMatchesArrays that are created by RegExp#exec insert the "undefined" entries where necessary, whether they're at the beginning of a match Array, in the middle or the end. It also tests the "in" operator and forEach function to make sure that the RegExpMatchesArray is behaving in the same manner as a plain old JavaScript Array. In the end, the three test functions should test the equivalence of Arrays to RegExpMatchesArrays with regards to the "in" operator and the forEach function.

I just noticed that I didn't include my forEach function tests, so I should add those as well.

I'll upload a new patch with the fixes for formatting and my missing forEach test. For the copyright statement, I'll add it, but I feel rather sheepish for adding a copyright statement for a two-liner. Again, if there's any group that accepts copyright assignment on WebKit's behalf, I can certainly assign copyright over if it's easier for licensing...

(On a somewhat related note with regards to DumpRenderTree, I'm having problems building that tool and am getting "make: *** No rule to make target .../Debug/DumpRenderTree" errors. I've only been able to run tests in LayoutTests manually. Any hints for a WebKit newbie?)
Comment 9 J Smith 2009-11-24 12:02:09 PST
Created attachment 43795 [details]
Patch for RegExpMatchesArray with corrections
Comment 10 Eric Seidel 2009-11-24 12:03:50 PST
(In reply to comment #8)
> Is there anything like the "WebKit Group" or something the copyright could be
> assigned to? I have no problems assigning copyright over for a two-liner,
> especially if it makes licensing easier...

No, for better or worse, there is no "WebKit Group".  Most folks just list themselves, or their employer (if they have an explicit copyright assignment agreement with their employer).

> I'll upload a new patch with the fixes for formatting and my missing forEach
> test. For the copyright statement, I'll add it, but I feel rather sheepish for
> adding a copyright statement for a two-liner. Again, if there's any group that
> accepts copyright assignment on WebKit's behalf, I can certainly assign
> copyright over if it's easier for licensing...

Thank you.

> (On a somewhat related note with regards to DumpRenderTree, I'm having problems
> building that tool and am getting "make: *** No rule to make target
> .../Debug/DumpRenderTree" errors. I've only been able to run tests in
> LayoutTests manually. Any hints for a WebKit newbie?)

Which port are you building?  If you're using make instead of build-webkit and build-dumprendertree I assume that you're using the gtk port.

run-webkit-tests calls build-dumprendertree on most ports and correctly builds DumpRenderTree if needed.  Not all ports have correctly wired up the scripts though, and it's possible you're using one of the ports which is missing this functionality.
Comment 11 Eric Seidel 2009-11-24 12:05:53 PST
Comment on attachment 43795 [details]
Patch for RegExpMatchesArray with corrections

This looks right to me.  Thanks again for the patch!
Comment 12 Oliver Hunt 2009-11-24 12:06:51 PST
(In reply to comment #11)
> (From update of attachment 43795 [details])
> This looks right to me.  Thanks again for the patch!

Still no expected results :-/
Comment 13 WebKit Commit Bot 2009-11-24 12:08:07 PST
Comment on attachment 43795 [details]
Patch for RegExpMatchesArray with corrections

Rejecting patch 43795 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit']" exit_code: 1
Last 500 characters of output:
ojects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/RenderLayerBacking.o /Users/eseidel/Projects/CommitQueue/WebCore/rendering/RenderLayerBacking.cpp normal i386 c++ com.apple.compilers.gcc.4_2
	Distributed-CompileC /Users/eseidel/Projects/build/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSWebGLRenderingContextCustom.o /Users/eseidel/Projects/CommitQueue/WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(14 failures)
Comment 14 Eric Seidel 2009-11-24 12:10:12 PST
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 43795 [details] [details])
> > This looks right to me.  Thanks again for the patch!
> 
> Still no expected results :-/

Oops.  My bad.  Looks like the commit-queue also found a compile failure.  Sadly the commit-queue doesn't know how to upload the entire compile results yet.  :(  See bug 28286.
Comment 15 Eric Seidel 2009-11-24 12:11:46 PST
Actually, looks like the compile failure was bogus.  I'll leave the patch cq- though due to the missing results.  The false rejection was caused by bug 30098 (which I have a fix for, I just haven't landed it yet).
Comment 16 J Smith 2009-11-24 12:28:54 PST
Ah, yeah, I guess those would help. I'll add those to the patch and re-up as soon as my local build is done. I think my problem with DumpRenderTree may be because I'm building JavaScriptCore in Xcode but running build-dumprendertree and run-webkit-tests from a terminal. Probably shouldn't be mixing those I guess. (My build environment is OSX 10.6.2 on an MBP, fwiw...)
Comment 17 Oliver Hunt 2009-11-24 12:31:24 PST
(In reply to comment #16)
> Ah, yeah, I guess those would help. I'll add those to the patch and re-up as
> soon as my local build is done. I think my problem with DumpRenderTree may be
> because I'm building JavaScriptCore in Xcode but running build-dumprendertree
> and run-webkit-tests from a terminal. Probably shouldn't be mixing those I
> guess. (My build environment is OSX 10.6.2 on an MBP, fwiw...)

build-dumprendertree is called by run-webkit-tests.  The issue is likely that you need to do build-webkit so that the entire stack is built.

--Oliver
Comment 18 J Smith 2009-11-24 13:18:50 PST
Created attachment 43804 [details]
Patch for RegExpMatchesArray with expected test results

Okay, here's the updated patch with the expected test results.

Since this attachment upload page says "Please also add your copyright (name and year) to the relevant files for changes that are more than 10 lines of code", I guess I can forgo the copyright statement as the patch is pretty trivial.

Hopefully everything's in place now...
Comment 19 Eric Seidel 2009-11-24 13:49:22 PST
Comment on attachment 43804 [details]
Patch for RegExpMatchesArray with expected test results

The ChangeLog is doesn't mention the -expected.txt file, but that's OK. :)  Looks good, I'll add it to the commit-queue again for automated landing.

Thanks, and congrats on your first WebKit patch!
Comment 20 WebKit Commit Bot 2009-11-24 15:11:00 PST
Comment on attachment 43804 [details]
Patch for RegExpMatchesArray with expected test results

Rejecting patch 43804 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	ChangeLog
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/runtime/RegExpConstructor.cpp
	A	LayoutTests/fast/js/regexp-in-and-foreach-handling-expected.txt
	A	LayoutTests/fast/js/regexp-in-and-foreach-handling.html
	A	LayoutTests/fast/js/script-tests/regexp-in-and-foreach-handling.js
Comment 21 Eric Seidel 2009-11-24 15:27:20 PST
Comment on attachment 43804 [details]
Patch for RegExpMatchesArray with expected test results

I think that rejection was bogus.  Looks like bug 31734.
Comment 22 WebKit Commit Bot 2009-11-24 15:37:53 PST
Comment on attachment 43804 [details]
Patch for RegExpMatchesArray with expected test results

Rejecting patch 43804 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	ChangeLog
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/runtime/RegExpConstructor.cpp
	A	LayoutTests/fast/js/regexp-in-and-foreach-handling-expected.txt
	A	LayoutTests/fast/js/regexp-in-and-foreach-handling.html
	A	LayoutTests/fast/js/script-tests/regexp-in-and-foreach-handling.js
Comment 23 Eric Seidel 2009-11-24 15:47:42 PST
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:

    The following files contain tab characters:

        trunk/LayoutTests/fast/js/script-tests/regexp-in-and-foreach-handling.js

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/libexec/git-core//git-svn line 469

Ahha!  The fact that that comment is missing from teh bug  is a recent bugzilla-tool regression caused by zoltan's change.  I'll update bug 31734
Comment 24 Eric Seidel 2009-11-24 15:50:47 PST
The end result is that one of your files has a tab in it.  Due to bug 31734, the correct reason for the failure was not correctly posted in the bug.  In order to get this landed, either someone will have to land it by hand (removing the tab), or you'll have to upload a new patch w/o a tab in it.  I'm sorry that our current tooling does not warn you about stray tabs earlier. :(  We really need to make our tools more friendly to new contributers!
Comment 25 J Smith 2009-11-24 18:15:08 PST
Created attachment 43821 [details]
Patch for RegExpMatchesArray with corrected tabs

Okay, deep breath here, J.

Expected results for tests -- check.
ChangeLog completed -- check.
No tabs for indentation -- check.
Fingers crossed -- check.

Here goes nothin'...
Comment 26 WebKit Commit Bot 2009-11-24 18:45:10 PST
Comment on attachment 43821 [details]
Patch for RegExpMatchesArray with corrected tabs

Clearing flags on attachment: 43821

Committed r51369: <http://trac.webkit.org/changeset/51369>
Comment 27 WebKit Commit Bot 2009-11-24 18:45:17 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Eric Seidel 2009-11-24 20:29:42 PST
Yay!  Congrats again on your first change to webkit.  Sorry our tools gave you trouble!