Last Comment Bug 635017 - /undefined/.match() should succeed
: /undefined/.match() should succeed
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla8
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
: 668934 (view as bug list)
Depends on:
Blocks: test262 es5
  Show dependency treegraph
 
Reported: 2011-02-17 13:05 PST by Jason Orendorff [:jorendorff]
Modified: 2011-09-29 00:40 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove ancient behavior and make regexp functions ES5 compliant (15.47 KB, patch)
2011-05-11 10:21 PDT, Tom Schuster [:evilpie]
no flags Details | Diff | Splinter Review
v2 (18.14 KB, patch)
2011-07-19 12:12 PDT, Tom Schuster [:evilpie]
jorendorff: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2011-02-17 13:05:18 PST
We fail http://test262.ecmascript.org/ test S15.10.6.2_A1_T16.

/blah/.match() should be the same as /blah/.match("undefined").
Instead we throw a SyntaxError.

As far as I can tell from the ES5 spec, the test is right and we're wrong.
http://people.mozilla.org/~jorendorff/es5.html#sec-15.10.6.2
Comment 1 Jason Orendorff [:jorendorff] 2011-02-17 13:24:02 PST
We fail S15.10.6.3_A1_T16 for the same reason.
Comment 2 Brendan Eich [:brendan] 2011-02-17 13:25:28 PST
RegExps have no match method -- do you mean exec? We do the same as Chrome and Safari on /blah/.exec().

/be
Comment 3 Jason Orendorff [:jorendorff] 2011-02-17 13:55:40 PST
Oops. I meant /blah/.exec(). Also /blah/.test(). IE9 and Chrome both get this right. Safari throws an Error rather than a SyntaxError.

Here are the tests:


Test S15.10.6.2_A1_T16

Description

    RegExp is /undefined/ and call exec() without arguments

Testcase

function testcase() {
    __re = /undefined/.exec()[0];
    if (__re !== "undefined") {
        $ERROR("#1: /undefined/.exec()[0] === \"undefined\". Actual: " + __re);
    }
}


Test S15.10.6.3_A1_T16

Description

    RegExp is /undefined/ and call test() without arguments

Testcase

function testcase() {
    __re = /undefined/;
    if (__re.test() !== (__re.exec() !== null)) {
        $ERROR("#0: __re = /undefined/; __re.test() === (__re.exec() !== null)");
    }
}
Comment 4 Tom Schuster [:evilpie] 2011-04-26 08:26:28 PDT
I would need to remove this non-standard feature, that calling test/exec with no arguments uses the previous input. Otherwise we could come up with cases where we either match 'undefined' or on the previous input.

From what i see this isn't documented, and fwiw i didn't even know we had such an extension.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-26 09:51:50 PDT
test262 is hitting this?  I think that means we fix, and remove the old stuff if necessary.
Comment 6 Tom Schuster [:evilpie] 2011-04-26 10:56:06 PDT
Yes. 

Summary:
  String.prototype.test(regexp)
  String.prototype.match(regexp)
     Does new RegExp(regexp), so regexp of undefined => "" (empty string)
     |this| of undefined would throw
  String.prototype.split(separator, limit)
     Undefined separator just returns [|this|]
  String.prototype.replace(searchValue, replaceValue)
     If search value isn't a regexp, it's just ToString(searchValue), so 'undefined'

  new RegExp(pattern, flags)
     undefined pattern => ""
  Regexp.prototype.match(string)
  Regexp.prototype.test(string)
     Just ToString(string), so "undefined"
Comment 7 Tom Schuster [:evilpie] 2011-04-26 11:11:15 PDT
Sorry made some mistakes.
Summary:
  String.prototype.search(regexp)
  String.prototype.match(regexp)
     Does new RegExp(regexp), so regexp of undefined => "" (empty string)
     |this| of undefined would throw
  String.prototype.split(separator, limit)
     Undefined separator just returns [|this|]
  String.prototype.replace(searchValue, replaceValue)
     If search value isn't a regexp, it's just ToString(searchValue), so
