Repository navigation
fix: handle a backreference to a non-participating capture group - #460
snowyukitty wants to merge 3 commits into
Conversation
FindStringSubmatchIndex returns -1 offsets for a capture group that did
not participate in the match, such as the inner group of `(x)*` when it
repeats zero times. Slicing the input with those offsets panicked with
"slice bounds out of range [:-1]" for any rule that has an action.
Record a capture as its text plus whether the group participated, and
expand a backreference to a group that did not participate into an
expression that can never match rather than into nothing:
- groups[n] stays capture group n, so a backreference in a pushed state
still resolves to the group its pattern names;
- "\1" cannot match when group 1 did not participate, as in engines with
real backreferences, while "\1?" stays optional;
- a group that participated and matched the empty string still expands
to an empty match, which is a different thing.
An expansion is now always a single group, so that a quantified or
alternated backreference still parses when the capture is empty. "\1?"
previously expanded to "?" and failed to compile.
Fixes alecthomas#324
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be1cb9db59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !groups[n].present { | ||
| return prefix + neverMatch | ||
| } | ||
| return prefix + "(?:" + regexp.QuoteMeta(groups[n].text) + ")" |
There was a problem hiding this comment.
When a child rule uses a backreference inside a character class, this unconditional wrapper changes the class itself. For example, [\1] with capture a previously expanded to [a], but now expands to [(?:a)], which also matches (, ?, :, and ). Such an unexpected character can trigger the rule and its state action—for example, a Pop()—instead of falling through to the correct rule. Preserve character-class expansion semantics while applying grouping only where a subexpression is valid.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is right, and it took two commits to fix properly. The branch is now at 291bfb4 — git diff --stat master...291bfb4 is 2 files, 339 insertions, 18 deletions.
It understates the problem in one place. [\1] capturing a did expand to [(?:a)], a class that also matches (, ?, : and ) — but [[:alpha:]\1] capturing - expanded to [[:alpha:](?:-)], which does not compile at all, because RE2 reads :-) as a character range. A lexer definition that builds on master failed to build on this branch.
It is this branch's regression, not a defect in master. The grouping was added so that a quantified or alternated backreference (\1?, a|\1) still parses when the capture is empty. That is correct everywhere except inside a character class, where (?: is not grouping syntax — it is four more members of the set.
The fix, in 11b1093. The expansion now depends on where the backreference sits. Outside a class it is unchanged. Inside one it is the quoted capture alone, exactly as master does it, and a capture that did not participate contributes no members rather than the zero-width expression a class cannot hold — a class left with nothing then fails to compile, which is what an empty capture in that position has always done, so no new failure mode appears. characterClassSpans finds those positions in one pass; it has to know about escapes and \Q...\E runs, and about POSIX names such as [:alpha:], whose ] closes the name rather than the class.
And one more class shape, in 291bfb4. That scanner closed a class on the first ] it saw. RE2 does not: a ] that is the first member — right after [ or [^ — stands for itself, so []a] is a two-member class rather than an empty one. []\1] therefore looked to the scanner like a backreference outside any class, and was wrapped again:
pattern capture master be1cb9d 11b1093 291bfb4
[\1] a [a] [(?:a)] [a] [a]
[[:alpha:]\1] - [[:alpha:]-] does not compile [[:alpha:]-] [[:alpha:]-]
[]\1] a []a] [](?:a)] [](?:a)] []a]
[^]\1] a [^]a] [^](?:a)] [^](?:a)] [^]a]
Two things left deliberately alone: QuoteMeta does not escape -, so [\1] capturing a-z is a range on master and remains one here; and [] with nothing left in it still fails to compile, as above.
Verification, from the repository root at 291bfb4:
go test ./lexer/ -run 'TestStatefulLexer/Backref'
13 subtests pass, five of them new. Each fails with the corresponding revision of lexer/stateful.go restored and the tests kept — three against be1cb9d and two against 11b1093 — and the failures are of three different kinds, not one:
BackrefInsideNegatedCharacterClassandBackrefAfterLeadingBracketInNegatedCharacterClassstop withlexer: invalid input text, because a character the class should not hold ends the run early.BackrefInsideCharacterClassandBackrefAfterLeadingBracketInCharacterClassreturn the wrong tokens, joining runs that should be separate.BackrefBesidePosixNameInCharacterClassnever builds at all:
lexer: rule "Letters": invalid backref expansion: "[[:alpha:](?:-)]+": error parsing regexp: invalid character class range: `:-)`
go test ./... passes everywhere except lexer/internal/conformance, which fails identically at be1cb9d, 11b1093 and 291bfb4 with exec: "../../../scripts/participle": executable file not found because it needs a helper binary I do not build here. I am not claiming a clean full-suite run.
The grouping this change added so that `\1?` and `a|\1` still parse when the capture is empty is not grouping inside a character class. `[\1]` capturing `a` expanded to `[(?:a)]`, a class that also matches `(`, `?`, `:` and `)`, and `[[:alpha:]\1]` capturing `-` expanded to `[[:alpha:](?:-)]`, which does not compile at all because `:-)` is read as a range. The expansion now depends on where the backreference sits. Outside a class it is unchanged. Inside one it is the quoted capture alone, as it was before this branch, and a capture that did not participate contributes no members rather than a zero-width expression that a class cannot hold. A class left with nothing fails to compile, which is what an empty capture in that position has always done. `characterClassSpans` finds those positions in one pass. It has to know about escapes, `\Q...\E` runs, and POSIX names such as `[:alpha:]`, whose `]` closes the name rather than the class.
The previous commit decided where a backreference sits by scanning for
character classes, and that scanner closed a class on the first "]" it
found. RE2 does not: a "]" that is the first member of a class -- right
after "[" or "[^" -- stands for itself, so "[]a]" is the two-member class
it accepts rather than an empty class it rejects.
So "[]\1]" put the backreference outside a class as far as the scanner was
concerned, and it was expanded as a subexpression again: capturing "a" it
became "[](?:a)]", a class that also matches "(", "?", ":" and ")". That is
the same defect the previous commit fixed for "[\1]", surviving in the one
class shape that shape does not cover.
firstClassMember recognises those two positions, and the scanner leaves the
class open across them.
Fixes #324.
Impact
A stateful-lexer rule that carries an action (
Push,Pop,Include) and whose pattern has a capture group that can fail to participate — the inner group of([[:alnum:]])*, or(-)?— panics the lexer:Reproduced on current
masterwith the grammar from #324:"%x + y"panics,"aa + bb"is fine. The reporter's workaround is to make the group non-capturing.Root cause
FindStringSubmatchIndexreturns-1, -1for a group that did not participate in the match, andStatefulLexer.Nextsliced the input with those offsets. That vector is also what a pushed state's backreferences resolve against, so removing the panic means deciding what a backreference to an absent group means.Fix
A capture is now its text plus whether the group participated, and a backreference always expands to exactly one subexpression:
That gives the behaviour of engines with real backreferences:
(a)?+\1ENDEND(a*)+\1ENDEND(a)?+\1?ENDEND(a*)+\1?ENDEND?ENDDropping a non-participating group instead of keeping its slot would be wrong in a different way: every later group shifts down, so a
\2in a child state would silently resolve to group 3's text.Next's rule loop is unchanged — an unmatchable rule simply does not match, and the following rules are tried.BackrefRegexkeeps its exported signature and behaviour; it delegates to an internal helper with every group marked present, so generated lexers are unaffected.Action.applyActionnow takes[]capture, which is not a breaking change: it is an unexported method, so no type outside this package can implementAction.The backref cache key now identifies the pattern together with the captures it was expanded against, including their participation, since an absent group and an empty one no longer compile to the same expression.
Tests
Seven cases added to
TestStatefulLexer; all seven fail onmaster— five with the panic, two with the invalid-regexp error:NonParticipatingGroup— the reported crash.BackrefToNonParticipatingGroup/BackrefToEmptyParticipatingGroup— the same grammar with(a)?and(a*), showing the two are lexed differently.OptionalBackrefToNonParticipatingGroup/OptionalBackrefToEmptyParticipatingGroup— the same pair through\1?.BackrefCachedPerParticipation— both branches in one input, so they cannot share a cache entry.BackrefAfterNonParticipatingGroup— a\2in a child state after a non-participating group 1, pinning group numbering.Validation
go test ./...andcd _examples && go test ./...pass;gofmtclean;go vetandgolangci-lintshow no new findings.BenchmarkStatefulBackrefsgoes 2208 → 1608 allocs/op, since the cache key is now built once into a pre-sized builder;BenchmarkStatefulis allocation-identical.Happy to split the empty-capture wrapping (the fourth table row above, a pre-existing issue) into its own commit if you would rather review it separately.