Skip to content

Fix search for lookbehind#193

Merged
bzz merged 2 commits intogo-enry:masterfrom
DecimalTurn:patch-3
Jan 7, 2026
Merged

Fix search for lookbehind#193
bzz merged 2 commits intogo-enry:masterfrom
DecimalTurn:patch-3

Conversation

@DecimalTurn
Copy link
Copy Markdown
Contributor

@DecimalTurn DecimalTurn commented Feb 24, 2025

The previous search using (?< was matching the syntax (?<name>re) which is not a lookbehind. This new check ((?<= or (?<!) is more reliable.

Sidenote:
The syntax (?<name>re) corresponds to a "named & numbered capturing group (submatch)". Looking at RE2's syntax, this syntax doesn't have the mention "UNSUPPORTED", so one must assume that it is supported. However, we currently assume that the similar syntax (?P<name>re) is unsupported, but it also doesn't have the "UNSUPPORTED" mention 🤔.

I think what makes it confusing is that RE2 supports named capture group to make it easier to extract the value of the capture afterwards, but there can be no backreference to that group within the regex. Hence, a better approach to look for incompatible syntax would be to look for a backreference to a named capture group such as \k<name> or \g<name> instead of looking for (?P<name>re) like we do at the moment.

@lafriks lafriks added the bug Something isn't working label Apr 6, 2025
@bzz
Copy link
Copy Markdown
Member

bzz commented May 28, 2025

I'm curious how hard would that be to add a test that would demonstrate this behavior? (not necessary as prat of this RP)
I don't remember peculiarities of RE2 by now, but I always found that having few examples of patterns help refreshing memory..

However, we currently assume that the similar syntax (?Pre) is unsupported, but it also doesn't have the "UNSUPPORTED" mention 🤔.

indeed, it would be nice to double-check (e.g. in playground) if named capture groups are supported!
There must have been a reason we added them in the first place... but it's been a while).
Should be fine to remove the check, probably in a different PR, if that doesn't break anything 🤞

Comment thread internal/code-generator/generator/heuristics.go
Copy link
Copy Markdown
Member

@bzz bzz left a comment

Choose a reason for hiding this comment

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

LGTM, but could we possibly also update the doc to only mention lookbehind? 🙏

@bzz bzz mentioned this pull request May 28, 2025
Co-authored-by: Alex <bzz@users.noreply.github.com>
@DecimalTurn
Copy link
Copy Markdown
Contributor Author

DecimalTurn commented May 28, 2025

LGTM, but could we possibly also update the doc to only mention lookbehind? 🙏

Which part of the documentation are you thinking of?

If by that you meant the comment you suggested, then yes, I've added it in 8abe5b1 (#193)

@bzz bzz merged commit 6e7b690 into go-enry:master Jan 7, 2026
14 checks passed
@DecimalTurn DecimalTurn deleted the patch-3 branch January 7, 2026 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Development

Successfully merging this pull request may close these issues.

3 participants