Re: [PATCH v2] grep: correctly identify utf-8 characters with \{b,w} in -P

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jan 9, 2023, 11:35 UTC
Message-ID: <230109.86v8lf297g.gmgdl@evledraar.gmail.com>
In-Reply-To: <20230108155217.2817-1-carenas@gmail.com>

On Sun, Jan 08 2023, Carlo Marcelo Arenas Belón wrote:

Show 33 quoted lines

> 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)?