57572 - regexp with ? matches incorrectly

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Robert Ginda]

js> str = "foobar"
js> str.match (/(\S+)? ?(.*)/);
foobar,fooba,r
js> str = "foo bar"
js> str.match (/(\S+)? ?(.*)/);
foo bar,foo,bar

The first match should have been "foobar, foobar", the second match is correct. 
This used to work.  Maybe something missed in rogerl's 3.33 revision if
jsregexp.c?

Comment 1

[Robert Ginda]

*** Bug 56249 has been marked as a duplicate of this bug. ***

Comment 2

[Robert Ginda]

*** Bug 56249 has been marked as a duplicate of this bug. ***

Comment 3

[rogerl (gone)]

DOTSTAR was behaving as if no match was a failed match. Here's a patch:

Index: jsregexp.c
===================================================================
RCS file: /m/pub/mozilla/js/src/jsregexp.c,v
retrieving revision 3.33
diff -u -r3.33 jsregexp.c
--- jsregexp.c  2000/09/14 22:39:21     3.33
+++ jsregexp.c  2000/10/24 15:39:35
@@ -1865,7 +1865,6 @@
           for (cp2 = cp; cp2 < cpend; cp2++)
               if (*cp2 == '\n')
                   break;
-          if (cp2 == cp) return NULL;
           while (cp2 >= cp) {
               const jschar *cp3 = matchRENodes(state, ren->next, NULL, cp2);
               if (cp3 != NULL) {

Comment 4

[Robert Ginda]

works here, r=rginda.

Comment 5

[rogerl (gone)]

Attachment: new mega patch fixes all RE bugs

Comment 6

[timeless]

<JS_ASSERT((ts == NULL) || (ts->linebuf.limit < ts->linebuf.base + 
JS_LINE_LIMIT));
>JS_ASSERT(!ts || (ts->linebuf.limit < ts->linebuf.base + JS_LINE_LIMIT));

maybe?

Comment 7

[Brendan Eich [:brendan]]

timeless: and don't parenthesize the < expression, this ain't Pascal.

rogerl, I'll review, someone else should too.

/be

Comment 8

[rogerl (gone)]

Attachment: Patched personal paranthesis paranoia.

Comment 9

[Phil Schwartau]

Added testcase to JS test suite:

             js/tests/ecma_3/RegExp/regress-57572.js


This testcase is currently passing on SpiderMonkey, but parts of it 
are failing in Rhino. Specifically:


CASE A     pattern = /^(\S+)?( ?)(B+)$/;  //single space before second "?" 
           string = 'AAABBB';             //no spaces in given string
           actual = string.match(pattern);
           expect = Array(string, 'AAABB', cnEmptyString, 'B');


In Rhino:  string.match(pattern) === null 



CASE B     pattern = /(.+)?(!?)(!+)/;
           string = 'WOW !!! !!!';
           actual = string.match(pattern);
           expect = Array(string, 'WOW !!! !!', cnEmptyString,  '!');


In Rhino:  string.match(pattern) = ("!!!", "!!! !!!", "!",  "!!")
           This is especially strange: although matches are being produced, 
           the first term in the match array is not the given string. 



In SpiderMonkey:

All expected values are being produced. (I do not know regular expressions 
well enough to know if these are indeed what we should expect, however - )

Comment 10

[Phil Schwartau]

For convenience, here is a hyperlink to the testcase (once lxr updates):

http://lxr.mozilla.org/mozilla/source/js/tests/ecma_3/RegExp/regress-57572.js

Comment 11

[rogerl (gone)]

Generated a meta bug to capture all current R.E. bugs.

Comment 12

[Brendan Eich [:brendan]]

Nits:

- Tabs in the patch?  See for example

+            cp += 3;
+
state->cp = cp;
+        /* Handle empty paren */
+        if (*cp == ')') {

- else after goto is a non-sequitur:

+                    goto madeit;
+                }
+                else {

- (Same area) Why not make the error case the most indented one, by testing

+                if (argc >= 2 && !JSVAL_IS_VOID(argv[1])) { /* 'flags' defined */

instead of

+                if (argc < 2 || JSVAL_IS_VOID(argv[1])) { /* 'flags' undefined */

- (Same area) Hanging indent doesn't line up with start of actual parameters:

+                    JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, 
+                                                    JSMSG_NEWREGEXP_FLAGGED);

- JSMSG_IN_NOT_OBJECT wording too different from JSMSG_BAD_INSTANCEOF_RHS. 
Compare the wording of

-MSG_DEF(JSMSG_IN_NOT_OBJECT,           27, 0, JSEXN_ERR, "target of 'in'
operator must be an object")
+MSG_DEF(JSMSG_IN_NOT_OBJECT,           27, 0, JSEXN_TYPEERR, "target of 'in'
operator must be an object")

to

-MSG_DEF(JSMSG_BAD_INSTANCEOF_RHS,      31, 1, JSEXN_ERR, "invalid instanceof
operand {0}")
+MSG_DEF(JSMSG_BAD_INSTANCEOF_RHS,      31, 1, JSEXN_TYPEERR, "invalid
instanceof operand {0}")

Also, the latter provides a decompiled version of the offending right operand. 
I think the IN message and call (in jsinterp.c) should do the same.  It's good
to say *why* the operand is invalid, and it helps to mention which operand if
possible (neither the IN nor INSTANCEOF messages do that clearly; is "target"
the right operand?).  Sorry, beating a gnat with a crowbar here.

Pick these nits and sr=[e-mail].

/be

Comment 13

[rogerl (gone)]

Fix checked in.

Comment 14

[Phil Schwartau]

Verified Fixed - ran this test suite directory on Linux, WinNT, and Mac

                      js/tests/ecma_3/RegExp/

with both debug and optimized versions of the JS shell, and got 0 errors.