Improve consistency with existing RegExp methods and subclass handling #37

mathiasbynens · 2018-08-08T15:55:13Z

@anba outlined two separate concerns about the remaining IsRegExp call in MatchAllIterator. Quoting from #34 (comment):

When MatchAllIterator is called from RegExp.prototype [ @@matchAll ], we should/have to assume the user explicitly decided to treat R as a RegExp object, so having an additional IsRegExp call to change this decision seems questionable. It’s also not consistent with how the other RegExp.prototype methods work.
When MatchAllIterator is called from String.prototype.matchAll, I’d prefer to handle it more like the other String.prototype methods which create RegExp objects (that means String.prototype.match and String.prototype.search), because I want to avoid adding yet another way to handle RegExp sub-classes. There are already two different RegExp sub-classing/extension interfaces: the RegExp.prototype methods all call RegExpExec, which means sub-classes, or any other classes, only need to provide their own exec methods when they want to reuse the other RegExp.prototype methods. And in addition to that, the @@match/replace/search/split interfaces allow sub-classes to provide their own implementations for just these methods. The matchAll proposal in its current form adds another dimension to this by providing different code paths depending on whether or not an object is RegExp-like (as per the IsRegExp abstract operation). In my opinion we should only support RegExp sub-classing in two ways: 1) Either the RegExp sub-class has %RegExpPrototype% on its prototype chain, or 2) the RegExp sub-class copies the relevant methods from %RegExpPrototype% into its prototype object.

This PR addresses those issues following @anba’s proposal.

Ref. #21, #34.

@anba

@anba outlined two separate concerns about the remaining `IsRegExp` call in `MatchAllIterator`. Quoting from tc39#34 (comment): 1. When `MatchAllIterator` is called from `RegExp.prototype [ @@matchall ]`, we should/have to assume the user explicitly decided to treat `R` as a RegExp object, so having an additional `IsRegExp` call to change this decision seems questionable. It’s also not consistent with how the other `RegExp.prototype` methods work. 2. When `MatchAllIterator` is called from `String.prototype.matchAll`, I’d prefer to handle it more like the other `String.prototype` methods which create RegExp objects (that means `String.prototype.match` and `String.prototype.search`), because I want to avoid adding yet another way to handle RegExp sub-classes. There are already two different RegExp sub-classing/extension interfaces: the `RegExp.prototype` methods all call `RegExpExec`, which means sub-classes, or any other classes, only need to provide their own `exec` methods when they want to reuse the other `RegExp.prototype` methods. And in addition to that, the `@@match/replace/search/split` interfaces allow sub-classes to provide their own implementations for just these methods. The `matchAll` proposal in its current form adds another dimension to this by providing different code paths depending on whether or not an object is RegExp-like (as per the `IsRegExp` abstract operation). In my opinion we should only support RegExp sub-classing in two ways: 1) Either the RegExp sub-class has `%RegExpPrototype%` on its prototype chain, or 2) the RegExp sub-class copies the relevant methods from `%RegExpPrototype%` into its prototype object. Ref. tc39#21, tc39#34.

spec.emu

@@ -20,7 +20,9 @@ contributors: Jordan Harband
 			1. Let _matcher_ be ? GetMethod(_regexp_, @@matchAll).
 			1. If _matcher_ is not *undefined*, then
 				1. Return ? Call(_matcher_, _regexp_, &laquo; _O_ &raquo;).
-		1. Return ? MatchAllIterator(_regexp_, _O_).
+		1. Let _string_ be ? ToString(_O_).
+		1. Let _rx_ be ? RegExpCreate(_regexp_, `"g"`).


ljharb · 2018-08-08T19:12:25Z

spec.emu

@@ -20,7 +20,9 @@ contributors: Jordan Harband
 			1. Let _matcher_ be ? GetMethod(_regexp_, @@matchAll).
 			1. If _matcher_ is not *undefined*, then
 				1. Return ? Call(_matcher_, _regexp_, &laquo; _O_ &raquo;).
-		1. Return ? MatchAllIterator(_regexp_, _O_).


Separately, I very intentionally made this function continue to work after delete RegExp.prototype[Symbol.matchAll], which this change breaks.

It's more consistent with the existing methods though, as discussed here: #34 (comment)

