Closed Bug 635005 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -

People

(Reporter: jorendorff, Unassigned)

References

Details

(Whiteboard: [has patch])

Attachments

(2 files, 2 obsolete files)

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/.
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".
Blocks: es5
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
I think these tests are from Sputnik...
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.
Attached patch patch (obsolete) — Splinter Review
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.)
Attached file Test (obsolete) —
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?)
Attached patch PatchSplinter Review
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)
Attachment #513593 - Attachment is obsolete: true
Attached file 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
Attachment #513880 - Attachment is obsolete: true
Attachment #513881 - Flags: review?(cdleary)
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.
Attachment #513881 - Flags: review?(cdleary) → review+
This seems like something we want to get into a dot release if those tests are high profile.
blocking2.0: --- → ?
Whiteboard: [has patch]
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
Attachment #513881 - Flags: approval2.0?
Attachment #513881 - Flags: approval2.0? → approval2.0+
Has approval. Not blocking.
blocking2.0: ? → -
(In reply to comment #8)
> Created attachment 513898 [details]

haytjes, do I have your permission to release this into the public domain?
(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 ;-)
http://hg.mozilla.org/mozilla-central/rev/bea6dacd47f9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 394846
You need to log in before you can comment on or make changes to this bug.