Refactor the spec #17

ljharb · 2017-08-01T09:09:23Z

Highlights:

Symbol.matchAll
String.prototype.matchAll stringifies and coerces to a regex when not IsRegExp (the canonical test for "it's a regex")
- If the regex has a Symbol.matchAll method, it calls it
- if not, it calls into the MatchAllIterator abstract operation
RegExp.prototype[Symbol.matchAll] throws if this isn't a regex, and otherwise calls into MatchAllIterator
MatchAllIterator uses SpeciesConstructor to construct a copy of the regex using its .flags and .lastIndex and then calls into CreateRegExpStringIterator
- this means that the existing lastIndex is respected, which is particularly important for "sticky" regexes
CreateRegExpStringIterator creates a new iterator, with some internal slots to hold on to the regex, the string, the last matched index, and whether the iteration is finished or not
the effect is that a non-sticky/non-global regex will always yield one match before being "done", and a sticky and/or global one would yield all the matches you'd get from repeatedly .execing it, before being "done".

I have similar changes prepared on my polyfill/shim (which I've moved to a separate repo) and will push those up and release them on npm as soon as this is merged.

Closes #6. Closes #4. Closes #3. Closes #2. Closes #15.

…erly support subclassing.

…on to iterate over.

- In other words, ensure that two match objects in a row with the same “index” will stop the iteration.

…coerce it to a regex).

ljharb · 2017-08-01T09:16:56Z

@allenwb @littledan @anba @bterlson any thoughts?

It's not stage 2 yet, so detailed spec review isn't needed (tho I always welcome it!), but any overarching direction comments would be helpful :-)

bterlson

Not an official review (or technically in depth) but the direction seems fine.

bterlson · 2017-08-09T23:43:10Z

spec.emu

-	<emu-note>_matchAll_ does not visibly mutate the provided _regexp_ in any way.</emu-note>
-	<p>When the _matchAll_ method is called, the following steps are taken:</p>
+
+	<p>When the `matchAll` method is called, the following steps are taken:</p>


usually enumerates the arguments and their expected values here

I initially had that, but the arguments are in the heading above, so it felt redundant to repeat them here. As for "expected values", do you mean like, prose that describes them?

I guess value is often encoded in the var name as it is in this case, so it's fine. Agree about just repeating the inputs; if you can come up with more descriptive prose ala exec it would be good, but it's not a major deal of course.

I can definitely do that, update incoming.

bterlson · 2017-08-09T23:43:51Z

spec.emu

+		1. Let _matcher_ be ? GetMethod(_R_, @@matchAll).
+		1. If _matcher_ is not *undefined*, then
+			1. Return ? Call(_matcher_, _R_, &laquo; _O_ &raquo;).
+		1. Return ? <emu-xref href="#sec-matchalliterator">MatchAllIterator</emu-xref>(_R_, _O_).


This shouldn't require an explicit xref.

bterlson · 2017-08-09T23:46:39Z

spec.emu

 				1. Return ! CreateIterResultObject(*null*, *true*).
 			1. Else,
-				1. Return ! CreateIterResultObject(_match_, *false*).
+				1. Let _previousIndex_ be _O_.[[PreviousIndex]].
+				1. Assert: Type(_previousIndex_) is Number.


Least surprising assert ever :-P

littledan · 2017-08-10T13:10:07Z

spec.emu

+	<emu-alg>
+		1. Let _R_ be the *this* value.
+		1. If ? IsRegExp(_R_) is not *true*, throw a *TypeError* exception.
+		1. Return ? <emu-xref href="#sec-matchalliterator">MatchAllIterator</emu-xref>(_R_, _string_).


Nit: Why include this link explicitly?

Fixed per #17 (comment)

littledan · 2017-08-10T13:10:33Z

spec.emu

+</ins>
+
+<ins class="block">
+<emu-clause id="sec-matchalliterator" aoid-"MatchAllIterator">


Probably you want a = after aoid, rather than - (so then you might not need the explicit link).

whoops, thanks, good catch

littledan · 2017-08-10T13:16:04Z

spec.emu

+		1. Let _S_ be ? ToString(_O_).
+		1. Let _C_ be ? SpeciesConstructor(_R_, %RegExp%).
+		1. Let _flags_ be ? ToString(? Get(_R_, *"flags"*)).
+		1. Let _matcher_ be ? Construct(_C_, &laquo; _R_, _flags_ &raquo;).


