593663 - "1+2".replace("1+2", "$&+3","g"); returns "1+2", expected "1+2+3"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alice0775 White]

Related:
Bug 592143 - TM: retire three-argument RegExp extensions,
Bug 592027 - YARR: regexp syntax error on unbalanced parens.

"1+2".replace("1+2", "$&+3","g"); 
"1+2".replace("1+2", "$&+3","i"); 
etc...

However, there are *no error* in the Error console.
it annoy everybody in developing Web site and Extensions.

Comment 1

[timeless]

this is of course invalid because "+" is a repetition operator which means you were trying to match "12", "112", etc. and as "1+2" doesn't match that, it wasn't changed.

Comment 2

[Alice0775 White]

(In reply to comment #1)
> this is of course invalid because "+" is a repetition operator which means you
> were trying to match "12", "112", etc. and as "1+2" doesn't match that, it
> wasn't changed.
Why invalid?
in Firefox 3.6.8, both return "1+2+3".

Comment 3

[timeless]

Well, Bug 592143 asked to remove support for flags, that's wontfix.

Thus, a bug expecting flags not to apply in the face of wontfix is invalid.

https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/String/replace

str.replace(regexp|substr, newSubStr|function[, Non-standard flags]);

flags 
A string containing any combination of the RegExp flags:
...
example
...
In this version, a string is used as the first parameter and the global and ignore case flags are specified in the flags parameter.

var str = "Apples are round, and apples are juicy.";  
var newstr = str.replace("apples", "oranges", "gi");

Comment 4

[Boris Zbarsky [:bzbarsky]]

timeless, why did the behavior change from 3.6.8 here?  That seems to not have been intended...

Comment 5

[Chris Leary [:cdleary] (not checking bugmail)]

The bug is valid -- we differ from our former behavior. We used to compile the example as a "flat" regexp. Escaping regexp metachars in the engine can be used as a workaround for this case.

Comment 6

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Flattens metachars in the three-argument replace form.

Simple workaround until we remove the three-argument form.

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

Realized I forgot a null check -- which will occur if we OOM due to a flattening. Will fix that with the rest of the review comments.

Comment 8

[David Mandelin [:dmandelin]]

The basic approach seems OK, but I wonder if it might not be easier to add a flat-match mode to Yarr.

flattenPattern seems overcomplicated to me--does it really cost us anything to just use the buffer every time? It would make the code easier to read. Also, what is lastCharWasEscape for?

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #8)
> The basic approach seems OK, but I wonder if it might not be easier to add a
> flat-match mode to Yarr.

I'd rather minimize the differences between us and upstream for merging and patch sharing purposes.

> flattenPattern seems overcomplicated to me--does it really cost us anything to
> just use the buffer every time? It would make the code easier to read. Also,
> what is lastCharWasEscape for?

We could use the buffer every time -- this is a rare/slow path anyway. I realized that you didn't actually care whether the last character was an escape, but forgot to remove that declaration!

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #9)
> I'd rather minimize the differences between us and upstream for merging and
> patch sharing purposes.

Especially because we dislike this feature :-) -- seems a bad reason to create code skew.

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Flattens metachars in the three-argument replace form.

Simplified!

Comment 12

[Chris Leary [:cdleary] (not checking bugmail)]

Attachment: Flattens metachars in the three-argument replace form.

Sorry, previous was stale exported patch.

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 476369 [details] [diff] [review]
Flattens metachars in the three-argument replace form.

Just a few nits:

>+        for (jschar *it = patstr->chars(); it != patstr->chars() + patstr->length(); ++it) {

I know its cold code, but for the sake of setting a good example and not setting of the spidey-senses of passer-byers, could you compute a beg/end pair before the loop (since neither chars() nor length() is a trivial accessor)?

>+                if (!cb.append(ESCAPE_CHAR) ||
>+                    !cb.append(*it))

Seems like this could fit on one line (thereby avoiding a nit about no {'s around the then stmt ;-)

>+    static const uint32 optarg = 2;

The future says 'thank you' :)  Perhaps even a comment about what optarg means?

>+++ b/js/src/trace-test/tests/basic/bug593663-regexp.js
>@@ -0,0 +1,1 @@
>+assertEq( "1+2".replace("1+2", "$&+3","g"), "1+2+3");

Could you write a similar test for every character in isMetaChar and perhaps some extra tests for odd things like \+, \\+, \\\, \\\\ and check that it passes on TM tip and 3.6?  The pessimist in me assumes that there is at least one special case in that set and it can't be as simple as just throwing \ in front ;-)

Comment 15

[Brendan Eich [:brendan]]

Seems like this might be important to fix or b7, from dup'age.

/be

Comment 16

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/tracemonkey/rev/faa24a902263

(There was a comment about what optarg meant elsewhere in the file.)

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/faa24a902263

Comment 18

[timeless]

oops, sorry