1120169 - Implement RegExp.prototype.{global, ignoreCase, multiline, source, sticky, unicode}

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[ziyunfei]

RegExp global, ignoreCase, multiline, source, sticky are now prototype accessor properties rather than instance own data properties.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Implement RegExp.prototype.{global, ignoreCase, multiline, source, sticky, unicode}.

Moved those properties to RegExp.prototype, and made them accessors.

RegExp.prototype.unicode is added, but it always returns false, until /u flag is supported.

Also, following behaviors are changed by adding EscapeRegExpPattern:
* `RegExp().source` returns '(?:)' instead of ''
* `RegExp("\r\n\u2028\u2029").source` returns '\\r\\n\\u2028\\u2029' instead of '\r\n\u2028\u2029'
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-escaperegexppattern

Green on try runs:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ff84a7c3b30 (this patch)
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c30610a063fb (this patch and the patch for bug 1079919)

Comment 2

[Till Schneidereit [:till]]

Comment on attachment 8560919 [details] [diff] [review]
Implement RegExp.prototype.{global, ignoreCase, multiline, source, sticky, unicode}.

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

Thanks for the patch and the very nice tests!

There are, however, a few issues with the patch. First of all, it really should be multiple patches: the one for this bug, which just changes the flags to accessors, then one for changing the result of .source to "(?:)", and then a third one.

That third one, however, isn't required or even correct: we already escape the result of .source in the right way. You can test this by doing something like the following:
var re1 = new RegExp("\r\n\u2028\u2029");
var re2 = new RegExp(re1.source);
print(re1.source === re2.source);

This prints `true` with the current code, but would print `false` with your change. Also, new RegExp(/\r\n\u2028\u2029/).source gives the result you expect, with double-\\s.

The changes to the flags look great, and so does the change to "(?:)". The most important reason why I'd like to see the latter split out into its own bug is that it is potentially a breaking change: there might be Firefox-specific code that somehow depends on us returning an empty string there. I hope there isn't, but it's uncertain, so we should land that change on its own.

Please ask for a re-review for the two patches. Should be a very quick r+ for both.

::: js/src/tests/ecma_2/RegExp/function-001.js
@@ +35,1 @@
>  AddTestCase(

Please just remove this entire file. We don't need multiple identical copies of the same test, and this is identical to constructor-001.js

::: js/src/tests/ecma_5/Object/15.2.3.3-01.js
@@ +266,5 @@
>  /******************************************************************************/
>  
>  o = /foo/im;
>  
> +/* They are no more instance data property in ES6.

Please remove the tests entirely. Keeping them around in disabled form doesn't serve a purpose that a version control system wouldn't serve better.

::: js/src/tests/ecma_5/Object/15.2.3.4-04.js
@@ +16,5 @@
>   **************/
>  
>  var actual = Object.getOwnPropertyNames(/a/);
> +var expected = ["lastIndex"];
> +// source, global, ignoreCase, multiline are no more instance property in ES6.

Remove this comment. Tests don't need to inform about things that were different in earlier specs.

::: js/src/tests/ecma_5/RegExp/instance-property-storage-introspection.js
@@ +53,5 @@
>  
>    expect = { value: lastIndex, enumerable: false, configurable: false, writable: true };
>    checkDataProperty(r, "lastIndex", expect, msg);
>  
> +  /* They are no more instance data property in ES6.

Please remove the tests entirely. Keeping them around in disabled form doesn't serve a purpose that a version control system wouldn't serve better.

::: js/src/tests/ecma_5/strict/15.10.7.js
@@ +4,5 @@
>   * Any copyright is dedicated to the Public Domain.
>   * http://creativecommons.org/licenses/publicdomain/
>   */
>  
> +/* They are no more instance data property in ES6.

Please remove the tests entirely. Keeping them around in disabled form doesn't serve a purpose that a version control system wouldn't serve better.

::: js/src/tests/ecma_6/RegExp/descriptor.js
@@ +21,5 @@
> +    assertEq("get" in Object.getOwnPropertyDescriptor(r, name), true);
> +  }
> +}
> +
> +testProto(RegExp.prototype);

It doesn't really matter here, so just for future reference: no need to wrap the test into a function, you can instead just run it. Only if you'd want to run the same test against multiple objects would that be needed.

::: js/src/tests/ecma_6/RegExp/flag-accessors.js
@@ +15,5 @@
> +test(/foo/iymg, [true, true, true, true, false]);
> +test(RegExp(""), [false, false, false, false, false]);
> +test(RegExp("", "mygi"), [true, true, true, true, false]);
> +assertThrowsInstanceOf(() => RegExp("", "mygui").flags, SyntaxError);
> +// When the /u flag is supported, uncomment the line below and remove the line above

Please move this comment up one line (and reword it accordingly).

::: js/src/vm/RegExpObject.cpp
@@ +413,5 @@
> +    }
> +    return true;
> +}
> +
> +/* Note: returns the original if no escaping need be performed. */

There's a mixture of /* and //-style comments in this file. We nowadays usually prefer the latter if not all of the context uses the former. Hence, please change all the new or modified comments to //-style. (But ignore this, because these changes shouldn't be done anyway, as explained in the top comment.)

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Implement RegExp.prototype.{global, ignoreCase, multiline, source, sticky, unicode}.

Thank you for reviewing :)

This patch implements flags, and simplified source accessors without any change to its return value.

