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

Normative: correct spec bug in `RegExp.prototype[Symbol.matchAll]` #1517

Open
wants to merge 1 commit into
base: master
from

Conversation

@ljharb
Copy link
Member

commented Apr 24, 2019

Filed on v8 as https://bugs.chromium.org/p/v8/issues/detail?id=9174, which it turns out matched the spec but not the intention/convention of the RegExp flags value.

Normative: correct spec bug in `RegExp.prototype[Symbol.matchAll]`
Filed on v8 as https://bugs.chromium.org/p/v8/issues/detail?id=9174,
which it turns out matched the spec but not the intention/convention of
the RegExp flags value.

@ljharb ljharb requested review from mathiasbynens and gsathya Apr 24, 2019

@gsathya

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

@ljharb

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

This was a mistake on my part - https://tc39.github.io/ecma262/#sec-regexpinitialize has a special case for undefined being the empty string, and I overlooked that when ToStringing the flags value (in order to check if "g" is in it).

@@ -31129,7 +31129,8 @@ <h1>RegExp.prototype [ @@matchAll ] ( _string_ )</h1>
1. If Type(_R_) is not Object, throw a *TypeError* exception.
1. Let _S_ be ? ToString(_string_).
1. Let _C_ be ? SpeciesConstructor(_R_, %RegExp%).
1. Let _flags_ be ? ToString(? Get(_R_, `"flags"`)).
1. Let _flagsValue_ be ? Get(_R_, `"flags"`).

This comment has been minimized.

Copy link
@zenparsing

zenparsing Apr 24, 2019

Member

Do you think that @@split suffers from a similar issue?

var str = 'a\u{1d11e}b';
var pattern = /\u{1d11e}/u;
Object.defineProperty(pattern, 'flags', { value: undefined, configurable: true });
str.split(pattern);
// SyntaxError: Invalid flags supplied to RegExp constructor 'undefinedy'

undefinedy 🤣

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 24, 2019

Author Member

Yes, that also seems like the same spec bug to me.

I'd be happy to pursue changing this in the other methods too, if folks feel that's viable.

This comment has been minimized.

Copy link
@rbuckton

rbuckton Jul 23, 2019

Contributor

If we're going to do this in multiple places, should we have something like a ToStringOrEmpty operation to handle undefined (or null)?

BTW: null is an issue too, both for matchAll and split

zloirock added a commit to zloirock/core-js that referenced this pull request Apr 24, 2019
@littledan

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

I'm not convinced we should be adding defensive cases like this in subclassing built-ins. There remain lots of other silly things you can do to interfere with how things run.

@ljharb

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

@littledan this isn't about subclassing; my example in the v8 bug is about a mutated base regex literal, and it follows the convention in new RegExp.

@littledan

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

I know; I was using "subclassing" in the broad sense where this is all caused by "subclassable built-ins", the justification in ES6 to read flags here, rather than the internal slot. I don't see how this affects my point, though; we discussed this point several times in the development of matchAll.

@ljharb

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2019

That was about people deleting things off of RegExp.prototype. This is about following existing conventions when reading flags off of an object, which are that undefined becomes the empty string, not the string “undefined”. If i were merely handling edge cases I’d want to do the same for null, but that’s not the convention so I’m not proposing that.

@littledan

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

We discussed a case of deleting things off of RegExp.prototype in depth, but the underlying motivation (for me) was that we should not start being defensive against these things, in general--this was the design of ES6. The defensive design would be to just use internal slots for these things. I'd be in favor of returning to ES5-style RegExps, if it's web-compatible, but with the situation we have, I am not in favor of putting in lots of kinds of validation to avoid weird cases. There will be lots more weird cases, e.g., if flags returns something different from the individual getters, or if they return different values on subsequent calls.

ljharb added a commit to es-shims/String.prototype.matchAll that referenced this pull request Apr 25, 2019
ljharb added a commit to es-shims/String.prototype.matchAll that referenced this pull request Apr 25, 2019
zloirock added a commit to zloirock/core-js that referenced this pull request May 10, 2019
zloirock added a commit to zloirock/core-js that referenced this pull request May 10, 2019
@mathiasbynens

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

(Posting here since Jordan asked)

The V8 team is not in favor of this patch. It enables even more esoteric RegExp subclassing behavior than is already possible today, and if accepted, would set precedent for similar changes.

Subclassing built-ins is an important use case, but in particular for RegExp the subclassing design is already pretty weird. We'd rather not add more complexity to support even stranger use cases, especially without them being driven by real-world use cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.