Return undefined from RegExp.prototype flags accessors on type mismatch #263

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
9 participants
@littledan
Member

littledan commented Dec 17, 2015

This addresses #262, a web compatibility concern from the addition
of 'sticky'. Websites apparently used RegExp.prototype.sticky to
feature-test for that RegExp feature. This patch makes all flag
accessors simply return false if the receiver does not have an
[[OriginalFlags]] internal slot.

Return false from RegExp.prototype flags accessors on type mismatch
This addresses #262, a web compatibility concern from the addition
of 'sticky'. Websites apparently used RegExp.prototype.sticky to
feature-test for that RegExp feature. This patch makes all flag
accessors simply return *false* if the receiver does not have an
[[OriginalFlags]] internal slot.
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Dec 17, 2015

Member

Bikeshed: I generally agree that undefined would be a better choice.

Member

domenic commented Dec 17, 2015

Bikeshed: I generally agree that undefined would be a better choice.

@zloirock

This comment has been minimized.

Show comment
Hide comment
@zloirock

zloirock Dec 17, 2015

@domenic I have many times referred to this example. With undefined we will not have an error here but support sticky regexs will not be detected.

@domenic I have many times referred to this example. With undefined we will not have an error here but support sticky regexs will not be detected.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Dec 17, 2015

Member

Yes, which I think is appropriate for code that is using feature-testing on not-a-regexp. It would be like checking { foo: bar }.sticky; you should not expect it to be a good feature test for sticky support on actual regexp objects.

Member

domenic commented Dec 17, 2015

Yes, which I think is appropriate for code that is using feature-testing on not-a-regexp. It would be like checking { foo: bar }.sticky; you should not expect it to be a good feature test for sticky support on actual regexp objects.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Dec 17, 2015

Member

With all the ES6 extension points for regexp, having a Symbol.match method is what makes it an "actual" RegExp object, and ".sticky" is in fact the way all the spec methods check for sticky support.

Member

ljharb commented Dec 17, 2015

With all the ES6 extension points for regexp, having a Symbol.match method is what makes it an "actual" RegExp object, and ".sticky" is in fact the way all the spec methods check for sticky support.

@claudepache

This comment has been minimized.

Show comment
Hide comment
@claudepache

claudepache Dec 17, 2015

Contributor

Note that there are still valid ways to test sticky support that don't need spec-wise hack around RegExp.prototype.sticky, e.g. "sticky" in /(?:)/.

Contributor

claudepache commented Dec 17, 2015

Note that there are still valid ways to test sticky support that don't need spec-wise hack around RegExp.prototype.sticky, e.g. "sticky" in /(?:)/.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Dec 17, 2015

Member

having a Symbol.match method is what makes it an "actual" RegExp object

That is not true; RegExp.prototype has a @@match property and is not a RegExp object.

and ".sticky" is in fact the way all the spec methods check for sticky support.

That is also not true. Apart from the generic .flags accessor, the only other place that uses .sticky is RegExpBuiltinExec, which is guarded by "If R does not have a [[RegExpMatcher]] internal slot, throw a TypeError exception."

Member

domenic commented Dec 17, 2015

having a Symbol.match method is what makes it an "actual" RegExp object

That is not true; RegExp.prototype has a @@match property and is not a RegExp object.

and ".sticky" is in fact the way all the spec methods check for sticky support.

That is also not true. Apart from the generic .flags accessor, the only other place that uses .sticky is RegExpBuiltinExec, which is guarded by "If R does not have a [[RegExpMatcher]] internal slot, throw a TypeError exception."

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson Dec 17, 2015

Member

Undefined seems better if we can get away with it - it's undefined in most browsers and is undefined in ES5. If we want to support more efficient feature detection, there are better ways to do it.

Member

bterlson commented Dec 17, 2015

Undefined seems better if we can get away with it - it's undefined in most browsers and is undefined in ES5. If we want to support more efficient feature detection, there are better ways to do it.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Dec 17, 2015

Member

That's an interesting point - @allenwb has claimed that in fact the presence of a Symbol.match property is what makes something a regex as part of RegExp subclassability, so if RegExp.prototype is an ordinary object, it shouldn't have that property.

In general, RegExp subclasses are supposed to use only the accessors to check flags, so although in the spec that's the only place .sticky is checked specifically, I'm referring to all the accessors in aggregate, as well as possible subclass implementations that would use them.

Member

ljharb commented Dec 17, 2015

That's an interesting point - @allenwb has claimed that in fact the presence of a Symbol.match property is what makes something a regex as part of RegExp subclassability, so if RegExp.prototype is an ordinary object, it shouldn't have that property.

In general, RegExp subclasses are supposed to use only the accessors to check flags, so although in the spec that's the only place .sticky is checked specifically, I'm referring to all the accessors in aggregate, as well as possible subclass implementations that would use them.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Dec 17, 2015