(In reply to Till Schneidereit [:till] from comment #2)
> That third one, however, isn't required or even correct: we already escape
> the result of .source in the right way. You can test this by doing something
> like the following:
> var re1 = new RegExp("\r\n\u2028\u2029");
> var re2 = new RegExp(re1.source);
> print(re1.source === re2.source);
> 
> This prints `true` with the current code, but would print `false` with your
> change. Also, new RegExp(/\r\n\u2028\u2029/).source gives the result you
> expect, with double-\\s.

With my last patch, it also prints true.

Then, in EscapeRegExpPattern's description
> 2. The code points / or any LineTerminator occurring in the pattern shall be
> escaped in S as necessary to ensure that the String value formed by
> concatenating the Strings "/", S, "/", and F can be parsed (in an appropriate
> lexical context) as a RegularExpressionLiteral that behaves identically to
> the constructed regular expression.
Sorry If I'm wrong, I think
> concatenating the Strings "/", S, "/", and F
means
  "/" + re1.source + "/" + re1.flags
and
> can be parsed as a RegularExpressionLiteral
means
  eval("/" + re1.source + "/" + re1.flags)
shouldn't throw SyntaxError, also, following should print true.
  print(eval("/" + re1.source + "/" + re1.flags).source === re1.source)

Currently re1.source returns raw LineTerminators, and it cannot be written in RegularExpressionLiteral, am I correct?

I found that bug 866367 is related to this.


> The changes to the flags look great, and so does the change to "(?:)". The
> most important reason why I'd like to see the latter split out into its own
> bug is that it is potentially a breaking change: there might be
> Firefox-specific code that somehow depends on us returning an empty string
> there. I hope there isn't, but it's uncertain, so we should land that change
> on its own.

Okay, I filed bug 1130798 for "(?:)".


> ::: js/src/tests/ecma_2/RegExp/function-001.js
> @@ +35,1 @@
> >  AddTestCase(
> 
> Please just remove this entire file. We don't need multiple identical copies
> of the same test, and this is identical to constructor-001.js

I'll remove it in bug 1130798, since it touches constructor-001.js.


Green on try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c19245ac5efc
  (Please ignore "Part 1" in the commit message)
Also, running try run for bug 1130798
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=976d9db08519

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8560944 [details] [diff] [review]
Implement RegExp.prototype.{global, ignoreCase, multiline, source, sticky, unicode}.

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

Looks good, thanks.

While looking into bug 1130860, I found a problem with cross-compartment objects, though:
https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/RegExp.cpp#249-255

What the comment says doesn't hold any longer, so it should be fairly easy to build a test that fails with this patch. Fixing it should involve doing a `getSource` on the RegExpGuard above. Also, the returned thing should always be an atom, so all of this can be simplified quite a bit.

I also just filed bug 1054755 about the other change we need to do to the same code.

I'd like to see the result of that quickly, so f+ instead of r+.

::: js/src/tests/js1_8_5/extensions/clone-regexp.js
@@ +28,5 @@
>  re.expando = true;
>  testRegExp(re);
> +// source and other accessors are defined in prototype, and cannot access to
> +// them if __proto__ is modified, so pass original regexp literal as 2nd
> +// argument here.

"`source` and the flag accessors are defined on RegExp.prototype, so they're not available after re.__proto__ has been changed. We solve that by passing in an additional copy of the same RegExp to compare the serialized-then-deserialized clone with."

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Implement RegExp.prototype.{global, ignoreCase, multiline, source, sticky, unicode}.

Thanks again :)

(In reply to Till Schneidereit [:till] from comment #4)
> While looking into bug 1130860, I found a problem with cross-compartment
> objects, though:
> https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/RegExp.cpp#249-
> 255
> 
> What the comment says doesn't hold any longer, so it should be fairly easy
> to build a test that fails with this patch. Fixing it should involve doing a
> `getSource` on the RegExpGuard above. Also, the returned thing should always
> be an atom, so all of this can be simplified quite a bit.

Okay, I added constructor-regexp.js test, it fails with unpatched build.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3de81e8f89c8

Comment 6

[Till Schneidereit [:till]]

Comment on attachment 8561091 [details] [diff] [review]
Implement RegExp.prototype.{global, ignoreCase, multiline, source, sticky, unicode}.

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

Excellent, thanks!

::: js/src/tests/ecma_6/RegExp/constructor-regexp.js
@@ +41,5 @@
> +  flagsCalled = true;
> +  return "i";
> +}});
> +b;
> +`);

Man, template strings are nice. (No action required, just marveling at the beauty of things.)

Comment 8

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ddd307bb5d1

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ddd307bb5d1

Comment 10

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/38#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/global
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/ignoreCase
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/multiline
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/source
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/sticky
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/unicode

Comment 11

[Jeff Walden [:Waldo]]

We shouldn't add RegExp.prototype.unicode if we don't actually support Unicode regular expressions.  People will quite reasonably want to use the presence of this property to indicate whether Unicode support is available, and adding this now stymies that.  Filed bug 1132295 on that.

Comment 12

[Tooru Fujisawa [:arai]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> We shouldn't add RegExp.prototype.unicode if we don't actually support
> Unicode regular expressions.  People will quite reasonably want to use the
> presence of this property to indicate whether Unicode support is available,
> and adding this now stymies that.  Filed bug 1132295 on that.

Thank you for letting me know that perspective.
I agree with removing it :)