1317397 - Implement RegExp.lastIndex changes from ES2017

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

lastIndex is no longer read resp. written to when the RegExp object is neither global nor sticky.


Spec changes:
https://github.com/tc39/ecma262/pull/627/files

Comment 1

[André Bargull [:anba]]

The PR was accepted, but it seems to be erroneous and hence not implementable for us. Consider the following case:

```
var re = /(.)\1/gu;
re.lastIndex = {
  valueOf() {
    // Recompile without Unicode-flag.
    re.compile(re.source, "g");
    return 0;
  }
};
var r = re.exec("\u{D83D}\u{DCA3}\u{DCA3}");
print(r ? r[0] : null);
```

With the new semantics, it seems that the expected result value is `null`. And here's why:

In RegExpBuiltinExec, the [[OriginalFlags]] is retrieved and stored, followed by calling ToLength(Get(R, "lastIndex")). Side-effects in ToLength can recompile the RegExp object! When the [[RegExpMatcher]] tries to find a match at string index 0, a failure is returned, because the recompiled RegExp works on code units and the sequence <U+D83D, U+DCA3> doesn't match "(.)\1". AdvanceStringIndex is called to advance to the next string index, but AdvanceStringIndex uses fullUnicode=true, so it skips over the complete code point U+1F4A3 and returns 2. The matcher also won't be able to find a match at string index 2, and therefore RegExpBuiltinExec returns `null`.

I don't think we want to support these semantics, so back to TC39 to get this corrected.

Comment 2

[Tooru Fujisawa [:arai]]

partially reverted:
https://github.com/tc39/ecma262/pull/798/files

remaining changes:

https://tc39.github.io/ecma262/#sec-regexpbuiltinexec
> 21.2.5.2.2 Runtime Semantics: RegExpBuiltinExec ( R, S )
> ...
> 12. Repeat, while matchSucceeded is false
>   a. If lastIndex > length, then
>     i. If global is true or sticky is true, then
>       1. Perform ? Set(R, "lastIndex", 0, true).
> ...

https://tc39.github.io/ecma262/#sec-regexp.prototype-@@search
> 21.2.5.9 RegExp.prototype [ @@search ] ( string )
> ...
> 5. If SameValue(previousLastIndex, 0) is false, then
>   a. Perform ? Set(rx, "lastIndex", 0, true).
> ...
> 7. Let currentLastIndex be ? Get(rx, "lastIndex").
> 8. If SameValue(currentLastIndex, previousLastIndex) is false, then
>   a. Perform ? Set(rx, "lastIndex", previousLastIndex, true).

Comment 3

[André Bargull [:anba]]

Attachment: bug1317397.patch

The changes for the optimized path in RegExpSearch are bit more tricky, please let me know if you think it needs more explanations.

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 8843468 [details] [diff] [review]
bug1317397.patch

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

Thank you :)
great testcases!

the comment for RegExpSearch opt path should be sufficient.

Comment 5

[André Bargull [:anba]]

Attachment: bug1317397.patch

Updated the patch to:
- fix an eslint style warning
- use correct git hashes for ES2017 draft references
- and to apply cleanly on inbound.

Carrying r+ from arai.

Comment 6

[André Bargull [:anba]]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11a6a031711e02a3c2953e85c5fce2d54d8f92f8
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c88bf63995cd57e9f28219aff71299d916d60dbd

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21743640e4d1
Only set lastIndex for global or sticky RegExps in RegExpBuiltinExec per ES2017. r=arai

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21743640e4d1