Disregard lastIndex for non-global non-sticky regexps

[claudepache]

With these changes , non-global non-sticky regexps with nonwritable
lastIndex property should just work:

• In RegExpBuiltinExec, remove any access to the lastIndex property for
  non-global, non-sticky regexp.
• In RegExp.prototype.@@search, avoid to to set the lastIndex property
  when not necessary.

[claudepache]

The intention of this PR is to fix #625. I have attempted to find and remove all plausible write accesses to the lastIndex property on non-global, non-sticky regexps.

[claudepache]

I have attempted to find and remove all plausible write accesses to the lastIndex property on non-global, non-sticky regexps.

There remains the unconditional initialisation of rx.lastIndex that is part of the RegExpInitialize abstract operation (step 12). However, I don’t think it is worth to "fix" it; for I think it could go wrong here only when applying the deprecated RegExp#compile().

[littledan]

Do you think this will be web-compatible? Does this correspond to Firefox or Safari semantics?

[claudepache]

Do you think this will be web-compatible? Does this correspond to Firefox or Safari semantics?

Good question. First, note that setting a value to the lastIndex property of a non-global, non-sticky regexp is already useless, as it is not used when performing a match (see Step 8 of RegExpBuiltinExec). The only significant difference is that that property is no longer updated to 0 after a failed match. Test cases:

(function() {
    var rx = /a/
    rx.lastIndex = 3
    rx.exec('b')
    return rx.lastIndex
})()

• 0 in Firefox, Chrome, Edge (in accordance to current and ES5 spec);
• 3 in Safari (in accordance to the proposed changes).

(function() {
    var rx = /a/
    rx.lastIndex = 3
    'b'.match(rx)
    return rx.lastIndex
})()

• 0 in Chrome, Edge, Firefox Nightly (in accordance to current and ES5 spec);
• 3 in Safari and stable Firefox (in accordance to the proposed changes).

[claudepache]

Also, just in case it was not clear (note the /g flag):

'b'.match(Object.freeze(/a/g))

Throws in current Firefox, Safari, Chrome, as well as any version of spec that supports Object.freeze.

[littledan]

Would we need all of the changes in this patch to get the intended effect, or just the changes in lines 28700-28701?

[claudepache]

Would we need all of the changes in this patch to get the intended effect, or just the changes in lines 28700-28701?

• The changes in lines 28690/28694 just avoids a useless read access to rx.lastIndex.
• The changes of RegExp.prototype.@@search in lines 28940ff are needed in order to support String#search on frozen non-global regexps. (More precisely, it supports the most likely case, where lastIndex is blocked to 0: if we want to support the other cases, it becomes slightly more complicated.)

[bterlson]

This change seems good to me - no need to do puts that are useless. However, still needs-consensus. Any volunteers to lead the discussion at TC39?

[leobalter]

Count me in. I don't have anything complex to bring so I'll have time to do this.

[littledan]

I'd be happier with this if we made this a more minimal change, avoiding the useless yet observable changes to reading lastIndex. It sounds like the changes to search are needed, however. I will unfortunately not have data beyond the bug report to contribute to the meeting.

[claudepache]

I'd be happier with this if we made this a more minimal change, avoiding the useless yet observable changes to reading lastIndex. It sounds like the changes to search are needed, however. I will unfortunately not have data beyond the bug report to contribute to the meeting.

About the useless read of lastIndex. This is part of the RegExpBuiltinExec abstract operation, which is applied only to objects that have a [[RegExpMatcher]] internal slots; and those objects always have an own nonconfigurable data property named lastIndex (see RegExpAlloc). So that any useless reading of that property on such object is truly unobservable.

About the changes in RegExp.prototype.@@search: It might be possible that the change is not needed for webcompat; however, I have tried to be consistent in making frozen regexp to just work under reasonable conditions, rather than to modify the strict minimum number of lines needed for resolving the particular reported issue. Consistency is important in order to avoid bugs.

[arai-a]

ToLength is observable there.

let r = /a/;
a.lastIndex = { valueOf() { throw "hello" } };
r.test("a"); // this throws "hello", while ToLength operation

[claudepache]

ToLength is observable there.

True. This is probably only significant for checking how scrupulously implementations follow the spec. Test case:

function test(rx) {
    rx.lastIndex = { valueOf: function () { throw "ok" } }
    try {
        rx.exec('foo')
    }
    catch (e) {
        if (e === "ok")
            return true
    }
    return false
}

Results:

	test(/a/g)	test(/a/)
ECMA262, Firefox, Chrome	true	true
Safari, IE, Edge	true	false

[leobalter]

This reached consensus on the July meeting.

[littledan]

Sounds like we need test262 tests before merging this PR. Who wants to write them?

Edit: Oh, I see a bug was filed at tc39/test262#737 .

[leobalter]

it's good to verify the work on tc39/test262#711 to avoid any collision.

I can help @claudepache to get the tests done.

[tcare]

No collision that I could see.

[bterlson]

@tcare / @leobalter we merged test262 tests for this right? Shall I take this PR?

[leobalter]

yes, please

[schuay]

@bterlson Any news on merging this?

[bterlson]

@schuay thanks for the ping, I think it's good to go. I removed the label and then promptly lost it :-P