Skip to content

Commit

Permalink
Further ES2015 RegExp spec compliance fixes
Browse files Browse the repository at this point in the history
- RegExp.prototype.toString() doesn't have any special handling of
  RegExp instances and simply calls the source and flags getters
- Use the original values of global and sticky, rather than based
  on the current flag getters, as specified in
  tc39/ecma262#494

[email protected],adamk
LOG=Y
BUG=v8:4602

Review URL: https://codereview.chromium.org/1846303002

Cr-Commit-Position: refs/heads/master@{#35225}
  • Loading branch information
littledan authored and Commit bot committed Apr 4, 2016
1 parent 42f2261 commit 277f5bd
Show file tree
Hide file tree
Showing 8 changed files with 69 additions and 40 deletions.
33 changes: 10 additions & 23 deletions src/js/regexp.js
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,8 @@ function RegExpSubclassExecJS(string) {
// algorithm, step 4) even if the value is discarded for non-global RegExps.
var i = TO_LENGTH(lastIndex);

var global = TO_BOOLEAN(this.global);
var sticky = TO_BOOLEAN(this.sticky);
var global = TO_BOOLEAN(REGEXP_GLOBAL(this));
var sticky = TO_BOOLEAN(REGEXP_STICKY(this));
var updateLastIndex = global || sticky;
if (updateLastIndex) {
if (i > string.length) {
Expand Down Expand Up @@ -370,27 +370,14 @@ function TrimRegExp(regexp) {


function RegExpToString() {
if (!IS_REGEXP(this)) {
// RegExp.prototype.toString() returns '/(?:)/' as a compatibility fix;
// a UseCounter is incremented to track it.
// TODO(littledan): Remove this workaround or standardize it
if (this === GlobalRegExpPrototype) {
%IncrementUseCounter(kRegExpPrototypeToString);
return '/(?:)/';
}
if (!IS_RECEIVER(this)) {
throw MakeTypeError(
kIncompatibleMethodReceiver, 'RegExp.prototype.toString', this);
}
return '/' + TO_STRING(this.source) + '/' + TO_STRING(this.flags);
if (!IS_RECEIVER(this)) {
throw MakeTypeError(
kIncompatibleMethodReceiver, 'RegExp.prototype.toString', this);
}
var result = '/' + REGEXP_SOURCE(this) + '/';
if (REGEXP_GLOBAL(this)) result += 'g';
if (REGEXP_IGNORE_CASE(this)) result += 'i';
if (REGEXP_MULTILINE(this)) result += 'm';
if (REGEXP_UNICODE(this)) result += 'u';
if (REGEXP_STICKY(this)) result += 'y';
return result;
if (this === GlobalRegExpPrototype) {
%IncrementUseCounter(kRegExpPrototypeToString);
}
return '/' + TO_STRING(this.source) + '/' + TO_STRING(this.flags);
}


Expand Down Expand Up @@ -1126,7 +1113,7 @@ function RegExpGetSource() {
// TODO(littledan): Remove this RegExp compat workaround
if (this === GlobalRegExpPrototype) {
%IncrementUseCounter(kRegExpPrototypeSourceGetter);
return UNDEFINED;
return "(?:)";
}
throw MakeTypeError(kRegExpNonRegExp, "RegExp.prototype.source");
}
Expand Down
6 changes: 3 additions & 3 deletions test/cctest/test-regexp.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1949,15 +1949,15 @@ TEST(UseCountRegExp) {
// a UseCounter is incremented to track it.
v8::Local<v8::Value> resultToString =
CompileRun("RegExp.prototype.toString().length");
CHECK_EQ(1, use_counts[v8::Isolate::kRegExpPrototypeStickyGetter]);
CHECK_EQ(2, use_counts[v8::Isolate::kRegExpPrototypeStickyGetter]);
CHECK_EQ(1, use_counts[v8::Isolate::kRegExpPrototypeToString]);
CHECK(resultToString->IsInt32());
CHECK_EQ(6,
resultToString->Int32Value(isolate->GetCurrentContext()).FromJust());

// .toString() works on normal RegExps
v8::Local<v8::Value> resultReToString = CompileRun("/a/.toString().length");
CHECK_EQ(1, use_counts[v8::Isolate::kRegExpPrototypeStickyGetter]);
CHECK_EQ(2, use_counts[v8::Isolate::kRegExpPrototypeStickyGetter]);
CHECK_EQ(1, use_counts[v8::Isolate::kRegExpPrototypeToString]);
CHECK(resultReToString->IsInt32());
CHECK_EQ(
Expand All @@ -1969,7 +1969,7 @@ TEST(UseCountRegExp) {
"try { RegExp.prototype.toString.call(null) }"
"catch (e) { exception = e; }"
"exception");
CHECK_EQ(1, use_counts[v8::Isolate::kRegExpPrototypeStickyGetter]);
CHECK_EQ(2, use_counts[v8::Isolate::kRegExpPrototypeStickyGetter]);
CHECK_EQ(1, use_counts[v8::Isolate::kRegExpPrototypeToString]);
CHECK(resultToStringError->IsObject());
}
8 changes: 7 additions & 1 deletion test/mjsunit/es6/classes-subclass-builtins.js
Original file line number Diff line number Diff line change
Expand Up @@ -919,7 +919,13 @@ function TestMapSetSubclassing(container, is_map) {

var o = Reflect.construct(RegExp, [pattern], f);
assertEquals(["match", "tostring"], log);
assertEquals(/biep/, o);
// TODO(littledan): Is the RegExp constructor correct to create
// the internal slots and do these type checks this way?
assertEquals("biep", %_RegExpSource(o));
assertThrows(() => Object.getOwnPropertyDescriptor(RegExp.prototype,
'source').get(o),
TypeError);
assertEquals("/undefined/undefined", RegExp.prototype.toString.call(o));
assertTrue(o.__proto__ === p2);
assertTrue(f.prototype === p3);
})();
19 changes: 17 additions & 2 deletions test/mjsunit/es6/regexp-constructor.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,32 +21,47 @@ function should_not_be_called() {
})();

(function() {
let allow = false;
class A extends RegExp {
get source() { throw new Error("should not be called") }
get flags() { throw new Error("should not be called") }
get source() {
if (!allow) throw new Error("should not be called");
return super.source;
}
get flags() {
if (!allow) throw new Error("should not be called");
return super.flags
}
}

var r = new A("biep");
var r2 = RegExp(r);

assertFalse(r === r2);
allow = true;
assertEquals(r, r2);
allow = false;
assertTrue(A.prototype === r.__proto__);
assertTrue(RegExp.prototype === r2.__proto__);

var r3 = RegExp(r);
assertFalse(r3 === r);
allow = true;
assertEquals(r3, r);
allow = false;

var r4 = new A(r2);
assertFalse(r4 === r2);
allow = true;
assertEquals(r4, r2);
allow = false;
assertTrue(A.prototype === r4.__proto__);

r[Symbol.match] = false;
var r5 = new A(r);
assertFalse(r5 === r);
allow = true;
assertEquals(r5, r);
allow = false;
assertTrue(A.prototype === r5.__proto__);
})();

Expand Down
7 changes: 4 additions & 3 deletions test/mjsunit/es6/regexp-flags.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,10 @@ assertEquals(4, get_count);


function testName(name) {
// TODO(littledan): For web compatibility, we don't throw an exception,
// but ES2015 expects an exception to be thrown from this getter.
assertEquals(undefined, RegExp.prototype[name]);
// Test for ES2017 RegExp web compatibility semantics
// https://github.com/tc39/ecma262/pull/511
assertEquals(name === "source" ? "(?:)" : undefined,
RegExp.prototype[name]);
assertEquals(
"get " + name,
Object.getOwnPropertyDescriptor(RegExp.prototype, name).get.name);
Expand Down
11 changes: 11 additions & 0 deletions test/mjsunit/es6/regexp-tostring.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,14 @@ testThrows(1);

assertEquals("/pattern/flags", RegExp.prototype.toString.call(fake));
assertEquals(["p", "ps", "f", "fs"], log);

// Monkey-patching is also possible on RegExp instances

let weird = /foo/;
Object.defineProperty(weird, 'flags', {value: 'bar'});
Object.defineProperty(weird, 'source', {value: 'baz'});
assertEquals('/baz/bar', weird.toString());

assertEquals('/(?:)/', RegExp.prototype.toString());
assertEquals('(?:)', RegExp.prototype.source);
assertEquals('', RegExp.prototype.flags);
23 changes: 16 additions & 7 deletions test/test262/test262.status
Original file line number Diff line number Diff line change
Expand Up @@ -100,20 +100,32 @@
'language/asi/S7.9_A5.7_T1': [PASS, FAIL_OK],

###### BEGIN REGEXP SUBCLASSING SECTION ######
# https://code.google.com/p/v8/issues/detail?id=4602
# Fails due to mismatching [[OriginalFlags]] and getters
'built-ins/RegExp/prototype/exec/get-sticky-coerce': [FAIL],
# Spec change in progress https://github.com/tc39/ecma262/pull/494
# RegExpBuiltinMatch reads flags from [[OriginalFlags]]
'built-ins/RegExp/prototype/Symbol.match/builtin-coerce-sticky': [FAIL],
'built-ins/RegExp/prototype/Symbol.match/builtin-get-global-err': [FAIL],
'built-ins/RegExp/prototype/Symbol.match/builtin-get-sticky-err': [FAIL],
'built-ins/RegExp/prototype/Symbol.match/builtin-success-g-set-lastindex': [FAIL],
'built-ins/RegExp/prototype/Symbol.match/builtin-success-g-set-lastindex-err': [FAIL],
'built-ins/RegExp/prototype/Symbol.match/coerce-sticky': [FAIL],
'built-ins/RegExp/prototype/Symbol.replace/get-sticky-coerce': [FAIL],
'built-ins/RegExp/prototype/Symbol.match/get-sticky-err': [FAIL],
'built-ins/RegExp/prototype/Symbol.replace/coerce-global': [FAIL],
'built-ins/RegExp/prototype/Symbol.replace/coerce-unicode': [FAIL],
'built-ins/RegExp/prototype/Symbol.replace/get-sticky-coerce': [FAIL],
'built-ins/RegExp/prototype/Symbol.replace/get-sticky-err': [SKIP],
'built-ins/RegExp/prototype/Symbol.search/get-sticky-coerce': [FAIL],
'built-ins/RegExp/prototype/Symbol.search/get-sticky-err': [FAIL],
'built-ins/RegExp/prototype/exec/get-sticky-coerce': [FAIL],
'built-ins/RegExp/prototype/exec/get-sticky-err': [FAIL],
'built-ins/RegExp/prototype/test/get-sticky-err': [FAIL],

# Missing lastIndex support
'built-ins/RegExp/prototype/Symbol.split/str-result-coerce-length-err': [FAIL],

# Times out
'built-ins/RegExp/prototype/Symbol.split/str-coerce-lastindex': [SKIP],
'built-ins/RegExp/prototype/Symbol.match/coerce-global': [SKIP],
'built-ins/RegExp/prototype/Symbol.match/builtin-coerce-global': [SKIP],

# Sticky support busted
'built-ins/RegExp/prototype/Symbol.replace/y-init-lastindex': [FAIL],
Expand All @@ -123,9 +135,6 @@
# happens to be thrown for some other reason.
'built-ins/RegExp/prototype/Symbol.split/str-result-get-length-err': [SKIP],

# Spec change in progress: https://github.com/tc39/ecma262/issues/489
'built-ins/RegExp/prototype/Symbol.replace/get-sticky-err': [SKIP],
#
###### END REGEXP SUBCLASSING SECTION ######

# https://code.google.com/p/v8/issues/detail?id=4360
Expand Down
2 changes: 1 addition & 1 deletion test/webkit/fast/regex/toString-expected.txt
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE

PASS RegExp('/').source is "\\/"
PASS RegExp('').source is "(?:)"
FAIL RegExp.prototype.source should be (?:) (of type string). Was undefined (of type undefined).
PASS RegExp.prototype.source is "(?:)"
PASS RegExp('/').toString() is "/\\//"
PASS RegExp('').toString() is "/(?:)/"
PASS RegExp.prototype.toString() is "/(?:)/"
Expand Down

0 comments on commit 277f5bd

Please sign in to comment.