The default bug view has changed. See this FAQ.

Javascript Regular Expression Non-participating Capture Groups are inaccurate in edge cases.

RESOLVED DUPLICATE of bug 369778

Status

()

Core
JavaScript Engine
RESOLVED DUPLICATE of bug 369778
10 years ago
3 years ago

People

(Reporter: Kris Kowal, Unassigned)

Tracking

unspecified
Points:
---
Bug Flags:
wanted-next +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: DUPEME, URL)

Attachments

(2 attachments, 1 obsolete attachment)

4.58 KB, patch
Brian Crowder
: review+
Details | Diff | Splinter Review
10.36 KB, application/x-javascript
Details
(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en) AppleWebKit/522.11 (KHTML, like Gecko) Version/3.0.2 Safari/522.12
Build Identifier: Firefox 2.0.0.4

Javascript Regular Expression Non-participating Capture Groups are inaccurate in edge cases.  Steven Levithan wrote up a cross-browser test matrix comparing browser behaviors for edge case regular expressions to his interpretation of the ECMA specified behavior.  For details see his blog post:
http://blog.stevenlevithan.com/archives/npcg-javascript

Steve Levithan notes, in particular, that these cases fail for Firefox:

Input: "y".split(/(x)?\1y/)
Oracle:  ["", undefined, ""]
Actual: ["", "", ""]

Input: "y".split(/(x)?y/)
Oracle:  ["", undefined, ""]
Actual: ["", "", ""]

Input: "y".replace(/(x)?\1y/, function($0, $1){ return String($1); })
Oracle: "undefined"
Actual: ""

Input: "y".replace(/(x)?y/, function($0, $1){ return String($1); })
Oracle: "undefined"
Actual: ""

Input: "y".replace(/(x)?y/, function($0, $1){ return $1; })
Oracle: "undefined"
Actual: ""



Reproducible: Always
Totally sounds like a known bug crowder and mrbkap are tracking, for sure ;-).

/be
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: DUPEME

Updated

9 years ago
Status: NEW → ASSIGNED

Updated

9 years ago
Assignee: general → crowder
Status: ASSIGNED → NEW

Updated

9 years ago
Duplicate of this bug: 389717

Comment 3

9 years ago
Created attachment 313880 [details] [diff] [review]
patch

Actually no RegExp bug, but bugs in String.prototype.replace and String.prototype.split.

Patch passes the test suite.
Assignee: crowder → x00000000
Status: NEW → ASSIGNED
Attachment #313880 - Flags: review?(brendan)

Comment 4

9 years ago
MochiTests on this would be nice add'l info.  I'm going to steal the review from Brendan on this, though, in the hopes that he won't mind.  :)

Comment 5

9 years ago
Comment on attachment 313880 [details] [diff] [review]
patch

Looks good to me, thanks.  Will land conditional upon a1.9+ and mochitest results.  If you haven't got time to run the mochitests, please let me know.
Attachment #313880 - Flags: review?(brendan)
Attachment #313880 - Flags: review+
Attachment #313880 - Flags: approval1.9?

Comment 6

9 years ago
I don't know how to run the mochitests yet.
Assuming you have a Mozilla tree and Firefox built, just run:

python $objdir/_tests/testing/mochitest/runtests.py --autorun

...where $objdir is the value for MOZ_OBJDIR, if you set it, and otherwise is /probably/ just your source directory (but I'm not entirely certain, to be honest).

Comment 8

9 years ago
Thanks, Jeff, that's easy and also doesn't take too long (20 minutes). Mochitests passed.

There are (at least) 2 more instances in the code where an empty string for non-matched parens is returned: The $n replacements in String.prototype.replace and the global RegExp.$n properties.

In String.prototype.replace that behavior is accordingly to the spec, and the global RegExp properties are non-standard. Konqueror and Opera have also an empty string there (don't know about IE; Konqueror doesn't have the properties until the first use of a RegExp).

BTW, RegExp.$n goes only up to RegExp.$9. Opera has also only the first 9 paren matches as global properties, but Konqueror even has RegExp["$1.8e19"] (limit is slightly less than 2^64).
Wanted for the next time -- the day before RC1 freeze isn't the time to be taking these non-blocker non-regression wins, delicious though they look.
Flags: wanted-next+

Comment 10

9 years ago
Comment on attachment 313880 [details] [diff] [review]
patch

Per discussion with shaver, deferring until post-1.9 (like, 1.9.1)
Attachment #313880 - Flags: approval1.9?
Comment on attachment 313880 [details] [diff] [review]
patch

a-: non-regression non-blocker to repair edge case.  dot next will enjoy this much more.
I should also add that it would be great to get that edge-case test set into a suite somewhere.

Comment 13

9 years ago
bclary:  Would be great to get this known-failure into js/tests, if you can lend a hand.
Flags: in-testsuite?

Comment 14

9 years ago
crowder, Sure. I have a major update to go in tonight. I'll do this one after that lands.

Comment 15

9 years ago
Created attachment 314767 [details]
String.prototype.replace testcase for tests/ecma_3/String/15.5.4.11.js

This test fails also for bug 427733 and bug 427823, which I found while writing it. It doesn't test the split case.

tests/ecma_2/String/replace-001.js seems to be obsolete; String.prototype.replace wasn't in ECMAScript 2 final.

Code like this isn't exactly an edge case:

  str.replace(/(\d+)|Infinity|NaN/g,
              function (x, p) { return p != null ? +p + 1 : "???" });

But you can easily work around it by writing

  str.replace(/(\d+)|Infinity|NaN/g,
              function (x, p) { return p ? +p + 1 : "???" });

instead (that's what I do anyways), and most cases where you would need to compare explicitly against null like

  str.replace(/\s+($)?/g, function (x, p) { return p == null ? " " : "" });

don't work per spec (they do in Perl).

Comment 16

9 years ago
Created attachment 314788 [details]
3 more tests
Attachment #314767 - Attachment is obsolete: true

Comment 17

9 years ago
x00000000:  Thanks for the tests here!

Comment 18

9 years ago
/cvsroot/mozilla/js/tests/ecma_3/String/regress-392378.js,v  <--  regress-392378.js
initial revision: 1.1

Checking in ecma_3/String/15.5.4.11.js;
/cvsroot/mozilla/js/tests/ecma_3/String/15.5.4.11.js,v  <--  15.5.4.11.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/public-failures.txt,v  <--  public-failures.txt
new revision: 1.65; previous revision: 1.64

Flags: in-testsuite?
Flags: in-testsuite+
Flags: in-litmus-

Updated

9 years ago
Duplicate of this bug: 435990

Comment 20

8 years ago
back into the pool.
Assignee: x00000000 → general
Status: ASSIGNED → NEW
Marking as duplicate of bug 369778. That one doesn't have a patch, but it is very slightly newer and has current-era discussion going on.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 369778
You need to log in before you can comment on or make changes to this bug.