Editorial: Fix wording bug by tweaking CapturingGroupName #2722
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
First commit LGTM. Second doesn't really seem like an improvement to me, but we can see what @michaelficarra thinks. |
|
Other editors were in favor of the second commit, so let's keep it. |
jmdyck
force-pushed
the
CapturingGroupName
branch
2 times, most recently
from
b6ac2bd to
e50cac4
Compare
jmdyck
force-pushed
the
CapturingGroupName
branch
from
e50cac4 to
dbb1b41
Compare
bakkot
approved these changes
Apr 27, 2022
jmdyck
force-pushed
the
CapturingGroupName
branch
from
dbb1b41 to
46a06cf
Compare
|
(force-pushed to squash today's 3 commits into one) |
bakkot
added
the
ready to merge
A GroupSpecifier can enclose/contain *many* RegExpIdentifierNames, which causes some wording to have an unintended meaning. Fix this by changing the CapturingGroupName SDO to operate on the GroupName rather than the RegExpIdentifierName. (See PR for a more detailed explanation.)
That is, change:
Atom :: `(` GroupSpecifier Disjunction `)`
GroupSpecifier[UnicodeMode] :: [empty] | `?` GroupName
to:
Atom :: `(` GroupSpecifier? Disjunction `)`
GroupSpecifier[UnicodeMode] :: `?` GroupName
(and similarly for ExtendedAtom).
This doesn't change the language, it just means that
when we have a GroupSpecifier "in hand", we know that it has a GroupName child.
(As for the case where a GroupSpecifier is empty/omitted, the spec
doesn't specifically talk about that, so no changes are necessary.)
---
Extend CapturingGroupName to GroupSpecifier
Now that GroupSpecifier is defined by a chain production,
we can apply CapturingGroupName to a GroupSpecifier,
and let the chain rule handle it.
---
Reword the early error rule involving CapturingGroupName
Currently, the spec talks about
"multiple [things that] have the same CapturingGroupName"
which is unusual wording by which to invoke an SDO.
Change it to:
"two or more [things] for which CapturingGroupName of [thing] is the same"
which is still unusual, but maybe less so.
ljharb
force-pushed
the
CapturingGroupName
branch
from
46a06cf to
4052033
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While reviewing #2721, I noticed a wording bug in the current spec:
Because the nonterminal RegExpIdentifierName is defined recursively, a GroupSpecifier usually contains/encloses multiple RegExpIdentifierNames.
So in a phrasing such as:
a |GroupSpecifier| containing a |RegExpIdentifierName| which has ...the spec only intends the 'topmost' RegExpIdentifierName, but any of the 'deeper' RegExpIdentifierNames could satisfy the phrase.
E.g., consider
/(?<NotTheOne>foo)\k<TheOne>/. This should be a syntax error, because the AtomEscape\k<TheOne>doesn't have a matching GroupSpecifier. But the GroupSpecifier?<NotTheOne>does contain/enclose a RegExpIdentifierNameTheOnethat matches the GroupName of the AtomEscape, and so there isn't a syntax error.There are 3 spots in the spec that are subject to this bug. This PR fixes them.
It's possible to select the 'topmost' RegExpIdentifierName just by tweaking the wording at the problem spots. However, this would lengthen some already-lengthy sentences (because you have to involve the GroupName parent of the RegExpIdentifierName).
But I noticed that the only reason we're accessing the RegExpIdentifierName is to invoke
CapturingGroupNameon it. So if we changeCapturingGroupNameto instead operate on the GroupName, then accessing its RegExpIdentifierName child (and not any deeper RegExpIdentifierName) becomes the easiest thing to do, and everything (including the definition ofCapturingGroupNameitself) gets slightly simpler.That's the first commit.
(There are no references to
CapturingGroupNamein downstream specs.)The second commit is a bonus. It factors out the idea of finding the GroupSpecifier(s) that match a given GroupName, and presents it as the abstract operation
GroupSpecifiersThatMatch. (This should make the changes of #2721 a bit easier too.)