@anba, @littledan, @tschneidereit all spoke up in favor of consistency over deviating from the status quo in this edge case. I'm inclined to agree.

The majority of the existing methods fall back when the Symbol method is deleted; those two examples are exceptions, and I think matchAll should follow the more common approach of falling back.

FWIW I agree that String.p.matchAll should be specced consistently with String.p.match and String.p.search. These are the only other 'regexp' methods defined on String. Is there a reason to diverge from their behavior?

Likewise in RegExp.p[@@matchAll].

There's also split and replace - see #38 (comment).

The consistency argument is exactly why the proposal has its current fallback behavior.

But the fallback in split and replace is to perform a non-RegExp string matching (i.e. "." only matches the code point U+002E (FULL STOP)), which is different from what matchAll does.

spec.emu

@@ -20,7 +20,9 @@ contributors: Jordan Harband
 			1. Let _matcher_ be ? GetMethod(_regexp_, @@matchAll).
 			1. If _matcher_ is not *undefined*, then
 				1. Return ? Call(_matcher_, _regexp_, &laquo; _O_ &raquo;).
-		1. Return ? MatchAllIterator(_regexp_, _O_).
+		1. Let _string_ be ? ToString(_O_).
+		1. Let _rx_ be ? RegExpCreate(_regexp_, `"g"`).


jridgewell · 2018-08-08T19:56:19Z

spec.emu

-			1. Let _fullUnicode_ be *false*.
-			1. Assert: ! Get(_matcher_, `"lastIndex"`) is *0*.
+		1. Let _S_ be ? ToString(_string_).
+		1. Let _C_ be ? SpeciesConstructor(_R_, %RegExp%).


Can you explain why it's necessary to reconstruct a RegExp here? @@match doesn't behave like this.

Because I don't want mutations to .lastIndex to be observable, so matchAll intentionally clones the regex to prevent that.

matchAll provides an iterator, so it's much more important to avoid observable mutations over time, and also to ensure that mutating the lastIndex of the regex mid-iteration doesn't mess with the algorithm.

Is there an issue explaining this behavior? It seems we could use a internal object to store this state, instead of calling the @@species hook a second time.

A second time? it's only called once in this spec - RegExpCreate doesn't call it.

Ha, I got confused with the changes. Never mind.

- `String.prototype.matchAll`: - use `RegExpCreate` when `Symbol.prototype.matchAll` is not found - fall back to regex coercion otherwise - `RegExp.prototype[Symbol.matchAll]`: - receiver is assumed to be a regex implicitly - remove `MatchAllIterator` abstract operation Thus, `IsRegExp` call no longer exists. Addresses #21. Addresses #34. Closes #37.

ljharb · 2018-08-08T20:38:41Z

Now that I have more clarity on the committee consensus, on the objections to IsRegExp, and on the consistency arguments about the Symbol.matchAll method on RegExp.prototype, I think I understand what changes are needed.

I've put up #38; @mathiasbynens, does that alternate approach address your concerns?

mathiasbynens · 2018-08-09T07:23:16Z

I don't understand why we need the alternate approach. It looks like you're still trying to support delete RegExp.prototype[Symbol.matchAll] — is that it? That's one of the things #37 is supposed to fix, per the discussion in #34 (comment), where @anba, @littledan, @tschneidereit all spoke up in favor of consistency over deviating from the status quo in this edge case. It seems weird to simply dismiss this feedback.

ljharb · 2018-08-09T07:28:25Z

Indeed I am; commented here: #38 (comment)

schuay · 2018-08-09T08:25:07Z

spec.emu

@@ -20,7 +20,9 @@ contributors: Jordan Harband
 			1. Let _matcher_ be ? GetMethod(_regexp_, @@matchAll).
 			1. If _matcher_ is not *undefined*, then
 				1. Return ? Call(_matcher_, _regexp_, &laquo; _O_ &raquo;).
-		1. Return ? MatchAllIterator(_regexp_, _O_).


FWIW I agree that String.p.matchAll should be specced consistently with String.p.match and String.p.search. These are the only other 'regexp' methods defined on String. Is there a reason to diverge from their behavior?

Likewise in RegExp.p[@@matchAll].

