Bug 33319 – RegExp.prototype.toString returns "//" for empty regular expressions

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Kent Hansen]

The ECMA specification states that the string representation of an empty regular expression is /(?:)/.
With the current behavior, "eval((new RegExp).toString())" returns undefined (because // is interpreted as a comment), whereas it should return a regular expression object.

[Kent Hansen]

Created attachment 46048 [details]
Proposed patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 46048 [details] without any errors.

[Darin Adler]

Comment on attachment 46048 [details]
Proposed patch

Why is "/(?:)/" correct? Is this specified in the ECMAScript specification? Is it critical to match Firefox? What does IE do?

[Darin Adler]

Oh, I see.

[Darin Adler]

Comment on attachment 46048 [details]
Proposed patch

> -    return jsNontrivialString(exec, makeString("/", asRegExpObject(thisValue)->get(exec, exec->propertyNames().source).toString(exec), postfix));
> +    UString source = asRegExpObject(thisValue)->get(exec, exec->propertyNames().source).toString(exec);
> +    return jsNontrivialString(exec, makeString("/", source.size() ? source : UString("(?:)"), postfix));

There should be a comment here explaining that "//" has to be avoided because it collides with comment syntax. Otherwise, it's non-obvious why that code exists.

r=me

[WebKit Commit Bot]

Comment on attachment 46048 [details]
Proposed patch

Clearing flags on attachment: 46048

Committed r52981: <http://trac.webkit.org/changeset/52981>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Looks like this may have broken some JS tests:
** Danger, Will Robinson! Danger! The following failures have been introduced:
	ecma_2/RegExp/properties-001.js
	js1_2/regexp/toString.js

2 regressions found.

At least on the Qt bot:
http://build.webkit.org/builders/Qt%20Linux%20Release/builds/5736/steps/jscore-test/logs/stdio

If the error shows up in other bots, I'll begin a rollout.

[Eric Seidel (no email)]

Yup.  Broke all the bots.  Rolling out.  I expect the js tests may just need expectation updates.

[Eric Seidel (no email)]

Reverted r52981 for reason:

Caused two JS tests to start failing: ecma_2/RegExp/properties-001.js and js1_2/regexp/toString.js

Committed r52984: <http://trac.webkit.org/changeset/52984>

[Kent Hansen]

Created attachment 46135 [details]
Revised patch (update Mozilla tests, add comment)

Sorry, I didn't know/remember to run the ECMA test suite on the first try.
Interestingly, the behavior was fixed in Mozilla just one month after the tests were imported into WebKit trunk. :)

[WebKit Review Bot]

Attachment 46135 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/tests/mozilla/js1_2/regexp/toString.js:44:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1

[Kent Hansen]

(In reply to comment #12)
> Attachment 46135 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> JavaScriptCore/tests/mozilla/js1_2/regexp/toString.js:44:  Line contains tab
> character.  [whitespace/tab] [5]
> Total errors found: 1

The line already had a tab character. I only changed the content of '//'.

[Kent Hansen]

(In reply to comment #10)
> Caused two JS tests to start failing: ecma_2/RegExp/properties-001.js and
> js1_2/regexp/toString.js

Stale tests. I spun off https://bugs.webkit.org/show_bug.cgi?id=33382 to investigate bringing WebKit's copy of the tests up-to-date.

[Darin Adler]

Comment on attachment 46135 [details]
Revised patch (update Mozilla tests, add comment)

> +    // If source is empty, use "/(?:)/" to avoid colliding with comment syntax

We use periods in sentence-style comments like this one.

> +    return jsNontrivialString(exec, makeString("/", source.size() ? source : UString("(?:)"), postfix));

I would write it like this:

    return jsNontrivialString(exec, isEmpty() ?  UString("(?:)" : makeString("/", source, postfix));

r=me as is because neither of those comments are important

[WebKit Commit Bot]

Comment on attachment 46135 [details]
Revised patch (update Mozilla tests, add comment)

Rejecting patch 46135 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
outTests/fast/regex/non-pattern-characters-expected.txt
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:

    The following files contain tab characters:

        trunk/JavaScriptCore/tests/mozilla/js1_2/regexp/toString.js

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
 at /usr/local/git/libexec/git-core/git-svn line 558


Full output: http://webkit-commit-queue.appspot.com/results/176030

[Eric Seidel (no email)]

This can't (easily) be landed via the commit-queue because of bug 27204 which deals with svn-apply not understanding svn properties.

[Eric Seidel (no email)]

It would be possible for someone first to mark all of the files in JavaScriptCore/mozilla/ with "allow-tabs" and then this could be commit-queue'd.  I assume those files just pre-date the banning of tabs from svn.webkit.org

[Darin Adler]

Comment on attachment 46135 [details]
Revised patch (update Mozilla tests, add comment)

Give the commit queue a try, now that I added allow-tabs properties.

[WebKit Commit Bot]

Comment on attachment 46135 [details]
Revised patch (update Mozilla tests, add comment)

Clearing flags on attachment: 46135

Committed r53062: <http://trac.webkit.org/changeset/53062>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.