635005 - new RegExp(undefined) should work like new RegExp("")

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

new RegExp(undefined) should be treated like new RegExp(""), per ECMA 5 section 15.10.4.1:

"...Otherwise, let P be the empty String if pattern is undefined and ToString(pattern) otherwise, and let F be the empty String if flags is undefined and ToString(flags) otherwise."

Instead we return /undefined/.

Comment 1

[Jason Orendorff [:jorendorff]]

We flunk several http://test262.ecmascript.org/ tests because of this:

S15.10.4.1_A3_T2
S15.10.4.1_A3_T3
S15.10.4.1_A3_T4
S15.10.4.1_A3_T5
S15.10.4.1_A4_T2
S15.10.4.1_A4_T3

Seriously this is stupid. It's like somebody with not much imagination spent all morning trying to think of all the ways to say "undefined".

Comment 2

[Brendan Eich [:brendan]]

Is the test being gamed to score us lower than IE9? Serious question, because undefined is undefined, however you spell it. Ok, never attribute to malice...

/be

Comment 3

[Jan de Mooij [:jandem]]

I think these tests are from Sputnik...

Comment 4

[Jeff Walden [:Waldo]]

Yeah, it's a Sputnik test.  It was probably written by someone at Google poring over the spec with a fine-toothed comb.  There's nothing wrong with that, and we should converge on what the spec says, even to the point of analities like these.  It just means the test results aren't something that should be used in marketing, or should be too important for compliance with the world people generally live in.  I may push back against that in a blog post soon.

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: patch

This transforms new RegExp(undefined) into new RegExp("") like ECMA suggests.
(This patch doesn't alter the behavior when the flag is 'undefined', because I'm not sure what the current behavior is, as well as what the behavior should be.)

Comment 6

[Hannes Verschore [:h4writer]]

Attachment: Test

I noticed I didn't cover everything right,
so I wrote a small testcase to make sure I solved this bug right.

(I'm ok with this getting included to the "tests", but can somebody point me to a document how to write a decent test?)

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Patch

Updated the patch.

Only problem with this patch is that it doesn't alter the behavior of /undefined/. I think this regexp is made with some other code. Not sure where I can find that code. Also I'm not sure if the behavior of /undefined/ should get altered??

new RegExp(undefined, ...) and RegExp(undefined, ...) work now according to the ECMA spec)

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: Tests

Well I mistakenly thought that /undefined/ equals RegExp(undefined). But it actually equals RegExp("undefined"). Therefor I had a wrong test. So I removed those faulty tests and now I can confirm the patch is finished

Comment 9

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 513881 [details] [diff] [review]
Patch

r+ with style nits. Thanks for the patch!

+    if(sourceValue.isUndefined()) {
+        /*
+         * per ECMA 5 section 15.10.4.1:
+         * new RegExp(undefined) should be treated like new RegExp("")
+         */

Space after if, can probably just cite ECMA on a single line without explanation.

Comment 10

[Chris Leary [:cdleary] (not checking bugmail)]

This seems like something we want to get into a dot release if those tests are high profile.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 513881 [details] [diff] [review]
Patch

Agree on the nits (Chris, we often pick them for the contibutor instead of requiring a new patch be attached). The bug isn't a blocker but the patch is obviously safe for next release, which is still Fx4, not 4.x. I say we go for patch approval.

/be

Comment 12

[Andreas Gal :gal]

Has approval. Not blocking.

Comment 13

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #8)
> Created attachment 513898 [details]

haytjes, do I have your permission to release this into the public domain?

Comment 14

[Hannes Verschore [:h4writer]]

(In reply to comment #13)
> (In reply to comment #8)
> > Created attachment 513898 [details]
> 
> haytjes, do I have your permission to release this into the public domain?

Yeah, go ahead ;-)

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/bea6dacd47f9