Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String#matchAll polyfill does not appear to correctly set or update the lastIndex property #3610

Open
Andrew-Cottrell opened this issue Jun 4, 2020 · 1 comment
Assignees

Comments

@Andrew-Cottrell
Copy link

Andrew-Cottrell commented Jun 4, 2020

The String#matchAll polyfill fails this test case

(function () {
    "use strict";
    var regexp = /\S+/g;
    var string = "one two three";
    regexp.lastIndex = string.indexOf(" ") + 1; // match all after the first space character
    var results = Array.from(string.matchAll(regexp));
    console.log(results.length); // Expected 2
    console.log(results[0]); // Expected ["two", index: 4, ...]
    console.log(results[1]); // Expected ["three", index: 8, ...]
}());

The polyfill does not appear to implement steps 7-8 of RegExp.prototype [ @@matchAll ].

  1. Let lastIndex be ? ToLength(? Get(R, "lastIndex")).
  2. Perform ? Set(matcher, "lastIndex", lastIndex, true).

The polyfill also doesn't appear to fully implement step 11.a.ii.2 of %RegExpStringIteratorPrototype%.next or AdvanceStringIndex, which require incrementing lastIndex by 2 when the Unicode flag is set and the next 2 code units form a surrogate pair.

Note, the comparison at

if (regexCopy.lastIndex === previousIndex) {
does not reliably detect empty-string matches in at least Internet Explorer. The following approach, using match.index, seems to work reliably

var fullUnicode = Boolean(regexCopy.unicode); // 8.         Let fullUnicode be O.[[Unicode]].
var matchStr = match[0];                      // 11.a.i.    Let matchStr be ? ToString(? Get(match, "0")).
if (!matchStr) {                              // 11.a.ii.   If matchStr is the empty String, then
    regexCopy.lastIndex = match.index + 1 + ( // 11.a.ii.1. Let thisIndex be ? ToLength(? Get(R, "lastIndex")).
        fullUnicode &&                        // 11.a.ii.2. Let nextIndex be ! AdvanceStringIndex(S, thisIndex, fullUnicode).
        isSurrogatePair(string, match.index)  // 11.a.ii.3. Perform ? Set(R, "lastIndex", nextIndex, true).
    );
}

Of course, without any additional polyfill, fullUnicode would be false in Internet Explorer.

@Andrew-Cottrell Andrew-Cottrell changed the title String#matchAll polyfill does not appear to Let lastIndex be ? ToLength(? Get(R, "lastIndex")). String#matchAll polyfill does not appear to correctly set or update the lastIndex property Jun 4, 2020
@concavelenz
Copy link
Contributor

Thank you for the detailed report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants