[Normative] Add RegExp named capture groups #1027

mathiasbynens · 2017-11-11T20:21:13Z

@littledan asked me to prepare the PR for his proposal adding named capture groups to ECMAScript regular expressions.

Proposal repo: https://github.com/tc39/proposal-regexp-named-groups

@littledan, I’ve added you as the commit author.

ljharb · 2017-11-11T20:50:53Z

spec.html

+                        1. Let the enclosed substring be _groupName_.
+                        1. Let _capture_ be ? Get(_namedCaptures_, _groupName_).
+                        1. If _capture_ is *undefined*, replace the text through `>` with the empty string.
+                        1. Otherwise, replace the text through this following `>` with ? ToString(_capture_).


this should probably be worded a bit differently to ensure that ToString is only called once on capture - otherwise this could be interpreted as "call ToString for each replacement", which would be observable.

How about:

Otherwise, let _replacement_ be ? ToString(_capture_) and replace the text through this following `>` with _replacement_.

That seems completely unambiguous, LGTM

I don't really understand how this change relates to what @ljharb is getting at. Get and ToString are called multiple times, before and after this editorial change.

But it does penalize the common case; if done eagerly, we'd need to iterate all keys on the groups object, while the replacement string may only reference a subset of them.

And since I suppose we wouldn't want to modify the given groups object, we'd have to allocate a new object somewhere to store the ToString normalized group values, even in the lazy case.

I didn't get the feeling that anyone was saying "we missed this stage 2 concern".

The comment about it “being a little late” seemed like that.

At any rate, allocating a new unobservanle object somewhere to store a string of Type(replacement) is not String, when in the common case it would be, seems like it would be both a cheap and a rare operation. However my concern is not for performance, which can always be improved upon later; but rather for observability, which can not be reduced later.

I don't think there was a lack of review. These semantics were thought out, put out for review for 8 months ago, and now had a thorough review by two implementers. I was aware when writing the spec text that you'd get multiple observable reads. As @schuay raises, it's not clear what the alternative would be, and the downside of the current approach does not seem so high.

RegExps are extremely observable in their behavior in a way which I don't see much of a use case for. I raised this as a concern in late 2015, proposing a more minimal RegExp subclassing mechanism, which would've been much easier to implement and with fewer performance cliffs. The proposal was rejected by the committee. We have a specification with very high observability due to that decision; it will impact many new RegExp features going forward no matter whether we decide to do one or two Get/ToString calls. If we did caching, it would also be observable.

Fair point :-)

Misunderstanding noted, and reverted to the original spec text.

littledan

LGTM, though I'd appreciate other reviewers also took a look.

anba · 2017-12-08T21:53:11Z

I think it'd be nice to resolve tc39/proposal-regexp-named-groups#34 before merging this PR, because both implementations (JSC and V8) don't implement this part correctly according to the current spec proposal (both implementations print --$<foo>-- for the test case in that issue, but the current spec proposal requires to return --bar--).

mathiasbynens · 2017-12-13T14:17:59Z

tc39/proposal-regexp-named-groups#34 has been resolved in tc39/proposal-regexp-named-groups#40. I’ve updated this patch accordingly.

mathiasbynens · 2017-12-21T05:59:12Z

Test262 tests for the recent groups change have landed: tc39/test262#1376

schuay

LGTM, thanks :)

ljharb

LGTM, but i'm not familiar enough with grammar stuff to review that part.

ljharb · 2017-12-21T17:12:34Z

spec.html

+            1. If _s_ is *undefined*, return _c_(_x_).
+            1. Let _e_ be _x_'s _endIndex_.
+            1. Let _len_ be the number of elements in _s_.
+            1. Let _f_ be _e_+_len_.


spaces around the + for clarity?

ljharb · 2017-12-21T17:12:42Z

spec.html

+            1. Let _e_ be _x_'s _endIndex_.
+            1. Let _len_ be the number of elements in _s_.
+            1. Let _f_ be _e_+_len_.
+            1. If _f_&gt;_InputLength_, return ~failure~.


also spaces around the >?

ljharb · 2017-12-21T17:12:52Z

spec.html

+            1. Let _len_ be the number of elements in _s_.
+            1. Let _f_ be _e_+_len_.
+            1. If _f_&gt;_InputLength_, return ~failure~.
+            1. If there exists an integer _i_ between 0 (inclusive) and _len_ (exclusive) such that Canonicalize(_s_[_i_]) is not the same character value as Canonicalize(_Input_[_e_+_i_]), return ~failure~.


and the e + i?

Done, but keep in mind that this is now inconsistent with other similar parts of the RegExp section. There is an open issue to fix this throughout the spec: #925

bterlson · 2018-01-17T22:44:58Z

spec.html

@@ -28558,6 +28558,8 @@ <h1>Runtime Semantics: GetSubstitution( _matched_, _str_, _position_, _captures_
            1. Assert: Type(_replacement_) is String.
            1. Let _tailPos_ be _position_ + _matchLength_.
            1. Let _m_ be the number of elements in _captures_.
+            1. If _namedCaptures_ is not *undefined*,


nit: block form of If ends in a ", then":

bterlson · 2018-01-17T22:46:16Z

spec.html

+                  <emu-alg>
+                    1. If _namedCaptures_ is *undefined*, the replacement text is the String `"$&lt;"`.
+                    1. Otherwise,
+                      1. Scan until the next `>`.


Probably could use the new single unicode codepoint convention here (U+XXXX CHAR NAME)

bterlson · 2018-01-17T22:48:56Z

I think this is ready to merge. I only found a few nits. The grammar all looks good, editorial conventions are good, merges and builds cleanly. Nice work!!

I had a couple minor nits but I'll pull by EOD and just fix them in a subsequent commit unless you want to get to it before me :)

ljharb · 2018-01-17T23:07:38Z

@bterlson does that mean that, per https://github.com/rwaldron/tc39-notes/blob/622ea4d93b4be44eb834d5e90bfebe0403f4216a/es8/2017-11/nov-28.md#conclusionresolution-10, this is now at stage 4?

mathiasbynens · 2018-01-22T20:28:46Z

@bterlson Thanks for the review! Nits addressed.

@ljharb I think it’s officially stage 4 once the PR gets merged.

Proposal repo: https://github.com/tc39/proposal-regexp-named-groups

littledan · 2018-01-23T22:10:05Z

emoji kazoo react

Specifically, add [N] parameter to CharacterClass ClassRanges NonemptyClassRanges NonemptyClassRangesNoDash ClassAtom These were implied when commit 95ec0c6 (of PR tc39#1027)... - added [?N] to RHS occurrences of CharacterClass without explicitly adding [N] to the LHS occurrence CharacterClass; and - added [N] to the LHS occurrence of ClassAtomNoDash (in Annex B) without adding [?N] to any RHS occurrence. This commit propagates [N] across that gap. (See issue tc39#1081.)

mathiasbynens force-pushed the named-captures branch from 7dd277f to 9af3ff8 Compare November 11, 2017 20:22

ljharb reviewed Nov 11, 2017

View reviewed changes

mathiasbynens force-pushed the named-captures branch from 9af3ff8 to 60ec63e Compare November 11, 2017 22:47

mathiasbynens added the pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. label Nov 11, 2017

littledan approved these changes Nov 17, 2017

View reviewed changes

mathiasbynens force-pushed the named-captures branch from 60ec63e to 6fe708f Compare November 17, 2017 16:00

mathiasbynens requested a review from bterlson December 5, 2017 00:37

bterlson force-pushed the named-captures branch from 6fe708f to a581524 Compare December 8, 2017 21:33

mathiasbynens force-pushed the named-captures branch from a581524 to 8afa992 Compare December 13, 2017 14:16

adrianheine mentioned this pull request Dec 17, 2017

Advanced to stage 4 tc39/proposal-regexp-named-groups#41

Closed

ljharb mentioned this pull request Dec 20, 2017

Move named capture groups to finished proposals tc39/proposals#102

Merged

ljharb added the has test262 tests label Dec 21, 2017

schuay approved these changes Dec 21, 2017

View reviewed changes

ljharb approved these changes Dec 21, 2017

View reviewed changes

mathiasbynens force-pushed the named-captures branch from 8afa992 to 9bfcabe Compare December 21, 2017 17:23

This was referenced Dec 21, 2017

Regex named capture groups babel/proposals#35

Closed

Add support for named capturing groups jviereck/regjsparser#83

Merged

bterlson reviewed Jan 17, 2018

View reviewed changes

mathiasbynens force-pushed the named-captures branch from 9bfcabe to 2fedbb1 Compare January 22, 2018 20:28

[Normative] Add RegExp named capture groups

898d7ae

Proposal repo: https://github.com/tc39/proposal-regexp-named-groups

mathiasbynens force-pushed the named-captures branch from 2fedbb1 to 898d7ae Compare January 22, 2018 22:18

bterlson merged commit 95ec0c6 into tc39:master Jan 23, 2018

nicolo-ribaudo mentioned this pull request Jan 23, 2018

Add @babel/plugin-transform-named-capturing-groups-regex babel/babel#7105

Merged

ljharb removed the pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. label Jan 23, 2018

jaredh159 mentioned this pull request May 3, 2019

downlevel regexp named capture groups microsoft/TypeScript#31241

Closed

5 tasks

jmdyck mentioned this pull request Aug 20, 2019

bug? re named groups in Annex B #1673

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Normative] Add RegExp named capture groups #1027

[Normative] Add RegExp named capture groups #1027

mathiasbynens commented Nov 11, 2017 •

edited

Loading

ljharb Nov 11, 2017

mathiasbynens Nov 11, 2017

ljharb Nov 11, 2017

mathiasbynens Nov 11, 2017

littledan Nov 16, 2017 •

edited

Loading

schuay Nov 17, 2017

ljharb Nov 17, 2017

littledan Nov 17, 2017

ljharb Nov 17, 2017

mathiasbynens Nov 17, 2017

littledan left a comment

anba commented Dec 8, 2017

mathiasbynens commented Dec 13, 2017

mathiasbynens commented Dec 21, 2017

schuay left a comment

ljharb left a comment

ljharb Dec 21, 2017

ljharb Dec 21, 2017

ljharb Dec 21, 2017

mathiasbynens Dec 21, 2017

bterlson Jan 17, 2018

bterlson Jan 17, 2018

bterlson commented Jan 17, 2018

ljharb commented Jan 17, 2018

mathiasbynens commented Jan 22, 2018 •

edited

Loading

littledan commented Jan 23, 2018

[Normative] Add RegExp named capture groups #1027

[Normative] Add RegExp named capture groups #1027

Conversation

mathiasbynens commented Nov 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

littledan Nov 16, 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

littledan left a comment

Choose a reason for hiding this comment

anba commented Dec 8, 2017

mathiasbynens commented Dec 13, 2017

mathiasbynens commented Dec 21, 2017

schuay left a comment

Choose a reason for hiding this comment

ljharb 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

bterlson commented Jan 17, 2018

ljharb commented Jan 17, 2018

mathiasbynens commented Jan 22, 2018 • edited Loading

littledan commented Jan 23, 2018

mathiasbynens commented Nov 11, 2017 •

edited

Loading

littledan Nov 16, 2017 •

edited

Loading

mathiasbynens commented Jan 22, 2018 •

edited

Loading