248444 - toString/toSource of RegExp does not escape slashes

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[timeless]

js> eval(""+RegExp("[^/]+$"))
typein:1: SyntaxError: unterminated character class ^:
typein:1: /[^/]+$/
typein:1: ^
js> build()
built on Jun 21 2004 at 18:03:35
(boffo)
also tested: mozilla1.4.1 (viper), mozilla1.2.1 (boffo), 1.7b (lab-xp-tp1)

afaik RegExp("[^/]+$") should be approximately equivalent to /[^\/]+$/

The testing example I have is:

"somepath/file.ext".match(regexpobject)[0]

expected output is: "file.ext".
since I'm using the regular expression twice, I figured I'd construct and store
the object instead of inlining it. Since I'm not particularly fond of escaping
things, I decided to use the RegExp() form. The form works, but xpcshell/js and
venkman all showed sketchy string representations. Note that the non RegExp
constructor /[^\/]+$/ form does correctly unescape.

The nature of the error message for my testcase will vary depending on whether
the build you're using is from before or after the regexp landing (e.g. 1.2.1
output):
js> eval(""+RegExp("[^/]+$"))
typein:1: SyntaxError: unterminated character class [:
typein:1: /[^/]+$/
typein:1: ^
js> build()
built on Nov 30 2002 at 14:36:14

Things to test:
/[^\/]+$/
RegExp("[^\\\/]+$")
RegExp("[^\\/]+$")
RegExp("[^\/]+$")
RegExp("[^/]+$")

Personally, I expect all of the preceding to behave the same (both as regular
expressions and as things convertable to strings).

Comment 1

[Mike Shaver (:shaver emeritus)]

Ah, sweet RegExp.  I think we're up to about 800 cuts now.

Comment 2

[timeless]

js> RegExp("/")
///

Comment 3

[Mike Shaver (:shaver emeritus)]

It's true that the toString/toSource representation isn't lot of use as
eval-fodder, but IE(6) does the same thing, so I'm not sure we can change
toString.  Maybe toSource?  Teetering towards WONTFIX.

Note that the RegExp(RegExp("/").source) works, though.

Comment 4

[Mike Shaver (:shaver emeritus)]

Not js1.5 -- breaking compat isn't a release requirement, usually.

(Wonder what 4.x does.)

Comment 5

[timeless]

Mozilla/4.8 [en] (Windows NT 5.0; U): (RegExp("/")).toSource() => /// 
Mozilla/4.8 [en] (Windows NT 5.0; U): (RegExp("/")).toString() => /// 

Comment 6

[Brendan Eich [:brendan]]

Attachment: proposed fix

Per ECMA-262 Edition 3, 15.10.6.4.

/be

Comment 7

[Brendan Eich [:brendan]]

See the NOTE in 15.10.6.4 -- it allows "///" to be the result of
RegExp("/").toString().  The spec also allows what the patch here does, which
seems better enough to be worth doing.

/be

Comment 8

[Mike Shaver (:shaver emeritus)]

Comment on attachment 152453 [details] [diff] [review]
proposed fix

Seems kinda a shame that we have this match_or_replace thing that can do _one_
text replacement for us, but not a global string replacement.  Still...

Comment 9

[Brendan Eich [:brendan]]

match_or_replace can work globally.  However, calling it from the engine to
replace / with \/ is tricky: you need a regexp with a negative lookbehind
asserting that \ does not precede / (either / is at the front of the string, or
there's no \ char before the /).  Seems better to open-code it (and not to worry
about avoiding reallocs).

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: proposed fix, v2

Oops -- first patch sucked, forgot to scale length by sizeof(jschar).

/be

Comment 11

[Mike Shaver (:shaver emeritus)]

Comment on attachment 152477 [details] [diff] [review]
proposed fix, v2

Even better. r=shaver

Comment 12

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 13

[Brendan Eich [:brendan]]

1.4.3?  Whatever.

Timeless, you can do the work.  I actually would rather *not* make this change
in any branch, since we were ECMA-conformant, and backward compatible, till this
went in for 1.8a2.  What's your requirement, exactly?

/be

Comment 14

[Brendan Eich [:brendan]]

Would like to sync up aviary 1.0 with current trunk in this regard.

/be

Comment 15

[timeless]

i'm not likely to use 1.4.3, but i am likely to use 1.7.x. I'm fine with not
doing 1.4.3.

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

I am against this landing on the 1.4 branch.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 152477 [details] [diff] [review]
proposed fix, v2

Per drivers' edict that 1.7x and aviary1.0 back-end modules match, I'm plusing
for 1.7.3.

/be

Comment 18

[Brendan Eich [:brendan]]

Fixed in 1.7 branch.

/be

Comment 19

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-248444.js

thanks to timeless.

Comment 20

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-248444.js checked in.

Comment 21

[Bob Clary [:bc] (inactive)]

verified fixed.