FWIW seems like the single-argument form of the RegExp constructor would work too (assuming other RegExp subclasses will do something similar, and that IsRegExp will be true for the relevant subclasses).

The single-argument form, when given a RegExp, returns it unmodified - the only way I'm aware of to unconditionally "clone" a regex with the constructor is by also passing the flags argument.

littledan · 2017-08-10T13:19:46Z

spec.emu

-		1. If _global_ is not *true*, throw a *TypeError* exception.
-		1. Let _iterator_ be ObjectCreate(<emu-xref href="#%RegExpStringIteratorPrototype%">%RegExpStringIteratorPrototype%</emu-xref>, &laquo; [[IteratingRegExp]], [[IteratedString]] &raquo;).
+		1. Assert: Type(_S_) is String.
+		1. Assert: IsRegExp(_R_) is *true*.


IsRegExp is really dynamic, and you're running this on whatever came out of the species constructor. I don't think this assertion is valid.

Hm, I guess that's true - it's a status that could change over time.

In that case, I think I should make it a runtime exception if it stops being a RegExp. Will fix.

Repeatedly reading @@match and throwing exceptions if it dissapears seems like rather surprising behavior. We don't do that in split, for example. I think you can just delete this assertion.

What we do in existing regex methods is done for legacy compat, not because it's necessarily the proper thing to do.

I suppose RegExpExec would fall back to the builtin behavior if the Symbol later disappeared, so it wouldn't be the worst thing in the world to delete this check, but what's the benefit of removing it?

The whole IsRegExp thing was just added in ES2015. I proposed changing it after it was spec'd, before it was implemented, but the committee rejected that. It seems a little early to go around calling it all legacy already.

What's the benefit of removing it? Because it doesn't make sense to put in tons of defensive checks all over the place that things are a certain way. We don't do that in any of the JS standard library. Subclassable builtins just does the appropriate operations at the right time, and if they're not there, calling them throws. These checks will not ensure that anything is a particular way; the exec method can still disappear at any time.

That's a fair point :-) what I mean is, the behavior for unmodified RegExp instances is legacy, and there's a lot of spec machinery to make that work. A non-default Symbol.match, however, is indeed not legacy.

Fair enough; I'm just trying to make things as strict and throwy as possible - but I'll remove the IsRegExp checks (after the iterator is created, at least) and let RegExpExec handle them.

littledan · 2017-08-10T13:25:22Z

spec.emu

-				1. Return ! CreateIterResultObject(_match_, *false*).
+				1. Let _previousIndex_ be _O_.[[PreviousIndex]].
+				1. Assert: Type(_previousIndex_) is Number.
+				1. Let _index_ be ? ToLength(? Get(_match_, *index*)).


Is index here supposed to be "lastIndex"?

No, match objects have an "index" and "input" property - regexes have "lastIndex".

littledan · 2017-08-10T13:27:02Z

spec.emu

+				1. Let _previousIndex_ be _O_.[[PreviousIndex]].
+				1. Assert: Type(_previousIndex_) is Number.
+				1. Let _index_ be ? ToLength(? Get(_match_, *index*)).
+				1. If _previousIndex_ is equal to _index_, then


I'm surprised by this special case. It's saying, if there's a zero-length match, then exit the iteration early? Why?

It's saying "if i've seen the match before, exit" - per https://github.com/tc39/proposal-string-matchall/pull/17/files#r132509509

littledan · 2017-08-10T13:27:58Z

spec.emu

 		1. Set _iterator_.[[IteratingRegExp]] to _R_.
 		1. Set _iterator_.[[IteratedString]] to _S_.
+		1. Set _iterator_.[[PreviousIndex]] to *-1*.


What's the purpose of maintaining the [[PreviousIndex]]?

If I don't, then a non-sticky, non-global regex will give an infinite iterator with the same match repeated over and over again - the index is the method I chose to ensure that a regex with one match only yields one match object.

littledan · 2017-08-10T13:31:37Z

spec.emu

 		1. Set _iterator_.[[IteratingRegExp]] to _R_.
 		1. Set _iterator_.[[IteratedString]] to _S_.
+		1. Set _iterator_.[[PreviousIndex]] to *-1*.
+		1. Set _iterator_.[[Done]] to *false*.


Is the purpose here to ensure that, even with a poorly behaved RegExp subclass, the iterator doesn't come back to life?

Yes, exactly - without caching the "done" status, it will call RegExpExec every time.