Member

As @zloirock suggests, I think it'd be great if we could let users be progressively enhanced to using sticky RegExps in the way that XRegExp tests for them. This is a pre-existing API, and we'd just be codifying it. It's undefined in existing browsers because they don't support it, and that's what XRegExp is testing for.

Basically, if we make the answer 'false' rather than 'undefined', we'll make the web faster at no real expense to any of us by meeting reasonable user expectations.

Member

littledan commented Dec 17, 2015

As @zloirock suggests, I think it'd be great if we could let users be progressively enhanced to using sticky RegExps in the way that XRegExp tests for them. This is a pre-existing API, and we'd just be codifying it. It's undefined in existing browsers because they don't support it, and that's what XRegExp is testing for.

Basically, if we make the answer 'false' rather than 'undefined', we'll make the web faster at no real expense to any of us by meeting reasonable user expectations.

@allenwb

This comment has been minimized.

Show comment
Hide comment
@allenwb

allenwb Dec 17, 2015

Member

Here is a different perspective to consider. Legacy code that does RegExp.prototype.sticky===false as a feature check must be specifically looking for the legacy FF sticky support as FF was only pre-ES6 browser that supported it. That code also must presumably have a fallback to use when not running on FF. If it doesn't it isn't interesting from a interop perspective. So, a false negative that lies about the sticky flag being unavailable would not be a web breaking change.

The ES6 sticky feature is not necessarily 100% identical to the legacy FF feature. One difference is the behavior of RegExp.prototype.sticky that we are already discussing. I suspect there may be other differences, as I was never provided a comprehensive spec. of the FF feature. From that perspective it is safer for a legacy check for FF's sticky support to report negative on any ES6 implementation.

Making ES6 RegExp.prototype.sticky return undefined accomplishes that. It avoids throwing on such feature tests, and it also avoids implying that that a pre-ES6 FF compatible implementation of sticky if available.

As @claudepache mentioned "sticky" in /(?:)/ or sticky in RegExp() would be the appropriate way to test for piece-mill ES6 level sticky support. Similarly for unicode support.

Member

allenwb commented Dec 17, 2015

Here is a different perspective to consider. Legacy code that does RegExp.prototype.sticky===false as a feature check must be specifically looking for the legacy FF sticky support as FF was only pre-ES6 browser that supported it. That code also must presumably have a fallback to use when not running on FF. If it doesn't it isn't interesting from a interop perspective. So, a false negative that lies about the sticky flag being unavailable would not be a web breaking change.

The ES6 sticky feature is not necessarily 100% identical to the legacy FF feature. One difference is the behavior of RegExp.prototype.sticky that we are already discussing. I suspect there may be other differences, as I was never provided a comprehensive spec. of the FF feature. From that perspective it is safer for a legacy check for FF's sticky support to report negative on any ES6 implementation.

Making ES6 RegExp.prototype.sticky return undefined accomplishes that. It avoids throwing on such feature tests, and it also avoids implying that that a pre-ES6 FF compatible implementation of sticky if available.

As @claudepache mentioned "sticky" in /(?:)/ or sticky in RegExp() would be the appropriate way to test for piece-mill ES6 level sticky support. Similarly for unicode support.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Dec 17, 2015

Member

@allenwb Do you know of any other differences with our 'sticky' feature? To me, the history of sticky seems like a great example of one browser pioneering it, and other browsers catching up through standardization and meeting standards. It'd be great if the committee could support feature testing as a way of endorsing progressive enhancement/degradation of websites. This is how new features get adopted, and we like people to use the new ES2015 features! If we do standardize things in ways that are incompatible, there are stronger ways that we can signal that to users, like by changing the name. This strikes me as a case which really is compatible, except for the feature test.

I agree that 'in' would be a better way to do feature testing. Maybe websites avoid it because of perceived performance differences. Or maybe they just consider it more ergonomic. Either way, we have to design for the web we have--there is a certain currently supported, in-use API, and we might as well continue to support it in this case.

Member

littledan commented Dec 17, 2015

@allenwb Do you know of any other differences with our 'sticky' feature? To me, the history of sticky seems like a great example of one browser pioneering it, and other browsers catching up through standardization and meeting standards. It'd be great if the committee could support feature testing as a way of endorsing progressive enhancement/degradation of websites. This is how new features get adopted, and we like people to use the new ES2015 features! If we do standardize things in ways that are incompatible, there are stronger ways that we can signal that to users, like by changing the name. This strikes me as a case which really is compatible, except for the feature test.

I agree that 'in' would be a better way to do feature testing. Maybe websites avoid it because of perceived performance differences. Or maybe they just consider it more ergonomic. Either way, we have to design for the web we have--there is a certain currently supported, in-use API, and we might as well continue to support it in this case.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Dec 17, 2015

