Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

[spec] Adjust to committee feedback #38

Merged
merged 1 commit into from
Aug 25, 2018
Merged

Conversation

ljharb
Copy link
Member

@ljharb ljharb commented Aug 8, 2018

  • 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.

 - `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.
@schuay
Copy link

schuay commented Aug 9, 2018

cc @peterwmwong

@mathiasbynens
Copy link
Member

cc @anba

@mathiasbynens
Copy link
Member

mathiasbynens commented Aug 9, 2018

@ljharb Can you elaborate on how this is different from #37? 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
Copy link
Member Author

ljharb commented Aug 9, 2018

That desire was brought up in committee in January, and there were no objections to doing so. If you think it needs to be brought before the committee again, I can add it to the agenda (noting that the "status quo" is neither, because some methods do one thing and some do another - so I've chosen the more robust approach).

@mathiasbynens
Copy link
Member

There were no objections in January, but the abovementioned discussion took place after that.

noting that the "status quo" is neither, because some methods do one thing and some do another

Can you list the other methods that are robust against delete in this way? This counter-argument wasn't brought up in #34 AFAICT.

@ljharb
Copy link
Member Author

ljharb commented Aug 9, 2018

@mathiasbynens Sure!

vs the ones without a fallback in the case of a missing symbol method:

My argument includes that "match" is a special method - its presence means IsRegExp - and thus it has two conflated purposes: matching, and marking regexes. As such, it would be pretty meaningless to have fallback behavior for the result of RegExpCreate if it was not a regexp (by lacking the Symbol.match method). However, matchAll only has the purpose of "matching" - and so if it's lacking the Symbol.matchAll method, but has Symbol.match, then it's still a regex, so the fallback behavior makes sense.

If you take away the methods from the above list that have dual purposes - namely, exec and match - then search is the standout (vs replace + split) - I'm not sure why it's the only one that lacks both a fallback and the "dual-purpose" justification, but I assume it's "that's the way browsers implemented it", so ES6 was able to get away with it. I think that matchAll having the fallback both matches the status quo, and also is more usable for users on the web who want their code to be robust, and don't want to have to extract even more methods to be so.

@ljharb
Copy link
Member Author

ljharb commented Aug 15, 2018

I'd like to merge this now - it seems as if doing so would leave only one outstanding item: what should happen when delete RegExp.prototype[Symbol.matchAll] (current behavior remains as it has always been in this proposal: it falls back to as if the original intrinsic was invoked).

Are there any objections to me merging this, and continuing that discussion in a new issue if there's still a need to pursue it?

@mathiasbynens
Copy link
Member

I’d like to hear from @anba, @littledan, and @tschneidereit first.

@anba
Copy link
Collaborator

anba commented Aug 17, 2018

It looks like the proposed changes here and the ones in #37 for RegExp.p[matchAll] are the same. So that's good to go, from my point of view. The only difference is about String.p.matchAll, but that's now going to be discussed in the next TC39 meeting, so I'd say it's okay to merge this PR now.

@anba
Copy link
Collaborator

anba commented Aug 17, 2018

If you take away the methods from the above list that have dual purposes - namely, exec and match - then search is the standout (vs replace + split) [...]

Mirroring my comment at #37 (comment)

The fallback in 'replace' and 'split' is different, because it defaults to perform String instead of RegExp matching (i.e. "." only matches the code point U+002E (FULL STOP)). Which would mean for String.p.matchAll, if it should be aligned to those two methods, it should also be changed to perform String matching. But then would make it completely different from String.p.match, which is going to be confusing, isn't it?

@tschneidereit
Copy link
Member

I agree with @anba, but I also don't feel all too strongly about this, fwiw. I certainly wouldn't hold up anything over the remaining differences. However, bringing them to the next meeting does seem reasonable, so 👍 on that.

@ljharb ljharb merged commit 7670a0b into master Aug 25, 2018
@ljharb ljharb deleted the final_isregexp_changes branch August 25, 2018 05:53
zloirock added a commit to zloirock/core-js that referenced this pull request Aug 28, 2018
xtuc pushed a commit to xtuc/v8 that referenced this pull request Nov 19, 2018
…/proposal-string-matchall#38)

- Removes IsRegExp check and special handling when false
- Removes MatchAllIterator
- Extracts previously inlined CreateRegExpStringIterator
- Update comments to match spec text and numbering

Bug: v8:6890
Change-Id: Ie81757a499acc77910f029835fb042e70d86d83d
Reviewed-on: https://chromium-review.googlesource.com/c/1317830
Commit-Queue: Peter Wong <peter.wm.wong@gmail.com>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57488}
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants