1147817 - Update RegExp constructor match to ES6 spec.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

Separated from bug 1054755.

RegExp constructor in ES6 uses IsRegExp function, which will be introduced in bug 1054755.
Also, there are some differences related to patternIsRegExp.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add RegExpInitialize.

Prepared 3 patches.

Part 1 factors out RegExpInitialize from CompileRegExpObject.
Also, regexp_construct_no_statics calls RegExpInitialize directly since it doesn't need remaining logic in CompileRegExpObject.


Then, I'm not sure whether Part 2 is the correct way.
In step 4.b.iii, spec requires comparing newTarget with pattern.constructor.
> If SameValue(newTarget, patternConstructor) is true, return pattern.

And following test fails if I use js::SameValue for patternConstructor and callee, because `r` is from different global.
https://dxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/basic/testCrossCompartmentTransparency.js#79
> test("new RegExp('1')", function(r) assertEq(r, RegExp(r)));

So, I added Proxy::fun_isNativeFunction and BaseProxyHandler::fun_isNativeFunction to compare the patternConstructor directly with js::regexp_construct.
If there is better way to compare values from different globals, please tell me :)


Part 3 adds step 4 and 6 to RegExp constructor.
In step 4.b.iii, I used IsNativeFunction instead of SameValue if patternConstructor is not a proxy, to make it consistent with proxy's case.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 2: add fun_isNativeFunction to Proxy and handlers.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Use IsRegExp in RegExp constructor.

Comment 4

[Tooru Fujisawa [:arai]]

Hm, if "global" corresponds to "Realm" in ES6 spec, RegExp functions in two Realms are different function object, thus `SameValue(newTarget, patternConstructor)` in Step 4.b.iii returns false if `patternConstructor` comes from different global than active function object, right?
so, should following assertion be fail now?
  var r = newGlobal().eval("/a/"); assertEq(RegExp(r), r)

If it's true, now we can use js::SameValue directly.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: (plan B) Part 2: Use IsRegExp in RegExp constructor.

in case comment #4 is correct, replacing previous Part 2 and Part 3.

Comment 6

[Tooru Fujisawa [:arai]]

try run for attachment 8588643 [details] [diff] [review]
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=75406c823d59

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Use IsRegExp in RegExp constructor.

Changed Step 4.b.iii to use operator==

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 8588096 [details] [diff] [review]
Part 1: Add RegExpInitialize.

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

Nice, r=me.

::: js/src/builtin/RegExp.cpp
@@ +167,5 @@
> +        if (!ParseRegExpFlags(cx, flagStr, &flags))
> +            return false;
> +    }
> +
> +    /* Step 9. */

Nit: Steps 9-10.

Comment 9

[Till Schneidereit [:till]]

Comment on attachment 8590406 [details] [diff] [review]
Part 2: Use IsRegExp in RegExp constructor.

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

Beautiful. r=me with feedback addressed.

::: js/src/builtin/RegExp.cpp
@@ +191,5 @@
>      return true;
>  }
>  
>  /*
> + * ES6 draft rc4 21.2.5.1 steps 5-10.

21.2.3.1 here and everywhere else.

::: js/src/tests/ecma_6/RegExp/constructor-IsRegExp.js
@@ +1,4 @@
> +var BUGNUMBER = 0;
> +var summary = "RegExp constructor with pattern with @@match.";
> +
> +print(BUGNUMBER + ": " + summary);

Either just remove this, or remove the BUGNUMBER part, or add 1147817. Whatever is fine, but printing "0: [description" is confusing.

::: js/src/tests/ecma_6/RegExp/constructor-constructor.js
@@ +1,4 @@
> +var BUGNUMBER = 0;
> +var summary = "RegExp constructor should check pattern.constructor.";
> +
> +print(BUGNUMBER + ": " + summary);

Same here.

Comment 10

[Tooru Fujisawa [:arai]]

Thank you for reviewing!
Fixed step and bug numbers.

https://hg.mozilla.org/integration/mozilla-inbound/rev/224db47e1e20
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad1cd598bb35

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/224db47e1e20
https://hg.mozilla.org/mozilla-central/rev/ad1cd598bb35

Comment 12

[Tooru Fujisawa [:arai]]

Updated following document:
  https://developer.mozilla.org/en-US/Firefox/Releases/40