Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upFeature testing no longer possible with RegExp.prototype.sticky #262
Comments
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
littledan
Dec 16, 2015
Member
I tried a couple other browsers. In Edge 25, properties like 'sticky' seem to still seem to be own properties of the RegExp instance, which violates the spec and bypasses this issue. In Firefox Nightly, 'sticky' seems to be a getter property of RegExp.prototype, but it returns false on RegExp.prototype. Maybe we should all adopt the Firefox behavior. The spec's strictness has not been shipped by any of these three browsers, and I suspect web incompatibility is the reason.
|
I tried a couple other browsers. In Edge 25, properties like 'sticky' seem to still seem to be own properties of the RegExp instance, which violates the spec and bypasses this issue. In Firefox Nightly, 'sticky' seems to be a getter property of RegExp.prototype, but it returns false on RegExp.prototype. Maybe we should all adopt the Firefox behavior. The spec's strictness has not been shipped by any of these three browsers, and I suspect web incompatibility is the reason. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
caitp
Dec 16, 2015
Contributor
FireFox's current behaviour is the case because RegExp.prototype is still a RegExp instance. Their behaviour will match V8's current behaviour as soon as https://bugzilla.mozilla.org/show_bug.cgi?id=1192038 is fixed, unless the decision ends up being to keep it the way it is now.
From a quick look at https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/RegExpPrototype.cpp, JSC simply does not yet implement RegExp.prototype.sticky, however the other getters are currently throwing for non RegExp instances. Like FireFox, RegExp.prototype is still a RegExp instance, which accounts for RegExp.prototype.multiline returning false rather than throwing.
/CC @evilpie / @bterlson who may be able to comment on spidermonkey and chakra's positions, respectively
|
FireFox's current behaviour is the case because RegExp.prototype is still a RegExp instance. Their behaviour will match V8's current behaviour as soon as https://bugzilla.mozilla.org/show_bug.cgi?id=1192038 is fixed, unless the decision ends up being to keep it the way it is now. From a quick look at https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/RegExpPrototype.cpp, JSC simply does not yet implement RegExp.prototype.sticky, however the other getters are currently throwing for non RegExp instances. Like FireFox, RegExp.prototype is still a RegExp instance, which accounts for /CC @evilpie / @bterlson who may be able to comment on spidermonkey and chakra's positions, respectively |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
littledan
Dec 16, 2015
Member
Sounds like Firefox is delaying shipping because they have further web compatibility concerns besides this one with ES2015. This doesn't seem like a big vote of confidence to me.
|
Sounds like Firefox is delaying shipping because they have further web compatibility concerns besides this one with ES2015. This doesn't seem like a big vote of confidence to me. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
caitp
Dec 16, 2015
Contributor
I'm not really seeing that, but I'm hoping one of them will comment on this and let us know
|
I'm not really seeing that, but I'm hoping one of them will comment on this and let us know |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
ljharb
Dec 16, 2015
Member
It would be wonderful to not be forced to try/catch to determine the presence of an internal slot - since RegExp#exec currently throws, in general having the accessors always return a boolean seems like a great change.
|
It would be wonderful to not be forced to |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
evilpie
Dec 16, 2015
Contributor
@littledan, no idea why you think that. https://bugzilla.mozilla.org/show_bug.cgi?id=1192038 There is just some Firefox internal code that makes it annoying to implement, we haven't made any decision based on web compat.
|
@littledan, no idea why you think that. https://bugzilla.mozilla.org/show_bug.cgi?id=1192038 There is just some Firefox internal code that makes it annoying to implement, we haven't made any decision based on web compat. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
allenwb
Dec 16, 2015
Member
In Firefox Nightly, 'sticky' seems to be a getter property of RegExp.prototype, but it returns false on RegExp.prototype. Maybe we should all adopt the Firefox behavior. The spec's strictness has not been shipped by any of these three browsers, and I suspect web incompatibility is the reason.
If it turns out that this is an actual web breakage issue, a better fix than returning false would be for the getter to return undefined if its this value is not a RegExp instance. This result is consistent with what legacy code could would have seen if it tried to access sticky in a non-Firefox browsers.
Since this is purely a web compatibility hack, the fix should probably go into Annex B.
If it turns out that this is an actual web breakage issue, a better fix than returning false would be for the getter to return undefined if its Since this is purely a web compatibility hack, the fix should probably go into Annex B. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
bterlson
Dec 17, 2015
Member
We just haven't gotten to it yet... I hope v8 can get some data on how prevalent this is. Maybe usage is low enough to "evangelize" our way out :-P
|
We just haven't gotten to it yet... I hope v8 can get some data on how prevalent this is. Maybe usage is low enough to "evangelize" our way out :-P |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
littledan
Dec 17, 2015
Member
@allenwb I think undefined would make it feature-test as missing, rather than present. I'd rather it feature-test as present so users can take advantage of it.
I'd really rather not separate this out into Annex B. Right now, Annex B consists of larger, "chunky" things. There's a lot in the main spec that's there for compatibility, for example all the extra logic in ArraySpeciesCreate. It would be a mess to move everything like this out into Annex B just because it has to do with compatibility.
I'm somewhat skeptical putting things in Annex B. There are some things that are really separate and seem to make sense, like String.prototype.blink. But I think most of what's there is really core to getting JavaScript running. Many non-web embedding environments, like Node, have both backwards compatibility concerns that require Annex B, or some even beyond that, would really like consistent execution with their non-browser environment compared to the browser. Separating them just makes reading the spec unclear. One person at Google tried to implement JavaScript-compatible regular expressions, and implemented the part that's in the main spec, but was really frustrated to later see the Annex B additions. It's confusing to piece those two parts together (there's no single grammar or semantics definition). I'd really rather not further this confusion by separating out the definition of the 'sticky' accessor into the hypothetical one with the world with no backwards compatibility concerns, and the real-world one in Annex B.
@evilpie Oh sorry, I just skimmed the thread and saw a bunch of discussion about things throwing and compatibility concerns; it was an honest misunderstanding.
@bterlson It's not clear to me exactly what we would do with usage data. Say we find that 0.5% of the web reads RegExp.prototype.sticky. Then what do we do? How do we do effective evangelism, and then how do we know that we're done? I'd rather stay conservative and maintain existing interfaces until there's a strong justification to changing them.
V8 has tried to ship this and run extremely quickly into the fact that it broke the web. The offending code actually looks pretty typical to me, given the way the rest of JavaScript and web APIs work and the way that feature testing is often done. It's the ES2015 spec which is uncharacteristically strict in its checking, not the user code being especially sloppy. It's just not common in JavaScript for reading an innocent property on a primordial to throw like that. The spec change to fix web compatibility would be simple: replace throwing the TypeError with returning false.
|
@allenwb I think undefined would make it feature-test as missing, rather than present. I'd rather it feature-test as present so users can take advantage of it. I'd really rather not separate this out into Annex B. Right now, Annex B consists of larger, "chunky" things. There's a lot in the main spec that's there for compatibility, for example all the extra logic in ArraySpeciesCreate. It would be a mess to move everything like this out into Annex B just because it has to do with compatibility. I'm somewhat skeptical putting things in Annex B. There are some things that are really separate and seem to make sense, like @evilpie Oh sorry, I just skimmed the thread and saw a bunch of discussion about things throwing and compatibility concerns; it was an honest misunderstanding. @bterlson It's not clear to me exactly what we would do with usage data. Say we find that 0.5% of the web reads RegExp.prototype.sticky. Then what do we do? How do we do effective evangelism, and then how do we know that we're done? I'd rather stay conservative and maintain existing interfaces until there's a strong justification to changing them. V8 has tried to ship this and run extremely quickly into the fact that it broke the web. The offending code actually looks pretty typical to me, given the way the rest of JavaScript and web APIs work and the way that feature testing is often done. It's the ES2015 spec which is uncharacteristically strict in its checking, not the user code being especially sloppy. It's just not common in JavaScript for reading an innocent property on a primordial to throw like that. The spec change to fix web compatibility would be simple: replace throwing the TypeError with returning false. |
added a commit
to littledan/ecma262
that referenced
this issue
Dec 17, 2015
littledan
referenced this issue
Dec 17, 2015
Closed
Return undefined from RegExp.prototype flags accessors on type mismatch #263
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
bterlson
Dec 17, 2015
Member
Generally speaking, I completely understand the desire to avoid breaking changes whenever possible, but there is also a long-term complexity cost to implementing little fixes for every minor breakage that ever occurs. It's nice to be data driven rather than anecdote driven if at all possible. 0.5% of the web is a huge number so obviously we couldn't do anything about that. On the other hand if all we can find are a handful of sites, and those sites get fixed, then I'd say we're done. Microsoft has done this in the past, pretty sure others have as well.
In this case the fix is simple enough that I don't mind much. If the committee is fine with #263 so am I, though I reserve the right to feel more strongly one way or the other if we get closer to shipping the current semantics, though :-P
|
Generally speaking, I completely understand the desire to avoid breaking changes whenever possible, but there is also a long-term complexity cost to implementing little fixes for every minor breakage that ever occurs. It's nice to be data driven rather than anecdote driven if at all possible. 0.5% of the web is a huge number so obviously we couldn't do anything about that. On the other hand if all we can find are a handful of sites, and those sites get fixed, then I'd say we're done. Microsoft has done this in the past, pretty sure others have as well. In this case the fix is simple enough that I don't mind much. If the committee is fine with #263 so am I, though I reserve the right to feel more strongly one way or the other if we get closer to shipping the current semantics, though :-P |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
littledan
Dec 17, 2015
Member
By the way, it's XRegExp which is the underlying library that does this feature test. I think that's probably copied around pretty widely.
Agreed that, in general, when it's just a few sites and/or it would be really ugly or unfortunate to do the change, then going around to the websites can be preferable. I don't think that's this case here.
|
By the way, it's XRegExp which is the underlying library that does this feature test. I think that's probably copied around pretty widely. Agreed that, in general, when it's just a few sites and/or it would be really ugly or unfortunate to do the change, then going around to the websites can be preferable. I don't think that's this case here. |
allenwb
closed this
Dec 17, 2015
bterlson
reopened this
Dec 17, 2015
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
oops, see #263 (comment) |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
mathiasbynens
Dec 17, 2015
Member
By the way, it's XRegExp which is the underlying library that does this feature test. I think that's probably copied around pretty widely.
Note that only XRegExp 2 (released in 2012) is affected; a fix was shipped in v3. Depending on how widespread v2 is, it might be possible to evangelize our way out of this…
Note that only XRegExp 2 (released in 2012) is affected; a fix was shipped in v3. Depending on how widespread v2 is, it might be possible to evangelize our way out of this… |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
jacobp100
Dec 18, 2015
What was the reason for changing the RegExp prototype to no longer be a RegExp instance?
jacobp100
commented
Dec 18, 2015
|
What was the reason for changing the RegExp prototype to no longer be a RegExp instance? |
mathiasbynens
referenced this issue
Dec 18, 2015
Merged
Fix native sticky regexp support detection #41
added a commit
to mathiasbynens/es5-ext
that referenced
this issue
Dec 18, 2015
mathiasbynens
referenced this issue
Dec 18, 2015
Merged
Fix native Unicode regexp support detection #42
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
slightlyoff
Dec 18, 2015
Member
It does seem punitive to change an innocent read to something that now throws. It's not at all "JavaScripty"
|
It does seem punitive to change an innocent read to something that now throws. It's not at all "JavaScripty" |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
allenwb
Dec 19, 2015
Member
@slightlyoff If only it was that simple. It the root of this is issues relating to the legacy RegExp complile method and that fact that it violated writable and configurable property attribute invariants opening possible security issues. Also RegExp subclassing issues...
|
@slightlyoff If only it was that simple. It the root of this is issues relating to the legacy RegExp |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
caitp
Dec 19, 2015
Contributor
It's also just not true. Pretty much any WebIDL attribute will throw when invoked on the prototype directly rather than an instance of whatever interface
|
It's also just not true. Pretty much any WebIDL attribute will throw when invoked on the prototype directly rather than an instance of whatever interface |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
littledan
Dec 21, 2015
Member
By the way, a related issue is https://bugs.chromium.org/p/v8/issues/detail?id=4637 (users apparently using RegExp.prototype.toString() as a browser test). I am working on a patch for V8 (https://codereview.chromium.org/1543723002) which will put in workarounds for both of these issues, while installing telemetry to see how often they come up. From there, we can both ship most of ES2015 semantics and find out more about how evangelism for websites to come up-to-date with ES2015 goes.
|
By the way, a related issue is https://bugs.chromium.org/p/v8/issues/detail?id=4637 (users apparently using RegExp.prototype.toString() as a browser test). I am working on a patch for V8 (https://codereview.chromium.org/1543723002) which will put in workarounds for both of these issues, while installing telemetry to see how often they come up. From there, we can both ship most of ES2015 semantics and find out more about how evangelism for websites to come up-to-date with ES2015 goes. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
jorendorff
Jan 4, 2016
We just hadn't gotten around to implementing the standard yet; don't interpret our choice as a "vote" either way.
However.
I don't especially enjoy driving the clown car directly into every web-compat brick wall, putting it in reverse, backing up a few feet, and leaving it parked in the middle of the road for a few months waiting for data. Being data-driven is important when the benefit is significant. Here, I think it's a waste of time and energy, because there's an alternative: we can just back off a little.
I'd be fine with special-casing the built-in prototype in all non-mutating methods and getters that existed in ES5 —under the TypeError branch, so there's no performance penalty to normal use.
jorendorff
commented
Jan 4, 2016
|
We just hadn't gotten around to implementing the standard yet; don't interpret our choice as a "vote" either way. However. I don't especially enjoy driving the clown car directly into every web-compat brick wall, putting it in reverse, backing up a few feet, and leaving it parked in the middle of the road for a few months waiting for data. Being data-driven is important when the benefit is significant. Here, I think it's a waste of time and energy, because there's an alternative: we can just back off a little. I'd be fine with special-casing the built-in prototype in all non-mutating methods and getters that existed in ES5 —under the TypeError branch, so there's no performance penalty to normal use. |
bterlson
added
the
normative change
label
Feb 18, 2016
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
littledan
Feb 23, 2016
Member
I tried keeping the workaround minimal, to just make RegExp.prototype.sticky return undefined, as XRegExp ran into, and leave other accessors like global throwing. However, this is incompatible with old versions of @ljharb 's es6shim. Even though Jordan has updated his library to avoid the bug, it seems that not all of the web has updated, and I extended the workaround to all properties https://bugs.chromium.org/p/chromium/issues/detail?id=581577 .
I think we should reconsider again extending this compatibility fix to the spec. It's not only that we have to deal with these legacy versions of XRegExp and es6shim while they roll out to the web: We have designed and spec'd an API that our most expert users get tripped up on.
|
I tried keeping the workaround minimal, to just make I think we should reconsider again extending this compatibility fix to the spec. It's not only that we have to deal with these legacy versions of XRegExp and es6shim while they roll out to the web: We have designed and spec'd an API that our most expert users get tripped up on. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
I would also like to reconsider this. |
AnnotationMark
referenced this issue
Mar 28, 2016
Closed
Throwing TypeErrors on access of RegExp.prototype flag properties breaks some websites #500
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
littledan
Mar 28, 2016
Member
With Chrome 49 shipping to stable, we finally have some data on this question (modulo bugs in our data collection system):
- 0.0534% of page loads encounter a get of
RegExp.prototype.sticky(likely mostly XRegExp, even though XRegExp 2.0 as well as XRegExp 3.0 have been updated to avoid calling this accessor) - 0.0155% of page loads call
RegExp.prototype.toString()(likely to detect Safari, which I had heard historically returned"//"instead of"/(?:)/"; @msaboff could give more background here).
Other counters are not yet shipped in any stable version of Chrome, so they don't have much useful data, but bug reports suggest that other flag accessors are also at risk, as historical versions of es6shim called RegExp.prototype.flags to feature-test for its presence.
|
With Chrome 49 shipping to stable, we finally have some data on this question (modulo bugs in our data collection system):
Other counters are not yet shipped in any stable version of Chrome, so they don't have much useful data, but bug reports suggest that other flag accessors are also at risk, as historical versions of es6shim called |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
msaboff
Mar 29, 2016
Contributor
0.0155% of page loads call RegExp.prototype.toString() (likely to detect Safari, which I had heard historically returned "//" instead of "/(?:)/"; @msaboff could give more background here).
Safari returns "/(?:)/" for RegExp.prototype.toString() and has for several years.
Safari returns "/(?:)/" for RegExp.prototype.toString() and has for several years. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
littledan
Mar 31, 2016
Member
At the March 2016 TC39 meeting, we achieved consensus on semantics that say:
- On a brand check failure, the getters should return 'undefined' if the receiver is RegExp.prototype, otherwise propagate an exception.
- RegExp.prototype.source should similarly return '(?:)' with that receiver.
I'll write up a PR for these new semantics.
|
At the March 2016 TC39 meeting, we achieved consensus on semantics that say:
I'll write up a PR for these new semantics. |
littledan commentedDec 16, 2015
Some websites check for whether the JavaScript implementation supports the 'sticky' flag by reading RegExp.prototype.sticky. Under ES2015, this throws a TypeError because RegExp.prototype is not a RegExp.
https://bugs.chromium.org/p/v8/issues/detail?id=4617#c9
What if we make these flag accessors a little more tolerant? If
thisdoes not have a [[RegExpMatcher]] internal slot, how about returningfalse? As it stands, this is blocking us from shipping sticky RegExps.