Skip to content
This repository has been archived by the owner on Feb 19, 2021. It is now read-only.

Throw if searchValue is a non-global RegExp #24

Merged
merged 3 commits into from Sep 23, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 6 additions & 5 deletions README.md
Expand Up @@ -44,15 +44,16 @@ It also removes the need to escape special regexp characters (note the unescaped

The proposed signature is the same as the existing `String.prototype.replace` method:

```
```js
String.prototype.replaceAll(searchValue, replaceValue)
```

Per the current TC39 consensus, `String.prototype.replaceAll` behaves identically to `String.prototype.replace` in all cases, **except** for the case where `searchValue` is a string.
Per the current TC39 consensus, `String.prototype.replaceAll` behaves identically to `String.prototype.replace` in all cases, **except** for the following two cases:

In that case, `String.prototype.replace` only replaces a single occurrence of the `searchValue`, whereas `String.prototype.replaceAll` replaces *all* occurrences of the `searchValue` (as if `.split(searchValue).join(replaceValue)` or a global & properly-escaped regular expression had been used).
1. If `searchValue` is a string, `String.prototype.replace` only replaces a single occurrence of the `searchValue`, whereas `String.prototype.replaceAll` replaces *all* occurrences of the `searchValue` (as if `.split(searchValue).join(replaceValue)` or a global & properly-escaped regular expression had been used).
1. If `searchValue` is a non-global regular expression, `String.prototype.replace` replaces a single match, whereas `String.prototype.replaceAll` throws an exception. This is done to avoid the inherent confusion between the lack of a global flag (which implies "do NOT replace all") and the name of the method being called (which strongly suggests "replace all").

Notably, `String.prototype.replaceAll` behaves just like `String.prototype.replace` if `searchValue` is a regular expression, [including if it's a non-global regular expression](https://github.com/tc39/proposal-string-replaceall/issues/16).
Notably, `String.prototype.replaceAll` behaves just like `String.prototype.replace` if `searchValue` is a global regular expression.

## Comparison to other languages

Expand Down Expand Up @@ -87,7 +88,7 @@ A: This is an awkward interface — because the default limit is 1, the user wou
## TC39 meeting notes

- [November 2017](https://tc39.es/tc39-notes/2017-11_nov-28.html#10ih-stringprototypereplaceall-for-stage-1)
- [March 2019](https://github.com/rwaldron/tc39-notes/blob/master/meetings/2019-03/mar-26.md#stringprototypereplaceall-for-stage-2)
- [March 2019](https://github.com/tc39/tc39-notes/blob/master/meetings/2019-03/mar-26.md#stringprototypereplaceall-for-stage-2)
- [July 2019](https://github.com/tc39/tc39-notes/blob/master/meetings/2019-07/july-24.md#stringprototypereplaceall)

## Specification
Expand Down
30 changes: 29 additions & 1 deletion spec.html
Expand Up @@ -42,8 +42,13 @@ <h1>String.prototype.replaceAll ( _searchValue_, _replaceValue_ )</h1>
<emu-alg>
1. Let _O_ be ? RequireObjectCoercible(*this* value).
1. If _searchValue_ is neither *undefined* nor *null*, then
1. Let _isRegExp_ be ? IsRegExp(_searchString_).
1. If _isRegExp_ is true, then
1. Let _flags_ be ? Get(_searchValue_, *"flags"*).
1. If _flags_ is not *undefined*, then
1. If ? ToString(_flags_) does not contain *"g"*, throw a *TypeError* exception.
1. Let _replacer_ be ? GetMethod(_searchValue_, @@replace).
2. If _replacer_ is not *undefined*, then
1. If _replacer_ is not *undefined*, then
1. Return ? Call(_replacer_, _searchValue_, « _O_, _replaceValue_ »).
1. Let _string_ be ? ToString(_O_).
1. Let _searchString_ be ? ToString(_searchValue_).
Expand Down Expand Up @@ -76,3 +81,26 @@ <h1>String.prototype.replaceAll ( _searchValue_, _replaceValue_ )</h1>
1. Return _result_.
</emu-alg>
</emu-clause>

<emu-clause id="sec-string.prototype.matchall">
<h1>String.prototype.matchAll ( _regexp_ )</h1>
<p>Performs a regular expression match of the String representing the *this* value against _regexp_ and returns an iterator. Each iteration result's value is an Array object containing the results of the match, or *null* if the String did not match.</p>
<p>When the `matchAll` method is called, the following steps are taken:</p>
<emu-alg>
1. Let _O_ be ? RequireObjectCoercible(*this* value).
1. If _regexp_ is neither *undefined* nor *null*, then
1. <ins>Let _isRegExp_ be ? IsRegExp(_regexp_).</ins>
1. <ins>If _isRegExp_ is true, then</ins>
1. <ins>Let _flags_ be ? Get(_regexp_, *"flags"*).</ins>
1. <ins>If _flags_ is not *undefined*, then</ins>
1. <ins>If ? ToString(_flags_) does not contain *"g"*, throw a *TypeError* exception.</ins>
1. Let _matcher_ be ? GetMethod(_regexp_, @@matchAll).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does seem kind of odd that a non RegExp can have any kind of non “all” behavior it wants, but an actual RegExp is forced to have the proper “all” behavior. Not sure anything can be done about that, but it is an inconsistency introduced by this change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you're saying. Still, there's value in making built-ins work well / in unsurprising ways by default, even if for userland subclasses, this is up to the user.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we only/mostly care about built-ins, I wonder if makes more sense to move this check into https://tc39.es/ecma262/#sec-regexp-prototype-matchall?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I don't have a strong preference tbh. Any opinions? @schuay @ljharb

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinion. One argument for the current spot is that the location is consistent between matchAll and replaceAll. There's no @@replaceAll so we could not consistently move both checks to a RegExp builtin. I'm fine with either way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the motivation/value of moving it inside the symbol methods is so a subclass could override the behavior - ie, could lack the g flag but still be “all” (vice versa is already going to be possible). I don’t see any benefit in allowing regex subclasses to deviate from this rather strong guard that we’ll have decided on - without concrete use cases, i think here is probably a better spot.

1. If _matcher_ is not *undefined*, then
1. Return ? Call(_matcher_, _regexp_, &laquo; _O_ &raquo;).
1. Let _S_ be ? ToString(_O_).
1. Let _rx_ be ? RegExpCreate(_regexp_, `"g"`).
1. Return ? Invoke(_rx_, @@matchAll, &laquo; _S_ &raquo;).
</emu-alg>
<emu-note>The `matchAll` function is intentionally generic, it does not require that its *this* value be a String object. Therefore, it can be transferred to other kinds of objects for use as a method.</emu-note>
<emu-note>Similarly to `String.prototype.split`, `String.prototype.matchAll` is designed to typically act without mutating its inputs.</emu-note>
</emu-clause>