schuay · 2018-08-09T08:37:42Z

spec.emu

+		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;).
+		1. Let _global_ be ? ToBoolean(? Get(_matcher_, `"global"`)).


@@split detects flag values of the resulting regexp by checking the string value of flags. It would probably make sense to do the same here: 1. for consistency, 2. it would let us avoid two ToBoolean(Get) sequences. See https://tc39.github.io/ecma262/#sec-regexp.prototype-@@split:

If flags contains "u", let unicodeMatching be true.

Good call. Instead of e.g.:

1. Let _global_ be ? ToBoolean(? Get(_matcher_, `"global"`)).

…we should do:

1. If _flags_ contains `"g"`, let _global_ be *true*. 1. Else, let _global_ be *false*.

…to follow the precedent.

Sure, that seems reasonable. I'll do that directly in master since that's an observability improvement instead of API design questions.

@split

This follows the convention used by `RegExp.prototype[@@split]`: https://tc39.github.io/ecma262/#sec-regexp.prototype-@@split

littledan · 2018-08-09T10:47:42Z

I agree with the others here who spoke up in favor of consistency. I don't see the need to be defensive of this particular case. I'm wondering, why were several comments marked as "outdated"? This marking makes it harder for me to read the thread.

ljharb · 2018-08-09T17:25:25Z

@littledan the one outdated comment thread is from a mistaken comment by me that justin corrected; nobody needs to read that thread.

- `String.prototype.matchAll`: - use `RegExpCreate` when `Symbol.prototype.matchAll` is not found - fall back to regex coercion otherwise - `RegExp.prototype[Symbol.matchAll]`: - receiver is assumed to be a regex implicitly - remove `MatchAllIterator` abstract operation Thus, `IsRegExp` call no longer exists. Addresses #21. Addresses #34. Closes #37.

This was referenced Aug 8, 2018

Re-evaluate IsRegExp call in MatchAllIterator? #34

Closed

Path to Stage 4! #21

Closed

ljharb suggested changes Aug 8, 2018

View reviewed changes

jridgewell reviewed Aug 8, 2018

View reviewed changes

ljharb mentioned this pull request Aug 8, 2018

[spec] Adjust to committee feedback #38

Merged

schuay reviewed Aug 9, 2018

View reviewed changes

Detect flag values by checking flags

5b99875

This follows the convention used by `RegExp.prototype[@@split]`: https://tc39.github.io/ecma262/#sec-regexp.prototype-@@split

ljharb closed this in #38 Aug 25, 2018

mathiasbynens deleted the suggestion-from-anba branch September 17, 2018 06:55

mathiasbynens mentioned this pull request Sep 17, 2018

Committee feedback: fallback behavior #39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve consistency with existing RegExp methods and subclass handling #37

Improve consistency with existing RegExp methods and subclass handling #37

mathiasbynens commented Aug 8, 2018 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

ljharb Aug 8, 2018

mathiasbynens Aug 9, 2018

ljharb Aug 9, 2018

schuay Aug 9, 2018

ljharb Aug 9, 2018

anba Aug 17, 2018

This comment was marked as outdated.

jridgewell Aug 8, 2018

ljharb Aug 8, 2018

jridgewell Aug 8, 2018

ljharb Aug 8, 2018

jridgewell Aug 8, 2018

ljharb commented Aug 8, 2018

mathiasbynens commented Aug 9, 2018

ljharb commented Aug 9, 2018

schuay Aug 9, 2018

schuay Aug 9, 2018

mathiasbynens Aug 9, 2018 •

edited

Loading

ljharb Aug 9, 2018

littledan commented Aug 9, 2018

ljharb commented Aug 9, 2018

Improve consistency with existing RegExp methods and subclass handling #37

Improve consistency with existing RegExp methods and subclass handling #37

Conversation

mathiasbynens commented Aug 8, 2018 • edited Loading

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

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

This comment was marked as outdated.

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 Aug 8, 2018

mathiasbynens commented Aug 9, 2018

ljharb commented Aug 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathiasbynens Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

littledan commented Aug 9, 2018

ljharb commented Aug 9, 2018

mathiasbynens commented Aug 8, 2018 •

edited

Loading

mathiasbynens Aug 9, 2018 •

edited

Loading