/undefined/.match() should succeed

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: jorendorff, Assigned: evilpie)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Other Branch
mozilla8
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
We fail S15.10.6.3_A1_T16 for the same reason.
RegExps have no match method -- do you mean exec? We do the same as Chrome and Safari on /blah/.exec().

/be
Blocks: 445494
(Reporter)

Comment 3

7 years ago
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)");
    }
}
Assignee: general → evilpies
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.
Status: NEW → ASSIGNED
Blocks: 652780
test262 is hitting this?  I think that means we fix, and remove the old stuff if necessary.
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"
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"
Created attachment 531669 [details] [diff] [review]
remove ancient behavior and make regexp functions ES5 compliant
Attachment #531669 - Flags: review?(jorendorff)
Attachment #531669 - Flags: review?(jorendorff)
(Reporter)

Comment 9

6 years ago
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.
(Reporter)

Comment 10

6 years ago
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);
(Reporter)

Comment 11

6 years ago
I love Bugzilla.
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 :/
Attachment #531669 - Attachment is obsolete: true
Duplicate of this bug: 668934
Attachment #546863 - Flags: review?(jorendorff)
(Reporter)

Comment 14

6 years ago
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.
Attachment #546863 - Flags: review?(jorendorff) → review+
http://hg.mozilla.org/mozilla-central/rev/54438ea37d53
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Keywords: dev-doc-needed
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.
Keywords: dev-doc-needed → dev-doc-complete
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?
Well previous input == input property.
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) ...
You need to log in before you can comment on or make changes to this bug.