'undefined'

  new RegExp(pattern, flags)
     undefined pattern => ""
  RegExp.prototype.exec(string)
  RegExp.prototype.test(string)
     Just ToString(string), so "undefined"
Comment 8 Tom Schuster [:evilpie] 2011-05-11 10:21:45 PDT
Created attachment 531669 [details] [diff] [review]
remove ancient behavior and make regexp functions ES5 compliant
Comment 9 Jason Orendorff [:jorendorff] 2011-05-19 09:14:41 PDT
Comment on attachment 531669 [details] [diff] [review]
remove ancient behavior and make regexp functions ES5 compliant

Review of attachment 531669 [details] [diff] [review]:
-----------------------------------------------------------------

Drive-by mini-review comment...

::: js/src/jsregexp.cpp
@@ +650,4 @@
>          input = js_ValueToString(cx, vp[2]);
>          if (!input)
>              return false;
>          vp[2] = StringValue(input);

This can be shortened a little bit. Start like this:

    JSString *input = js_ValueToString(cx, argc > 0 ?  vp[2] : UndefinedValue());

Don't bother assigning input back to vp[2]. Then change RegExp::executeInternal to anchor input just before getting input->chars(), around line 347 of jsregexpinlines.h.
Comment 10 Jason Orendorff [:jorendorff] 2011-05-19 09:19:02 PDT
Here's the code I meant to quote.

643	 
649	/* Step 2. */		644	/* Step 2. */
650	JSString *input;		645	JSString *input;
651	if (argc > 0) {		646	if (argc == 0) {
			647	input = cx->runtime->atomState.typeAtoms[JSTYPE_VOID];
			648	}
			649	else {
652	input = js_ValueToString(cx, vp[2]);		650	input = js_ValueToString(cx, vp[2]);
653	if (!input)		651	if (!input)
654	return false;		652	return false;
655	vp[2] = StringValue(input);		653	vp[2] = StringValue(input);
Comment 11 Jason Orendorff [:jorendorff] 2011-05-19 09:19:51 PDT
I love Bugzilla.
Comment 12 Tom Schuster [:evilpie] 2011-07-19 12:12:28 PDT
Created attachment 546863 [details] [diff] [review]
v2

So i would have done this a lot earlier, but i remembered that, i said you should stop reviewing, because of something i wanted to change, but i just can't figure out what it was. So i tried to get into the code again and except some null check, i didn't find anything. This scares me a bit :/
Comment 13 Tom Schuster [:evilpie] 2011-07-19 13:33:35 PDT
*** Bug 668934 has been marked as a duplicate of this bug. ***
Comment 14 Jason Orendorff [:jorendorff] 2011-07-22 06:49:47 PDT
Comment on attachment 546863 [details] [diff] [review]
v2

Review of attachment 546863 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with one minor quibble. I'll change it and push.

::: js/src/jsregexpinlines.h
@@ +346,3 @@
>      JSLinearString *input = inputstr->ensureLinear(cx);
>      if (!input)
>          return false;

This anchors the wrong thing. Since you're going to call input->chars(), you need to anchor input, not inputstr. The purpose of the anchor is to protect those chars from GC.
Comment 17 dynamis (Tomoya ASAI) 2011-09-27 00:45:07 PDT
I believe it's just typo but not 100% sure and let me ask here:

(In reply to Eric Shepherd [:sheppy] from comment #16)
> evilpie updated the docs and I did some cleanup:
> 
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/RegExp/
> exec
> https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/RegExp/
> test

You wrote:
 ... it would match against the value of the RegExp *property* input ...
But it should be
 ... it would match against the value of the RegExp *previous* input ...

right?
Comment 18 Tom Schuster [:evilpie] 2011-09-27 04:06:17 PDT
Well previous input == input property.
Comment 19 dynamis (Tomoya ASAI) 2011-09-29 00:40:12 PDT
Ahh, you mean the deprecated RegExp.input property.
https://developer.mozilla.org/en/JavaScript/Reference/Deprecated_Features

Thanks and I've updated the documents clearer:
 ... match against the value of the previous input (RegExp.input property) ...

Note You need to log in before you can comment on or make changes to this bug.