Update RegExp constructor match to ES6 spec.

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: arai, Assigned: arai)

Tracking

(Blocks: 1 bug)

Trunk
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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.
Assignee: nobody → arai.unmht
Attachment #8588096 - Flags: review?(till)
(Assignee)

Comment 2

4 years ago
Attachment #8588097 - Flags: feedback?(till)
(Assignee)

Comment 3

4 years ago
Attachment #8588098 - Flags: feedback?(till)
(Assignee)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
in case comment #4 is correct, replacing previous Part 2 and Part 3.
Attachment #8588643 - Flags: review?(till)
(Assignee)

Comment 7

4 years ago
Changed Step 4.b.iii to use operator==
Attachment #8588643 - Attachment is obsolete: true
Attachment #8588097 - Attachment is obsolete: true
Attachment #8588098 - Attachment is obsolete: true
Attachment #8588643 - Flags: review?(till)
Attachment #8590406 - Flags: review?(till)
Attachment #8588097 - Flags: feedback?(till)
Attachment #8588098 - Flags: feedback?(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.
Attachment #8588096 - Flags: review?(till) → review+
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.
Attachment #8590406 - Flags: review?(till) → review+
https://hg.mozilla.org/mozilla-central/rev/224db47e1e20
https://hg.mozilla.org/mozilla-central/rev/ad1cd598bb35
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.