google / google/closure-compiler

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

Open
#3,610 1 comment 0 reactions 1 assignee Claimed by @brad4d View on GitHub
Dominant language
JavaScript
Stars
7.7k
Forks
1.2k
Avg merge
2d 12h
Merged PRs (30d)
6

Description

https://github.com/google/closure-compiler/blob/3ae91f3902945df7df064b03afe4ebc24b67b132/src/com/google/javascript/jscomp/js/es6/string/matchall.js#L48

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 ]](https://tc39.es/ecma262/#sec-regexp-prototype-matchall).

> 7. Let lastIndex be ? ToLength(? Get(R, "lastIndex")).
> 8. Perform ? Set(matcher, "lastIndex", lastIndex, true).

The polyfill also doesn't appear to fully implement step 11.a.ii.2 of [%RegExpStringIteratorPrototype%.next](https://tc39.es/ecma262/#sec-%regexpstringiteratorprototype%.next) or [AdvanceStringIndex](https://tc39.es/ecma262/#sec-advancestringindex), which require incrementing [lastIndex](https://tc39.es/ecma262/#sec-lastindex) by 2 when the [Unicode](https://tc39.es/ecma262/#sec-get-regexp.prototype.unicode) flag is set and the next 2 code units form a surrogate pair.

Note, the comparison at https://github.com/google/closure-compiler/blob/3ae91f3902945df7df064b03afe4ebc24b67b132/src/com/google/javascript/jscomp/js/es6/string/matchall.js#L64 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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.