Member

Since XRegExp's test is for RegExp.prototype.sticky !== undefined, I think if we're going to give it a non-undefined value simply to make XRegExp faster, the value should be the string "hack to make XRegExp's feature testing work". That seems less arbitrary than false.

Member

domenic commented Dec 17, 2015

Since XRegExp's test is for RegExp.prototype.sticky !== undefined, I think if we're going to give it a non-undefined value simply to make XRegExp faster, the value should be the string "hack to make XRegExp's feature testing work". That seems less arbitrary than false.

@mathiasbynens

This comment has been minimized.

Show comment
Hide comment
@mathiasbynens

mathiasbynens Dec 17, 2015

Member

I’m still hoping this can be solved without any spec changes through evangelization. Note that only XRegExp 2 (released in 2012) is affected; a fix was shipped in v3. Do we know which website(s) this compatibility issue occurred on? To fix it they should update to XRegExp v3.

Update: https://bugs.chromium.org/p/v8/issues/detail?id=4617#c16 says it’s happening on Atlassian Stash. I’ve now reached out to Atlassian asking them to update to XRegExp v3 for Bitbucket Server (which is the new name for Stash, apparently).

Update: FWIW, Atlassian responded:

The upgrade of XRegExp to v3 is occurring in the release of Bitbucket Server version 4.3, the details of this can be found here: BSERVDEV-11403

Member

mathiasbynens commented Dec 17, 2015

I’m still hoping this can be solved without any spec changes through evangelization. Note that only XRegExp 2 (released in 2012) is affected; a fix was shipped in v3. Do we know which website(s) this compatibility issue occurred on? To fix it they should update to XRegExp v3.

Update: https://bugs.chromium.org/p/v8/issues/detail?id=4617#c16 says it’s happening on Atlassian Stash. I’ve now reached out to Atlassian asking them to update to XRegExp v3 for Bitbucket Server (which is the new name for Stash, apparently).

Update: FWIW, Atlassian responded:

The upgrade of XRegExp to v3 is occurring in the release of Bitbucket Server version 4.3, the details of this can be found here: BSERVDEV-11403

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Dec 17, 2015

Member

OK, consensus here seems to be that we don't want it to return 'false', so I changed it to 'undefined' in the latest version of this pull request.

@mathiasbynens This occurred on Atlassian Stash .

Member

littledan commented Dec 17, 2015

OK, consensus here seems to be that we don't want it to return 'false', so I changed it to 'undefined' in the latest version of this pull request.

@mathiasbynens This occurred on Atlassian Stash .

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson Dec 17, 2015

Member

@littledan FWIW, the high order for me is that we have two related but actually orthogonal issues: the web compat issue, and adding a feature to sticky flag to enable feature detection so that code written against Mozilla's pre-standards implementation continues to do the expected thing.

For the first issue, we should figure out if we actually need to fix it. If we do, we should fix it in a way that preserves the existing design where possible, and the existing design doesn't include any feature detection capabilities. Returning undefined is the right choice for this reason :)

If you think it's important we specify the feature detection feature that will light up some existing code, then you could advance that proposal through the normal process. The arguments you make upthread are good arguments motivating such a proposal.

Member

bterlson commented Dec 17, 2015

@littledan FWIW, the high order for me is that we have two related but actually orthogonal issues: the web compat issue, and adding a feature to sticky flag to enable feature detection so that code written against Mozilla's pre-standards implementation continues to do the expected thing.

For the first issue, we should figure out if we actually need to fix it. If we do, we should fix it in a way that preserves the existing design where possible, and the existing design doesn't include any feature detection capabilities. Returning undefined is the right choice for this reason :)

If you think it's important we specify the feature detection feature that will light up some existing code, then you could advance that proposal through the normal process. The arguments you make upthread are good arguments motivating such a proposal.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Dec 17, 2015

Member

I'm not going to advance this proposal through the stage process. I think, in the future, when adding new features which are already shipped by some browsers, it'd be reasonable to look at how feature testing is done in practice and take this into account in deciding on details. But it's clear that the consensus is against returning false in this case, and I don't see the stage process reversing the consensus.

For web compat: It's clear that, when some of the other parts of the RegExp spec are implemented, sticky can't be shipped on the current web. I'll defer to others on this thread to determine whether we should wait for evangelization to fix this issue over time (and either hold off for now on shipping sticky, or do some sort of temporary workaround), or if we should change the spec.

Member

littledan commented Dec 17, 2015

I'm not going to advance this proposal through the stage process. I think, in the future, when adding new features which are already shipped by some browsers, it'd be reasonable to look at how feature testing is done in practice and take this into account in deciding on details. But it's clear that the consensus is against returning false in this case, and I don't see the stage process reversing the consensus.

