| Message ID | 20230108155217.2817-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] grep: correctly identify utf-8 characters with \{b,w} in -P | expand |
On Sun, Jan 08 2023, Carlo Marcelo Arenas Belón wrote: > When UTF is enabled for a PCRE match, the corresponding flags are > added to the pcre2_compile() call, but PCRE2_UCP wasn't included. > > This prevents extending the meaning of the character classes to > include those new valid characters and therefore result in failed > matches for expressions that rely on that extention, for ex: > > $ git grep -P '\bÆvar' > > Add PCRE2_UCP so that \w will include Æ and therefore \b could > correctly match the beginning of that word. > > This has an impact on performance that has been estimated to be > between 20% to 40% and that is shown through the added performance > test. > > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > grep.c | 2 +- > t/perf/p7822-grep-perl-character.sh | 42 +++++++++++++++++++++++++++++ > 2 files changed, 43 insertions(+), 1 deletion(-) > create mode 100755 t/perf/p7822-grep-perl-character.sh > > diff --git a/grep.c b/grep.c > index 06eed69493..1687f65b64 100644 > --- a/grep.c > +++ b/grep.c > @@ -293,7 +293,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt > options |= PCRE2_CASELESS; > } > if (!opt->ignore_locale && is_utf8_locale() && !literal) > - options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF); > + options |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF); I have a definite bias towards liking this change, it would help my find myself :) But I don't think it's safe to change the default behavior "git-grep", it's not a mere bug fix, but a major behavior change for existing users of grep.patternType=perl. E.g. on git.git: $ diff <(git -P grep -P '\d+') <(git -P grep -P '(*UCP)\d') 53360a53361,53362 > git-gui/po/ja.po:"- 第１行: 何をしたか、を１行で要約。\n" > git-gui/po/ja.po:"- 第２行: 空白\n" So, it will help "do the right thing" on e.g. "\bÆ", but it will also find e.g. CJK numeric characters for \d etc. I see per the discussion on https://github.com/PCRE2Project/pcre2/issues/185 and https://lists.gnu.org/archive/html/bug-grep/2023-01/threads.html that you submitted similar fixes to GNU grep & PCRE itself. I see that GNU grep integrated it a couple of days ago as https://git.savannah.gnu.org/cgit/grep.git/commit/?id=5e3b760f65f13856e5717e5b9d935f5b4a615be3 As most discussions about PCRE will eventually devolve into "what does Perl do?": "Perl" itself will promiscuously use this behavior by default. E.g. here the same "１" character (not the ASCII digit "1") will be matched from the command-line: $ perl -Mre=debug -CA -wE 'shift =~ /\d/' "１" Compiling REx "\d" Final program: 1: POSIXU[\d] (2) 2: END (0) stclass POSIXU[\d] minlen 1 Matching REx "\d" against "%x{ff11}" UTF-8 string... Matching stclass POSIXU[\d] against "%x{ff11}" (3 bytes) 0 <> <%x{ff11}> | 0| 1:POSIXU[\d](2) 3 <%x{ff11}> <> | 0| 2:END(0) Match successful! Freeing REx: "\d" But I don't think it makes sense for "git grep" (or GNU "grep") to follow Perl in this particular case. For those not familiar with its Unicode model it doesn't assume by default that strings are Unicode, they have to be explicitly marked as such. in the above example I'm declaring that all of "argv" is UTF-8 (via the "-CA" flag). If I didn't supply that flag the string wouldn't have the UTF-8 flag, and wouldn't match, as the Perl regex engine won't use Unicode semantics except on Unicode target strings. Even for Perl, this behavior has been troublesome. Opinions differ, but I think many would agree (and I've CC'd the main authority on Perl's regex engine) that doing this by default was *probably* a mistake. You almost never want "everything Unicode considers a digit", and if you do using e.g. \p{Nd} instead of \d would be better in terms of expressing your intent. I see you're running into this on the PCRE tracker, where you're suggesting that the equivalent of /a (or /aa) would be needed. https://github.com/PCRE2Project/pcre2/issues/185#issuecomment-1374796393 Which brings me home to the seeming digression about "Perl" above. Unlike a programming language where you'll typically "mark" your data as it comes in, natural text as UTF-8, binary data as such etc., a "grep" utility has to operate on more of an "all or nothing" basis (except in the case of "-a"). I.e. we're usually searching through unknown data. Enabling this by default means that we'll pick up characters most people probably wouldn't expect, particularly from near-binary data formats (those that won't require "-a", but contain non-Unicode non-ASCII sequences). I don't have some completely holistic view of what we should do in every case, e.g. we turned on PCRE2_UTF so that things like "-i" would Just Work, but even case-insensitivity has its own unexpected edge cases in Unicode. But I don't think those edge cases are nearly as common as those we'd run into by enabling PCRE2_UCP. Rather than trying to opt-out with "/a" or "/aa" I think this should be opt-in. As the example at the start shows you can already do this with "(*UCP)" in the pattern, so perhaps we should just link to the pcre2pattern(3) manual from git-grep(1)?
On 1/9/23 03:35, Ævar Arnfjörð Bjarmason wrote: > You almost never want "everything Unicode considers a digit", and if you > do using e.g. \p{Nd} instead of \d would be better in terms of > expressing your intent. For GNU grep, PCRE2_UCP is needed because of examples like what Gro-Tsen and Karl Petterssen supplied. If there's some diagreement about how \d should behave with UTF-8 data the GNU grep hackers should let the Perl community decide that; that is, GNU grep can simply follow PCRE2's lead. But GNU grep does need PCRE2_UCP for \b etc. > $ diff <(git -P grep -P '\d+') <(git -P grep -P '(*UCP)\d') > 53360a53361,53362 > > git-gui/po/ja.po:"- 第１行: 何をしたか、を１行で要約。\n" > > git-gui/po/ja.po:"- 第２行: 空白\n" Although I don't speak Japanese I have dealt with quite a bit of Japanese text in a previous job, and personally I would prefer \d to match those two lines as they do contain digits. So to me this particular case is not a good argument that git grep should not match those lines. Of course other people might prefer differently, and there are cases where I want to match only ASCII digits. I've learned in the past to use [0-9] for that. I hope PCRE2 never changes [0-9] to match anything but ASCII digits when searching UTF-8 text.
On Mon, Jan 09 2023, Paul Eggert wrote: > On 1/9/23 03:35, Ævar Arnfjörð Bjarmason wrote: > >> You almost never want "everything Unicode considers a digit", and if you >> do using e.g. \p{Nd} instead of \d would be better in terms of >> expressing your intent. > > For GNU grep, PCRE2_UCP is needed because of examples like what > Gro-Tsen and Karl Petterssen supplied. [For reference, referring to this Twitter thread: https://twitter.com/gro_tsen/status/1610972356972875777] Those examples compared -E and -P. I think it's correct that UCP brings the behavior closer to -E, but it's also different in various ways. E.g. on emacs.git (which I've been finding to be quite a nice test case) a comparison of the two, with "git grep" because I found it easier to test, but GNU grep will presumably find the same for those files: for c in b s w do for pfx in '' '(*UCP)' do echo "$pfx/$c:" && diff -u <(git -P grep -E "\\$c") <(git -P grep -P "$pfx\\$c") | wc -l done done Yields: /b: 155781 (*UCP)/b: 46035 /s: 0 (*UCP)/s: 0 /w: 142468 (*UCP)/w: 9706 So the output still differs, and some of those differences may or may not be wanted. > If there's some diagreement > about how \d should behave with UTF-8 data the GNU grep hackers should > let the Perl community decide that; that is, GNU grep can simply > follow PCRE2's lead. PCRE2 tends to follow Perl, I'm mainly trying to point out here that it isn't a-priory clear how "let Perl decide" is supposed to map to the of a "grep"-like utility, since the Perl behavior is inherently tied up with knowing the encoding of the target data. For GNU grep and "git grep" that's more of an all-or-nothing with locales, although in this case being as close as possible to -E is probably more correct than not. >> $ diff <(git -P grep -P '\d+') <(git -P grep -P '(*UCP)\d') >> 53360a53361,53362 >> > git-gui/po/ja.po:"- 第１行: 何をしたか、を１行で要約。\n" >> > git-gui/po/ja.po:"- 第２行: 空白\n" > > Although I don't speak Japanese I have dealt with quite a bit of > Japanese text in a previous job, and personally I would prefer \d to > match those two lines as they do contain digits. So to me this > particular case is not a good argument that git grep should not match > those lines. I'm mainly raising the backwards compatibility concern, which GNU grep and git grep may or may not want to handle differently, but let's at least be aware of the various edge cases. For \b I think it mostly does the right thing. For \w and \d in particular I'm mainly noting that yes, sometimes you want to match [0-9], and sometimes you'd want to match Japanese numbers, but you rarely (or at least I haven't) want to match everything Unicode considers X, unless you're doing some self-reflection on Unicode itself. E.g. for \d it's at least (up from just 10): $ perl -CO -wE 'for (1..2**20) { say chr if chr =~ /\d/ }'|wc -l 650 For \w you similarly go from ~60 to ~130k: $ perl -CO -wE 'for (1..2**24) { say chr if chr =~ /\w/ }'|wc -l 134564 If all you're doing is matching either ASCII or Japanese text and you want "locale-aware numbers" it might do the wrong thing. But I've found it to be too promiscuous when casting a wider net, which is the usual use-case with 'grep". > Of course other people might prefer differently, and there are cases > where I want to match only ASCII digits. I've learned in the past to > use [0-9] for that. I hope PCRE2 never changes [0-9] to match anything > but ASCII digits when searching UTF-8 text. I think that'll never change.
On 1/9/23 11:51, Ævar Arnfjörð Bjarmason wrote: > /b: > 155781 > (*UCP)/b: > 46035 > /s: > 0 > (*UCP)/s: > 0 > /w: > 142468 > (*UCP)/w: > 9706 > > So the output still differs, and some of those differences may or may > not be wanted. I took a look at the output, and by and large I'd want the differences; that is, I'd want the UCP version, which generates less output. This is because several Emacs source files are not UTF-8, and \b has nonsense matches when searching text files encoded via Shift-JIS or Big 5 or whatever. For this sort of thing, the fewer matches the better. > If all you're doing is matching either ASCII or Japanese text and you > want "locale-aware numbers" it might do the wrong thing. I'm not seeing much of a problem here. When searching Japanese text, I would expect \d and [0-9０-９] (using both ASCII and full-width digits) to be equivalent so (assuming UCP) it's not a big deal as to which regex you use, since Japanese text won't contain Bengali (or whatever) digits. And when searching binary data, I'd expect a bunch of garbage no matter how \d is interpreted. Here I'm assuming [０-９] (using full-width digits) has the expected meaning in PCRE2, i.e., that PCRE2 didn't make the same mistake that POSIX made.
On Mon, Jan 9, 2023 at 4:17 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >Rather than trying to opt-out with "/a" or "/aa" I think this should be opt-in. > > As the example at the start shows you can already do this with "(*UCP)" > in the pattern, so perhaps we should just link to the pcre2pattern(3) > manual from git-grep(1)? Considering that PCRE is used internally even for cases that don't specify -P how would that opt-in work? For example, in a repository with code that uses utf identifiers, the following will fail: $ git grep -w -E motion u.c: int émotion = 0; $ git grep -w -E '(*UCP)motion' fatal: command line, '(*UCP)motion': Invalid preceding regular expression $ git -P grep -P -w '(*UCP)motion' u.c: int émotion = 0; Carlo CC removed gnu and the obsoleted PCRE developer list (if really needed would be better to use the documented pcre2-dev@googlegroups.com, instead)
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > But I don't think it's safe to change the default behavior "git-grep", > it's not a mere bug fix, but a major behavior change for existing users > of grep.patternType=perl. > ... > Even for Perl, this behavior has been troublesome. Opinions differ, but > I think many would agree (and I've CC'd the main authority on Perl's > regex engine) that doing this by default was *probably* a mistake. > ... > As the example at the start shows you can already do this with "(*UCP)" > in the pattern, so perhaps we should just link to the pcre2pattern(3) > manual from git-grep(1)? So, now do we have a final verdict on this patch? If we are not taking the "unconditonally enable ucp" patch (which I tend to agree with a safer choice for now), it may make sense to mention (*UCP) in our documentation somewhere, perhaps?
On Mon, Apr 3, 2023 at 2:39 PM Paul Eggert <eggert@cs.ucla.edu> wrote: > I've recently done some bug-report maintenance about a set of GNU grep > bug reports related to whether whether "grep -P '\d'" should match > non-ASCII digits, and have some thoughts about coordinating GNU grep > with git grep in this department. > > GNU Bug#62605[1] "`[\d]` does not work with PCRE" has been fixed on > Savannah's copy of GNU grep, and some sort of fix should appear in the > next grep release. However, I'm leaving the GNU grep bug report open for > now because it's related to Bug#60690[2] "[PATCH v2] grep: correctly > identify utf-8 characters with \{b,w} in -P" and to Bug#62552[3] "Bug > found in latest stable release v3.10 of grep". I merged these related > bug reports, and the oldest one, Bug#60690, is now the representative > displayed in the GNU grep bug list[4]. > > For this set of grep bug reports there's still a pending issue discussed > in my recent email[5], which proposes a patch so I've tagged Bug#60690 > with "patch". The proposal is that GNU grep -P '\d' should revert to the > grep 3.9 behavior, i.e., that in a UTF-8 locale, \d should also match > non-ASCII decimal digits. > > In researching this a bit further, I found that on March 23 Git disabled > the use of PCRE2_UCP in PCRE2 10.34 or earlier[6], due to a PCRE2 bug > that can cause a crash when PCRE2_UCP is used[7]. A bug fix[8] should > appear in the next PCRE2 release. > > When PCRE2 10.35 comes out, Thanks for finding that. It's clearly a good idea to disable PCRE2_UCP for those using those older, known-buggy versions of pcre2. The latest is 10.42, per https://github.com/PCRE2Project/pcre2/releases > it appears that 'git grep -P' will behave > like 'grep -P' only if GNU grep adopts something like the solution > proposed in [5]. > > [1]: https://bugs.gnu.org/62605 > [2]: https://bugs.gnu.org/60690 > [3]: https://bugs.gnu.org/62552 > [4]: https://debbugs.gnu.org/cgi/pkgreport.cgi?package=grep > [5]: https://lists.gnu.org/archive/html/grep-devel/2023-04/msg00004.html > [6]: > https://github.com/git/git/commit/14b9a044798ebb3858a1f1a1377309a3d6054ac8 > [7]: > https://lore.kernel.org/git/7E83DAA1-F9A9-4151-8D07-D80EA6D59EEA@clumio.com/ > [8]: > https://github.com/git/git/commit/14b9a044798ebb3858a1f1a1377309a3d6054ac8 Thanks for all of the links. However, have you seen justification (other than for compatibility with some other tool or language) for allowing \d to match non-ASCII by default, in spite of the risks? IMHO, we have an obligation to retain compatibility with how grep -P '\d' has worked since -P was added. I'd be happy to see an option to enable the match-multibyte-digits behavior, but making it the default seems too likely to introduce unwarranted risk.
On 2023-04-03 20:30, Jim Meyering wrote: > have you seen justification > (other than for compatibility with some other tool or language) for > allowing \d to match non-ASCII by default, in spite of the risks? In the example Ævar supplied in <https://bugs.gnu.org/60690>, my impression was that it was better when \d matched non-ASCII digits. That is, in a UTF-8 locale it's better when \d finds matches in these lines: >> > git-gui/po/ja.po:"- 第１行: 何をしたか、を１行で要約。\n" >> > git-gui/po/ja.po:"- 第２行: 空白\n" because they contain the Japanese digits "１" and "２". This was the only example I recall being given. Also, I find it odd that grep -P '^[\w\d]*$' matches lines containing any sort of Arabic word characters, but it rejects lines containing Arabic digits like "٣" that are perfectly reasonable in Arabic-language text. I also find it odd that [\d] and [[:digit:]] mean different things. There are arguments on the other side, otherwise we wouldn't be having this discussion. And it's true that grep -P '\d' formerly rejected Arabic digits (though it's also true that grep -P '\w' formerly rejected Arabic letters...). Still, the cure's oddness and incompatibility with Git, Perl, etc. appears to me to be worse than the disease of dealing with grep -P invocations that need to use [0-9] or LC_ALL="C" anyway if they want to be portable to any program other than GNU grep.
On Mon, Apr 3, 2023 at 11:47 PM Paul Eggert <eggert@cs.ucla.edu> wrote: > On 2023-04-03 20:30, Jim Meyering wrote: > > have you seen justification > > (other than for compatibility with some other tool or language) for > > allowing \d to match non-ASCII by default, in spite of the risks? > > In the example Ævar supplied in <https://bugs.gnu.org/60690>, my > impression was that it was better when \d matched non-ASCII digits. That > is, in a UTF-8 locale it's better when \d finds matches in these lines: > > >> > git-gui/po/ja.po:"- 第１行: 何をしたか、を１行で要約。\n" > >> > git-gui/po/ja.po:"- 第２行: 空白\n" > > because they contain the Japanese digits "１" and "２". This was the only > example I recall being given. Before it was unintentionally enabled in grep-3.9, lines like that have never been matched by grep -P's '\d'. By relaxing \d, we'd weaken any application that uses say grep -P '^\d+$' to perform input validation intending to ensure that some input is all ASCII digits. It's not a big stretch to imagine that some downstream processor of that "verified" data is not prepared to deal with multi-byte digits. > Also, I find it odd that grep -P '^[\w\d]*$' matches lines containing > any sort of Arabic word characters, but it rejects lines containing > Arabic digits like "٣" that are perfectly reasonable in Arabic-language > text. I also find it odd that [\d] and [[:digit:]] mean different things. > > There are arguments on the other side, otherwise we wouldn't be having > this discussion. And it's true that grep -P '\d' formerly rejected > Arabic digits (though it's also true that grep -P '\w' formerly rejected > Arabic letters...). Still, the cure's oddness and incompatibility with > Git, Perl, etc. appears to me to be worse than the disease of dealing > with grep -P invocations that need to use [0-9] or LC_ALL="C" anyway if > they want to be portable to any program other than GNU grep. I'm primarily concerned about not introducing a persistent regression in how GNU grep's -P '\d' works in multibyte locales. The corner cases you mention do matter, of course, but are far less likely to matter in practice.
diff --git a/grep.c b/grep.c index 06eed69493..1687f65b64 100644 --- a/grep.c +++ b/grep.c @@ -293,7 +293,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt options |= PCRE2_CASELESS; } if (!opt->ignore_locale && is_utf8_locale() && !literal) - options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF); + options |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF); #ifndef GIT_PCRE2_VERSION_10_36_OR_HIGHER /* Work around https://bugs.exim.org/show_bug.cgi?id=2642 fixed in 10.36 */ diff --git a/t/perf/p7822-grep-perl-character.sh b/t/perf/p7822-grep-perl-character.sh new file mode 100755 index 0000000000..87009c60df --- /dev/null +++ b/t/perf/p7822-grep-perl-character.sh @@ -0,0 +1,42 @@ +#!/bin/sh + +test_description="git-grep's perl regex + +If GIT_PERF_GREP_THREADS is set to a list of threads (e.g. '1 4 8' +etc.) we will test the patterns under those numbers of threads. +" + +. ./perf-lib.sh + +test_perf_large_repo +test_checkout_worktree + +if test -n "$GIT_PERF_GREP_THREADS" +then + test_set_prereq PERF_GREP_ENGINES_THREADS +fi + +for pattern in \ + '\\bhow' \ + '\\bÆvar' \ + '\\d+ \\bÆvar' \ + '\\bBelón\\b' \ + '\\w{12}\\b' +do + echo '$pattern' >pat + if ! test_have_prereq PERF_GREP_ENGINES_THREADS + then + test_perf "grep -P '$pattern'" --prereq PCRE " + git -P grep -f pat || : + " + else + for threads in $GIT_PERF_GREP_THREADS + do + test_perf "grep -P '$pattern' with $threads threads" --prereq PTHREADS,PCRE " + git -c grep.threads=$threads -P grep -f pat || : + " + done + fi +done + +test_done
When UTF is enabled for a PCRE match, the corresponding flags are added to the pcre2_compile() call, but PCRE2_UCP wasn't included. This prevents extending the meaning of the character classes to include those new valid characters and therefore result in failed matches for expressions that rely on that extention, for ex: $ git grep -P '\bÆvar' Add PCRE2_UCP so that \w will include Æ and therefore \b could correctly match the beginning of that word. This has an impact on performance that has been estimated to be between 20% to 40% and that is shown through the added performance test. Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- grep.c | 2 +- t/perf/p7822-grep-perl-character.sh | 42 +++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) create mode 100755 t/perf/p7822-grep-perl-character.sh