105972 - /^.*?$/ will not match anything

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Peter Davis]

The result of the following example:
/^.*?$/.exec("hello world")[0]
is an empty string.  If I understand correctly, the *? construct should match the 
minimum number of characters required to make a complete match, which means it should 
match the entire string "hello world".

On the other hand,
/^.*$/.exec("hello world")[0]
/^.+?$/.exec("hello world")[0]
/^.*?d/.exec("hello world")[0]
all return the string "hello world".

Perhaps I am simply misinterpreting the spec, in which case I'm sorry for wasting your 
time.  But it seems that it would be more logical that /^.*?$/ should work.  I have not 
had a chance to test this with other JS interpreters.

Comment 1

[Phil Schwartau]

Reassigning to rogerl. Same behavior in the SpiderMonkey shell:

                  js> /^.*?$/.exec("Hello World") === null
                  true


By contrast, Perl 5 matches the whole string. This script:

                  if ("Hello World" =~ /^.*?$/)
                  {
                    print("match: $&\n");
                  }
                  else
                  {
                    print("NO MATCH!\n");
                  }

produces this output:

                  match: Hello World

Comment 2

[Phil Schwartau]

Testcase added to JS testsuite:

           mozilla/js/tests/ecma_3/RegExp/regress-105972.js

Comment 3

[rogerl (gone)]

Attachment: Allow non-greedy matching up to AND including cpend to match EOL

Comment 4

[Peter Davis]

Thanks rogerl for the quick turnaround!  As part of the reason for filing this bug, 
though, I want to verify that more complicated cases work with your fix, such as:

/^.*?(:|$)/.exec("hello: world") => "hello:"
/^.*?(:|$)/.exec("hello: world") => "hello world"

(I do not have a source build available to test your patch myself).  Thank you!

Comment 5

[rogerl (gone)]

I'm guessing the second example was supposed to be

/^.*(:|$)/.exec("hello: world")

(i.e. greedy)
That being the case the results are:

/^.*?(:|$)/.exec("hello: world") => hello:,:
/^.*(:|$)/.exec("hello: world") => hello: world,
as expected. I think.

Comment 6

[Phil Schwartau]

The latter two examples have been added to the testcase above.
Both of them already pass in SpiderMonkey and Rhino, without the
patch to this bug even being applied.

For example, in SpiderMonkey, using the toSource() operator
to display the match arrays:


-------------  NON-GREEDY CASE (*? operator) -------------
js> /^.*?(:|$)/.exec("Hello: World").toSource()
["Hello:", ":"]

-------------  GREEDY CASE (* operator) ------------------
js> /^.*(:|$)/.exec("Hello: World").toSource()
["Hello: World", ""]

Comment 7

[Peter Davis]

> I'm guessing the second example was supposed to be
> /^.*(:|$)/.exec("hello: world")

Sorry, the example was supposed to be:
/^.*?(:|$)/.exec("hello world")

Comment 8

[Brendan Eich [:brendan]]

This must be the SpiderMonkey bug if that's what the patch fixes -- or should a
twin bug be filed on the JavaScript Engine component of the Browser product?

/be

Comment 9

[Phil Schwartau]

Changing product/component from Rhino/Core to Browser/JS Engine.
I have filed bug 106548 as the twin bug against Rhino -

Comment 10

[Phil Schwartau]

cc'ing some more reviewers for this patch - 

Comment 11

[Kenton Hanson (gone)]

r=khanson

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 54703 [details] [diff] [review]
Allow non-greedy matching up to AND including cpend to match EOL

diff -u format please (you should be able to set that as a default cvs diff
option, via .cvsrc or a MacCVS option [I hope!]).

The only worry I had was that the loop body (not shown due to lack of diff -u
context) would load *cp2 when cp2 == cpend, but that seems to be impossible
from a quick glance at matchRENodes and the REOP_EOL case.  Maybe JS_ASSERT
before the if (*cp2 == '\n') test that cp2 < cpend, just to be sure?

sr=[e-mail] with whatever assertion makes sense.

I'm recording khanson's r= via the patch manager (kenton, please get in the
habit of doing that, it helps mozilla.org gather stats -- thanks).

/be

Comment 13

[rogerl (gone)]

Attachment: Correct overrun error from previous patch

Brendan's right about loading *cp2 from cpend. If the next node does fail to
match, we would merrily attempt to look for a line terminator, eventually from
beyond the end of string. New patch (with context) detects that attempt and
fails.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 62050 [details] [diff] [review]
Correct overrun error from previous patch

I think this is good to go now.  Does it need re-review from khanson?

/be

Comment 15

[Kenton Hanson (gone)]

Yes, I will re review.

=Kenton=

Comment 16

[Kenton Hanson (gone)]

Comment on attachment 62050 [details] [diff] [review]
Correct overrun error from previous patch

r=khanson

Comment 17

[Brendan Eich [:brendan]]

Wahh, another patch all reviewed and ready to go, but not checked in.  Please
set target milestone and check in ASAP (by midnight tonight for 0.9.8, or appeal
to [e-mail] during the 0.9.8 freeze -- else 0.9.9 or bust).

/be

Comment 18

[rogerl (gone)]

Nuts, sorry - I had forgotten these were pending. Fix checked in.

Comment 19

[Phil Schwartau]

Marking Verified FIXED. The above testcase now passes in the
debug and optimized JS shell on Linux, WinNT, and Mac9.1.