For web compat: It's clear that, when some of the other parts of the RegExp spec are implemented, sticky can't be shipped on the current web. I'll defer to others on this thread to determine whether we should wait for evangelization to fix this issue over time (and either hold off for now on shipping sticky, or do some sort of temporary workaround), or if we should change the spec.

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson Dec 17, 2015

Member

@littledan Totally agree we should have considered this when designing the feature. My only point is that here is probably not the place to redesign aspects of an already-designed feature even though it missed important scenarios. Supporting a new scenario previously not considered doesn't seem like a bug-fix level thing to me I guess.

Member

bterlson commented Dec 17, 2015

@littledan Totally agree we should have considered this when designing the feature. My only point is that here is probably not the place to redesign aspects of an already-designed feature even though it missed important scenarios. Supporting a new scenario previously not considered doesn't seem like a bug-fix level thing to me I guess.

@littledan littledan changed the title from Return false from RegExp.prototype flags accessors on type mismatch to Return undefined from RegExp.prototype flags accessors on type mismatch Dec 17, 2015

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Dec 18, 2015

Member

OK, sounds like we'll put this on hold for a while while we try evangelizing to get the web in a state where it won't break, and change the spec only if that fails.

Member

littledan commented Dec 18, 2015

OK, sounds like we'll put this on hold for a while while we try evangelizing to get the web in a state where it won't break, and change the spec only if that fails.

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson Dec 18, 2015

Member

We are data starved right now. I will see if we can use some of the Chakra crawling/telemetry infrastructure to collect more, but my intuition is that this might be a case we could get fixed. That said, if no one else shares this intuition I'm happy to take this and be done with it!

Member

bterlson commented Dec 18, 2015

We are data starved right now. I will see if we can use some of the Chakra crawling/telemetry infrastructure to collect more, but my intuition is that this might be a case we could get fixed. That said, if no one else shares this intuition I'm happy to take this and be done with it!

@zloirock

This comment has been minimized.

Show comment
Hide comment
@zloirock

zloirock Dec 18, 2015

The problem not only with XRegExp. One more example - es5-ext (123 225 downloads in the last day):

if (RegExp.prototype.sticky !== false) {
  Object.defineProperty(RegExp.prototype, 'sticky', { configurable: true,
    enumerable: false, get: isSticky });
}

cc @medikoo

The problem not only with XRegExp. One more example - es5-ext (123 225 downloads in the last day):

if (RegExp.prototype.sticky !== false) {
  Object.defineProperty(RegExp.prototype, 'sticky', { configurable: true,
    enumerable: false, get: isSticky });
}

cc @medikoo

@medikoo

This comment has been minimized.

Show comment
Hide comment
@medikoo

medikoo Dec 18, 2015

I don't think one coming from es5-ext is really used.

Still it's a pity that with ES2015 suddenly some prototype objects stopped being a valid instances, it definitely broke known and agreed (and in my opinion very nice) convention.

e.g. we could always assume that Function.prototype can be used as noop function, Array.prototype as an empty array, same way it was with RegExp.prototype which was equivalent to /(?:)/.

Now when RegExp.prototype got broken, we can no longer be sure about Function.prototype or Array.prototype for future versions of ES.

medikoo commented Dec 18, 2015

I don't think one coming from es5-ext is really used.

Still it's a pity that with ES2015 suddenly some prototype objects stopped being a valid instances, it definitely broke known and agreed (and in my opinion very nice) convention.

e.g. we could always assume that Function.prototype can be used as noop function, Array.prototype as an empty array, same way it was with RegExp.prototype which was equivalent to /(?:)/.

Now when RegExp.prototype got broken, we can no longer be sure about Function.prototype or Array.prototype for future versions of ES.

@mathiasbynens mathiasbynens referenced this pull request in medikoo/es5-ext Dec 18, 2015

Merged

Fix native sticky regexp support detection #41

@mathiasbynens mathiasbynens referenced this pull request in medikoo/es5-ext Dec 18, 2015

Merged

Fix native Unicode regexp support detection #42

@zloirock zloirock referenced this pull request in kangax/compat-table Mar 3, 2016

Merged

Updated Chrome 50 and 51 #749

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan May 11, 2016

Member

This has been fixed in a slightly different way in 1a2ca97

Member

littledan commented May 11, 2016

This has been fixed in a slightly different way in 1a2ca97

@littledan littledan closed this May 11, 2016

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson

bterlson May 11, 2016

Member

Thanks @littledan :)

Member

bterlson commented May 11, 2016

Thanks @littledan :)

@fatcerberus fatcerberus referenced this pull request in svaarala/duktape Dec 12, 2016

Merged

Align RegExp.prototype and instance properties with ES6 #1178

18 of 18 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment