635017 - /undefined/.match() should succeed

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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]]

We fail S15.10.6.3_A1_T16 for the same reason.

Comment 2

[Brendan Eich [:brendan]]

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]]

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 S [:evilpie]]

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]]

test262 is hitting this?  I think that means we fix, and remove the old stuff if necessary.

Comment 6

[Tom S [:evilpie]]

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 S [:evilpie]]

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 S [:evilpie]]

Attachment: remove ancient behavior and make regexp functions ES5 compliant

Comment 9

[Jason Orendorff [:jorendorff]]

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]]

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]]

I love Bugzilla.

Comment 12

[Tom S [:evilpie]]

Attachment: 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 14

[Jason Orendorff [:jorendorff]]

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 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/54438ea37d53

Comment 16

[Eric Shepherd [:sheppy]]

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
https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/String/search
https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/String/match

And listed on Firefox 8 for developers.

Comment 17

[dynamis (Tomoya ASAI)]

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 S [:evilpie]]

Well previous input == input property.

Comment 19

[dynamis (Tomoya ASAI)]

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) ...