(this is also problematic with non-sticky non-global regexes, which either always or never return "no match")

littledan · 2017-08-10T13:31:59Z

spec.emu

@@ -52,13 +92,24 @@ contributors: Jordan Harband
 			1. Let _O_ be the *this* value.
 			1. If Type(_O_) is not Object, throw a *TypeError* exception.
 			1. If _O_ does not have all of the internal slots of a RegExp String Iterator Object Instance (see <emu-xref href="#PropertiesOfRegExpStringIteratorInstances"></emu-xref>), throw a *TypeError* exception.


What problem are you trying to solve with your changes to next() here?

https://github.com/tc39/proposal-string-matchall/pull/17/files#r132509719 and https://github.com/tc39/proposal-string-matchall/pull/17/files#r132509509

Per #17 (comment)

ljharb · 2017-09-22T21:22:49Z

I'm going to merge this soon if there's no further objections; at which point further review can be done via issues :-)

ljharb added 6 commits September 26, 2015 13:58

Add Symbol.matchAll

9998466

Add RegExp#[Symbol.matchAll], remove “global” restriction, and prop…

4a94a62

…erly support subclassing.

Ensure no observable mutations by constructing a new regular expressi…

a49b433

…on to iterate over.

Prevent infinite re-exec-ing of the regex with a [[Done]] slot.

bcbd44d

Ensure non-sticky non-global regexps only return one match.

b7fe336

- In other words, ensure that two match objects in a row with the same “index” will stop the iteration.

Allow String.prototype.matchAll to take a string like match (and …

465fd06

…coerce it to a regex).

ljharb requested review from bterlson, allenwb and littledan August 8, 2017 06:12

bterlson approved these changes Aug 9, 2017

View reviewed changes

ljharb added 2 commits August 9, 2017 17:06

Remove unnecessary xref.

adc224e

Add prose to String.prototype.matchAll.

d48dfaf

littledan reviewed Aug 10, 2017

View reviewed changes

ljharb added 6 commits August 10, 2017 10:01

Fix HTML attribute formatting error.

5b34960

Per #17 (comment)

IsRegExp is dynamic, so it can’t be an assertion.

2f5c432

Per #17 (comment)

Fix markdown footnote-links.

26add78

Remove unnecessary xref.

c07cced

Per #17 (comment)

Remove IsRegExp calls after the iterator is created.

f50e292

Per #17 (comment)

Fix table of RegExpStringIterator internal slots.

77c7d2e

ljharb merged commit 1444fb8 into master Sep 23, 2017

ljharb deleted the redux branch September 23, 2017 16:18

zloirock mentioned this pull request Oct 26, 2017

core-js@3 zloirock/core-js#325

Merged

mathiasbynens mentioned this pull request Jul 24, 2019

How should replaceAll behave if searchValue is a non-global RegExp? tc39/proposal-string-replaceall#16

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the spec #17

Refactor the spec #17

ljharb commented Aug 1, 2017

ljharb commented Aug 1, 2017 •

edited

Loading

bterlson left a comment

bterlson Aug 9, 2017

ljharb Aug 9, 2017

bterlson Aug 9, 2017

ljharb Aug 10, 2017

bterlson Aug 9, 2017

bterlson Aug 9, 2017

littledan Aug 10, 2017

ljharb Aug 10, 2017

littledan Aug 10, 2017

ljharb Aug 10, 2017

littledan Aug 10, 2017

ljharb Aug 10, 2017

littledan Aug 10, 2017

ljharb Aug 10, 2017

littledan Aug 10, 2017

ljharb Aug 10, 2017

littledan Aug 11, 2017

ljharb Aug 11, 2017 •

edited

Loading

littledan Aug 10, 2017

ljharb Aug 10, 2017

littledan Aug 10, 2017

ljharb Aug 10, 2017

littledan Aug 10, 2017

ljharb Aug 10, 2017

littledan Aug 10, 2017

ljharb Aug 10, 2017

ljharb Aug 10, 2017

littledan Aug 10, 2017

ljharb Aug 10, 2017

ljharb commented Sep 22, 2017

Refactor the spec #17

Refactor the spec #17

Conversation

ljharb commented Aug 1, 2017

ljharb commented Aug 1, 2017 • edited Loading

bterlson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Sep 22, 2017

ljharb commented Aug 1, 2017 •

edited

Loading

ljharb Aug 11, 2017 •

edited

Loading