Implement the `RegExp.prototype.flags` getter

Implement the `RegExp.prototype.flags` getter

TEST=mjsunit/harmony
BUG=v8:3751
LOG=N

[mathias, 2014-12-09 22:50:47]

mathias@qiwi.be changed reviewers:
+ arv@chromium.org, mathias@qiwi.be

[mathias, 2014-12-09 22:50:48]

PTAL

https://codereview.chromium.org/770333005/diff/1/src/regexp.js
File src/regexp.js (right):

https://codereview.chromium.org/770333005/diff/1/src/regexp.js#newcode456
src/regexp.js:456: //}
Uncommenting the `if` statement breaks the build. Am I doing something crazy
here?

[Dmitry Lomov (no reviews), 2014-12-09 23:01:59]

dslomov@chromium.org changed reviewers:
+ dslomov@chromium.org

[Dmitry Lomov (no reviews), 2014-12-09 23:02:00]

https://codereview.chromium.org/770333005/diff/1/src/regexp.js
File src/regexp.js (right):

https://codereview.chromium.org/770333005/diff/1/src/regexp.js#newcode452
src/regexp.js:452: //if (harmony_regexps) {
harmony_regexps should not be used in bootstrapping code (code that executes on
loading natives).
[other uses in this file are ok since they are inside function bodies]

What we typically do is create a separate file, harmony-regexp.js, that is
loaded conditionally under FLAG_harmony_regexp.

[mathias, 2014-12-09 23:23:36]

On 2014/12/09 23:02:00, Dmitry Lomov (chromium) wrote:
> https://codereview.chromium.org/770333005/diff/1/src/regexp.js
> File src/regexp.js (right):
> 
> https://codereview.chromium.org/770333005/diff/1/src/regexp.js#newcode452
> src/regexp.js:452: //if (harmony_regexps) {
> harmony_regexps should not be used in bootstrapping code (code that executes
on
> loading natives).
> [other uses in this file are ok since they are inside function bodies]
> 
> What we typically do is create a separate file, harmony-regexp.js, that is
> loaded conditionally under FLAG_harmony_regexp.

Thanks for the pointers, Dmitry and Erik (IRC)! Patch updated.

[mathias, 2014-12-09 23:26:22]

https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
File test/mjsunit/harmony/regexp-flags.js (right):

https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
test/mjsunit/harmony/regexp-flags.js:27: .get.call(object);
Nit: +2 indent

Will fix in the next patch, along with any feedback.

[Dmitry Lomov (no reviews), 2014-12-09 23:39:13]

https://codereview.chromium.org/770333005/diff/20001/src/harmony-classes.js
File src/harmony-classes.js (right):

