Bug 33319 - RegExp.prototype.toString returns "//" for empty regular expressions
Summary: RegExp.prototype.toString returns "//" for empty regular expressions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Kent Hansen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-07 05:34 PST by Kent Hansen
Modified: 2010-01-10 20:55 PST (History)
4 users (show)

See Also:


Attachments
Proposed patch (5.20 KB, patch)
2010-01-07 05:40 PST, Kent Hansen
no flags Details | Formatted Diff | Diff
Revised patch (update Mozilla tests, add comment) (7.03 KB, patch)
2010-01-08 06:35 PST, Kent Hansen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Hansen 2010-01-07 05:34:06 PST
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.
Comment 1 Kent Hansen 2010-01-07 05:40:01 PST
Created attachment 46048 [details]
Proposed patch
Comment 2 WebKit Review Bot 2010-01-07 05:43:30 PST
style-queue ran check-webkit-style on attachment 46048 [details] without any errors.
Comment 3 Darin Adler 2010-01-07 09:18:31 PST
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?
Comment 4 Darin Adler 2010-01-07 09:18:43 PST
Oh, I see.
Comment 5 Darin Adler 2010-01-07 09:20:15 PST
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
Comment 6 WebKit Commit Bot 2010-01-08 03:17:35 PST
Comment on attachment 46048 [details]
Proposed patch

Clearing flags on attachment: 46048

Committed r52981: <http://trac.webkit.org/changeset/52981>
Comment 7 WebKit Commit Bot 2010-01-08 03:17:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 8 Eric Seidel (no email) 2010-01-08 03:33:52 PST
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.
Comment 9 Eric Seidel (no email) 2010-01-08 03:34:45 PST
Yup.  Broke all the bots.  Rolling out.  I expect the js tests may just need expectation updates.
Comment 10 Eric Seidel (no email) 2010-01-08 03:44:41 PST
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>
Comment 11 Kent Hansen 2010-01-08 06:35:27 PST
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. :)
Comment 12 WebKit Review Bot 2010-01-08 06:40:46 PST
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
Comment 13 Kent Hansen 2010-01-08 06:45:13 PST
(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 '//'.
Comment 14 Kent Hansen 2010-01-08 06:48:11 PST
(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.
Comment 15 Darin Adler 2010-01-08 07:50:00 PST
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
Comment 16 WebKit Commit Bot 2010-01-08 17:41:22 PST
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
Comment 17 Eric Seidel (no email) 2010-01-10 19:34:33 PST
This can't (easily) be landed via the commit-queue because of bug 27204 which deals with svn-apply not understanding svn properties.
Comment 18 Eric Seidel (no email) 2010-01-10 19:35:37 PST
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
Comment 19 Darin Adler 2010-01-10 20:34:56 PST
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.
Comment 20 WebKit Commit Bot 2010-01-10 20:55:36 PST
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>
Comment 21 WebKit Commit Bot 2010-01-10 20:55:44 PST
All reviewed patches have been landed.  Closing bug.