Bug 71572 – String new RegExp('\n').toString() returns is invalid RegularExpressionLiteral

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

new RegExp('\n').toString() returns 
/
/
, so this result is including LineTerminator.

And, according to ECMA262 section 15.10.6.4
"The returned String has the form of a RegularExpressionLiteral that evaluates to another RegExp object with the same behaviour as this object."
http://es5.github.com/#x15.10.6.4

And, because of RegularExpressionNonTerminator, LineTerminator is not allowed in RegularExpressionLiteral. So RegExp.source should be escaped.

Furthermore, new RegExp('\\\n').toString() should provide /\n/, because /\\\n/ doesn't have the same behaviour to new RegExp('\\\n').

[Yusuke Suzuki]

Created attachment 113682 [details]
Patch

attached patch

[Yusuke Suzuki]

reported issue and sent patch to V8
http://code.google.com/p/v8/issues/detail?id=1813

[Gavin Barraclough]

Comment on attachment 113682 [details]
Patch

Good catch & nice fix. But I think we may want to make one tweak to this patch before landing.  Most regular expressions don't contain forwards slashes or newlines, but this patch will always produce a fresh copy of the string every time source is called. Currently the code has an early return for strings that don't contain a forwards slash. We could expand this to also check for newline characters, and early return if the string contains neither forward slashes or newlines. r- for the loss of this optimization, but r+ in principle, we should definitely take this fix & the code looks great otherwise. :-)

[Yusuke Suzuki]

(In reply to comment #3)
> (From update of attachment 113682 [details])
> Good catch & nice fix. But I think we may want to make one tweak to this patch before landing.  Most regular expressions don't contain forwards slashes or newlines, but this patch will always produce a fresh copy of the string every time source is called. Currently the code has an early return for strings that don't contain a forwards slash. We could expand this to also check for newline characters, and early return if the string contains neither forward slashes or newlines. r- for the loss of this optimization, but r+ in principle, we should definitely take this fix & the code looks great otherwise. :-)

Thanks for your review. You're right. I'll add early return to this patch.

[Yusuke Suzuki]

Created attachment 115232 [details]
Patch

attached patch v2. First, checking this pattern should be escaped, and if it is false return early

[Darin Adler]

Comment on attachment 115232 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115232&action=review

Looks OK. Needs a couple small changes.

> Source/JavaScriptCore/runtime/RegExpObject.cpp:124
> +    bool previousIsBackSlash = false;

backslash is one word, not two, so this should be named previousCharacterWasBackslash

> Source/JavaScriptCore/runtime/RegExpObject.cpp:145
> +        if (ch == '\n' || ch == '\r' || (ch & ~1) == 0x2028) {

This should use the inline isLineTerminator function instead of repeating its code.

> Source/JavaScriptCore/runtime/RegExpObject.cpp:157
> +    if (!shouldEscape)
> +      return jsString(exec, pattern);

This is indented wrong. Should be indented four spaces.

> Source/JavaScriptCore/runtime/RegExpObject.cpp:177
> +        if (ch == '\n' || ch == '\r' || (ch & ~1) == 0x2028) {

This should use the inline isLineTerminator function instead of repeating its code.

[Yusuke Suzuki]

(In reply to comment #6)
Thank you for your review.

> backslash is one word, not two, so this should be named previousCharacterWasBackslash
Yes, I'll rename it.

> > Source/JavaScriptCore/runtime/RegExpObject.cpp:145
> > +        if (ch == '\n' || ch == '\r' || (ch & ~1) == 0x2028) {
> This should use the inline isLineTerminator function instead of repeating its code.
Right, I'll change it to using Lexer<UChar>::isLineTerminator.

> This is indented wrong. Should be indented four spaces.
Oops. This is mistake.

I'll upload revised patch soon.

[Yusuke Suzuki]

Created attachment 115244 [details]
Patch

attached patch v3

[Gavin Barraclough]

Comment on attachment 115244 [details]
Patch

Looks great.

[WebKit Review Bot]

Comment on attachment 115244 [details]
Patch

Clearing flags on attachment: 115244

Committed r100469: <http://trac.webkit.org/changeset/100469>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.