https://codereview.chromium.org/770333005/diff/20001/src/harmony-classes.js#n...
src/harmony-classes.js:21: ['Function.prototype.toMethod',
Looks like this is from some other patch?

https://codereview.chromium.org/770333005/diff/20001/src/harmony-regexp.js
File src/harmony-regexp.js (right):

https://codereview.chromium.org/770333005/diff/20001/src/harmony-regexp.js#ne...
src/harmony-regexp.js:17: throw MakeTypeError('home_object_non_object',
Wrong error message.

https://codereview.chromium.org/770333005/diff/20001/src/harmony-regexp.js#ne...
src/harmony-regexp.js:30: %DefineAccessorPropertyUnchecked($RegExp.prototype,
'flags', RegExpGetFlags,
You should pass 'undefined' here instead of NoOpSetter.
Regexp.prototype.flags do not have a setter.

https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
File test/mjsunit/harmony/regexp-flags.js (right):

https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
test/mjsunit/harmony/regexp-flags.js:5: // Flags: --harmony-regexps
Also add test that validates properties of Regexp.prototype.flag from its
property descriptor (has getter, has no setter)

[arv (Not doing code reviews), 2014-12-10 02:54:50]

https://codereview.chromium.org/770333005/diff/20001/src/harmony-regexp.js
File src/harmony-regexp.js (right):

https://codereview.chromium.org/770333005/diff/20001/src/harmony-regexp.js#ne...
src/harmony-regexp.js:15: var RegExpGetFlags = function() {
Move this out of ExtendRegExpPrototype.

https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
File test/mjsunit/harmony/regexp-flags.js (right):

https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
test/mjsunit/harmony/regexp-flags.js:7: assertEquals('',
RegExp.prototype.flags);
Shouldn't this throw since the prototype object does not have [[OriginalFlags]]?

I guess this is a really a bug in our implementation of RegExp.prototype.global.
But once/if we fix that then this test will start to fail.

https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
test/mjsunit/harmony/regexp-flags.js:8: RegExp.prototype.flags = 'foo';
A better test is to get the descriptor and validate it.

[mathias, 2014-12-10 08:23:31]

https://codereview.chromium.org/770333005/diff/20001/src/harmony-classes.js
File src/harmony-classes.js (right):

https://codereview.chromium.org/770333005/diff/20001/src/harmony-classes.js#n...
src/harmony-classes.js:21: ['Function.prototype.toMethod',
On 2014/12/09 23:39:12, Dmitry Lomov (chromium) wrote:
> Looks like this is from some other patch?

It was intentional; I wanted to re-use the same error message. Updated the patch
now to use a new, separate error message.

https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
File test/mjsunit/harmony/regexp-flags.js (right):

https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
test/mjsunit/harmony/regexp-flags.js:7: assertEquals('',
RegExp.prototype.flags);
On 2014/12/10 02:54:50, arv wrote:
> Shouldn't this throw since the prototype object does not have
[[OriginalFlags]]?
> 
> I guess this is a really a bug in our implementation of
RegExp.prototype.global.
> But once/if we fix that then this test will start to fail.

Ack. What do you want to do here – fix the separate bug later and leave the test
for now? Or just remove this test?

[Dmitry Lomov (no reviews), 2014-12-10 11:11:18]

On 2014/12/10 08:23:31, mathias wrote:
> https://codereview.chromium.org/770333005/diff/20001/src/harmony-classes.js
> File src/harmony-classes.js (right):
> 
>
https://codereview.chromium.org/770333005/diff/20001/src/harmony-classes.js#n...
> src/harmony-classes.js:21: ['Function.prototype.toMethod',
> On 2014/12/09 23:39:12, Dmitry Lomov (chromium) wrote:
> > Looks like this is from some other patch?
> 
> It was intentional; I wanted to re-use the same error message. Updated the
patch
> now to use a new, separate error message.
> 
>
https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
> File test/mjsunit/harmony/regexp-flags.js (right):
> 
>
https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
> test/mjsunit/harmony/regexp-flags.js:7: assertEquals('',
> RegExp.prototype.flags);
> On 2014/12/10 02:54:50, arv wrote:
> > Shouldn't this throw since the prototype object does not have
> [[OriginalFlags]]?
> > 
> > I guess this is a really a bug in our implementation of
> RegExp.prototype.global.
> > But once/if we fix that then this test will start to fail.
> 
> Ack. What do you want to do here – fix the separate bug later and leave the
test
> for now? Or just remove this test?

Remove the test. It is incorrect per spec.
What you really want to test here is subclasses of RegExp but we do not support
that currently.

[Dmitry Lomov (no reviews), 2014-12-10 11:25:08]

On 2014/12/10 11:11:18, Dmitry Lomov (chromium) wrote:
> On 2014/12/10 08:23:31, mathias wrote:
> > https://codereview.chromium.org/770333005/diff/20001/src/harmony-classes.js
> > File src/harmony-classes.js (right):
> > 
> >
>
https://codereview.chromium.org/770333005/diff/20001/src/harmony-classes.js#n...
> > src/harmony-classes.js:21: ['Function.prototype.toMethod',
> > On 2014/12/09 23:39:12, Dmitry Lomov (chromium) wrote:
> > > Looks like this is from some other patch?
> > 
> > It was intentional; I wanted to re-use the same error message. Updated the
> patch
> > now to use a new, separate error message.
> > 
> >
>
https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
> > File test/mjsunit/harmony/regexp-flags.js (right):
> > 
> >
>
https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
> > test/mjsunit/harmony/regexp-flags.js:7: assertEquals('',
> > RegExp.prototype.flags);
> > On 2014/12/10 02:54:50, arv wrote:
> > > Shouldn't this throw since the prototype object does not have
> > [[OriginalFlags]]?
> > > 
> > > I guess this is a really a bug in our implementation of
> > RegExp.prototype.global.
> > > But once/if we fix that then this test will start to fail.
> > 
> > Ack. What do you want to do here – fix the separate bug later and leave the
> test
> > for now? Or just remove this test?
> 
> Remove the test. It is incorrect per spec.
> What you really want to test here is subclasses of RegExp but we do not
support
> that currently.

[Sorry that came out unclear:
- the test asserts the behavior that is contrary to the spec, so should be
removed
- subclassing comment is separate, not related to assertEquals('',
RegExp.prototype.flags line]

[mathias, 2014-12-10 12:16:43]

On 2014/12/10 11:25:08, Dmitry Lomov (chromium) wrote:
>
https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
> > > test/mjsunit/harmony/regexp-flags.js:7: assertEquals('',
> > > RegExp.prototype.flags);
> > > On 2014/12/10 02:54:50, arv wrote:
> > > > Shouldn't this throw since the prototype object does not have
> > > [[OriginalFlags]]?
> > > > 
> > > > I guess this is a really a bug in our implementation of
> > > RegExp.prototype.global.
> > > > But once/if we fix that then this test will start to fail.
> > > 
> > > Ack. What do you want to do here – fix the separate bug later and leave
the
> > test
> > > for now? Or just remove this test?
> > 
> > Remove the test. It is incorrect per spec.
> > What you really want to test here is subclasses of RegExp but we do not
> support
> > that currently.
> 
> [Sorry that came out unclear:
> - the test asserts the behavior that is contrary to the spec, so should be
> removed
> - subclassing comment is separate, not related to assertEquals('',
> RegExp.prototype.flags line]

I’m confused. The spec for `RegExp.prototype.global` says:

> 3. If R does not have an [[OriginalFlags]] internal slot throw a TypeError
exception.

But `RegExp.prototype.flags` doesn’t seem to have such a restriction. What makes
you say this should throw?

[Dmitry Lomov (no reviews), 2014-12-10 12:38:17]

On 2014/12/10 12:16:43, mathias wrote:
> On 2014/12/10 11:25:08, Dmitry Lomov (chromium) wrote:
> >
>
https://codereview.chromium.org/770333005/diff/20001/test/mjsunit/harmony/reg...
> > > > test/mjsunit/harmony/regexp-flags.js:7: assertEquals('',
> > > > RegExp.prototype.flags);
> > > > On 2014/12/10 02:54:50, arv wrote:
> > > > > Shouldn't this throw since the prototype object does not have
> > > > [[OriginalFlags]]?
> > > > > 
> > > > > I guess this is a really a bug in our implementation of
> > > > RegExp.prototype.global.
> > > > > But once/if we fix that then this test will start to fail.
> > > > 
> > > > Ack. What do you want to do here – fix the separate bug later and leave
> the
> > > test
> > > > for now? Or just remove this test?
> > > 
> > > Remove the test. It is incorrect per spec.
> > > What you really want to test here is subclasses of RegExp but we do not
> > support
> > > that currently.
> > 
> > [Sorry that came out unclear:
> > - the test asserts the behavior that is contrary to the spec, so should be
> > removed
> > - subclassing comment is separate, not related to assertEquals('',
> > RegExp.prototype.flags line]
> 
> I’m confused. The spec for `RegExp.prototype.global` says:
> 
> > 3. If R does not have an [[OriginalFlags]] internal slot throw a TypeError
> exception.
> 
> But `RegExp.prototype.flags` doesn’t seem to have such a restriction. What
makes
> you say this should throw?

`flags` call `this.global` and that throws since `this` in this case is
RegExp.prototype.

[mathias, 2014-12-10 13:00:57]

On 2014/12/10 12:38:17, Dmitry Lomov (chromium) wrote:
> On 2014/12/10 12:16:43, mathias wrote:
> > I’m confused. The spec for `RegExp.prototype.global` says:
> > 
> > > 3. If R does not have an [[OriginalFlags]] internal slot throw a TypeError
> > exception.
> > 
> > But `RegExp.prototype.flags` doesn’t seem to have such a restriction. What
> makes
> > you say this should throw?
> 
> `flags` call `this.global` and that throws since `this` in this case is
> RegExp.prototype.

Ah, of course! Test removed.

Since none of the major ECMAScript engines seem to implement step 3 of
`RegExp.prototype.global`’s algorithm (and similar for the other
`RegExp.prototype` getters) maybe the spec should just be changed to reflect
reality. I’ve filed https://bugs.ecmascript.org/show_bug.cgi?id=3432 for that.

[Dmitry Lomov (no reviews), 2014-12-10 16:04:44]

lgtm % small nit

https://codereview.chromium.org/770333005/diff/80001/test/mjsunit/harmony/reg...
File test/mjsunit/harmony/regexp-flags.js (right):

https://codereview.chromium.org/770333005/diff/80001/test/mjsunit/harmony/reg...
test/mjsunit/harmony/regexp-flags.js:15: // When support for the `u` flag is
added, uncomment the first line below and
Please add a TODO here. Feel free to put me as an owner of this TODO
(TODO(dslomov)

[Dmitry Lomov (no reviews), 2014-12-10 16:06:29]

I am sorry I have seconds thoughts :) 
still lgtm but with couple more nits - sorry.

https://codereview.chromium.org/770333005/diff/80001/src/harmony-regexp.js
File src/harmony-regexp.js (right):

https://codereview.chromium.org/770333005/diff/80001/src/harmony-regexp.js#ne...
src/harmony-regexp.js:4: 
style nit: 'use strict'

https://codereview.chromium.org/770333005/diff/80001/src/harmony-regexp.js#ne...
src/harmony-regexp.js:11: var RegExpGetFlags = function() {
style nit: just use function RegExpGetFlags here.

[mathias, 2014-12-10 20:06:37]

Done. Thanks for the thorough review!

[Dmitry Lomov (no reviews), 2014-12-10 20:13:18]

The CQ bit was checked by dslomov@chromium.org

[commit-bot: I haz the power, 2014-12-10 20:13:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/770333005/100001

[commit-bot: I haz the power, 2014-12-10 20:41:04]

Committed patchset #6 (id:100001)

[arv (Not doing code reviews), 2014-12-10 21:03:28]

LGTM

I think the tests could have been a bit more extensive but this is good enough.

[mathias, 2014-12-13 07:04:28]

On 2014/12/10 21:03:28, arv wrote:
> I think the tests could have been a bit more extensive but this is good
enough.

What kind of additional tests did you have in mind? I’ll do a follow-up patch
that fixes the configurability soon and I’d love to add some more tests.

Re: configurability, I assumed
https://people.mozilla.org/~jorendorff/es6-draft.html#table-4 applied but
actually the last paragraph of section 17 overrules this:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ecmascript-standard...