regexp highlighter depends on the RE_MATCH_PCRE option

[danielshahaf]

As pointed out in #747 (comment) (by @jnooree), the regexp highlighter runs under whichever setting of the RE_MATCH_PCRE option was in effect when _zsh_highlighter was invoked.

This gives rise to two converse issues:

1. If somebody, for whatever reason, calls _zsh_highlight with the RE_MATCH_PCRE option temporarily set to the opposite of whatever it is normally set to when z-sy-h is invoked (e.g., some_other_plugin() { setopt localoptions norematchpcre; _zsh_highlight } in a setup where that option is set when z-sy-h is invoked the usual way), the regexp highlighter will parse its patterns according to the new value of the option. This may cause patterns to fail to compile (if they use PCRE features that aren't supported by the local regcomp(3), or vice versa) or to match incorrectly. In any case, depending on the option's current setting doesn't sound right: whether a pattern is an ERE pattern or a PCRE pattern doesn't depend on the option settings at the time of any particular function call.

2. Running the driver under canonicalized option settings (Canonicalize option settings earlier #536) would constitute a breaking change for users who deliberately set that option and use PCRE patterns in ZSH_HIGHLIGHT_REGEXP.

One straightforward way to fix both issues is to extend the regexp highlighter's API with a way to specify whether matching should use the -regex-match operator or the -pcre-match operator. Should such a setting be per pattern? That would provide modularity: the highlighter could be used by different modules and those modules won't all need to agree on whether to use PCRE or ERE.

An alternative way to fix (2) only would be to query ${zsyh_user_options[rematchpcre]}.

@disarmer @jnooree WDYT?

[jnooree]

Adding an environment variable could be an option (is this @danielshahaf's 'query'?). This also gives flexibility, for those who may not want to set the rematchpcre option globally, but want to use a PCRE pattern for the highlighter. ZSH_HIGHLIGHT_REGEXP would then have to check it, and determine its behavior with respect to the variable.

If only one of the operators is allowed (for any reason), it should be the -regex-match not the -pcre-match. zsh/pcre module would be less stable than the regcomp(3) module, in some environments, especially when (non-ASCII) unicode characters take place. I also found a bug related to this topic (which will be sent to zsh-workers@zsh.org soon).

[danielshahaf]

Adding an environment variable could be an option (is this @danielshahaf's 'query'?). This also gives flexibility, for those who may not want to set the rematchpcre option globally, but want to use a PCRE pattern for the highlighter. ZSH_HIGHLIGHT_REGEXP would then have to check it, and determine its behavior with respect to the variable.

Adding a global variable is indeed an option (it doesn't need to be an environment variable; there's no reason to export it). However, as I mentioned, I suspect the setting should be per pattern rather than per process. Should it?

If only one of the operators is allowed (for any reason), it should be the -regex-match not the -pcre-match. zsh/pcre module would be less stable than the regcomp(3) module, in some environments, especially when (non-ASCII) unicode characters take place. I also found a bug related to this topic (which will be sent to zsh-workers@zsh.org soon).

That's workers/46242. The thread is ongoing.

As to choosing between -regex-match and -pcre-match, let's delay this decision until we need it. Right now, I suspect it won't be needed.

[disarmer]

Well, I personally think regexp highlighter should use PCRE syntax by default, it's much more convinient and widely used. AFAIK any ERE expression can be implemented in PCRE, but not sure if works vice-versa.
It's may warn and do nothing when PCRE module unavailable. But ERE may be used when user explicitly want this, for example with flag $ZSH_HIGHLIGHT_REGEXP_USE_ERE if anyone interested in it.
IMHO per-pattern PCRE/ERE flag will be too complex

Also, regexp highlighter is depends on BASH_REMATCH option, because it's using $MATCH, but man zshall says:

If BASH_REMATCH is set, then the array BASH_REMATCH will be set instead of MATCH and match.

[danielshahaf]

Hi @disarmer!

Well, I personally think regexp highlighter should use PCRE syntax by default,

Changing the default behaviour would be a backwards incompatibility. We shouldn't make incompatible changes without a good reason.

AFAIK any ERE expression can be implemented in PCRE, but not sure if works vice-versa.

Note that we have to consider not only ERE as specified by POSIX, but also whatever unportable extensions vendors have added to their regcomp() functions. So, in the general case, we can't assume that either ERE(+extensions) or PCRE is a subset of the other.

IMHO per-pattern PCRE/ERE flag will be too complex

Could you elaborate on this?

Also, regexp highlighter is depends on BASH_REMATCH option, because it's using $MATCH, but man zshall says:

Good catch. Other highlighters are affected too. I guess we should just set that option locally in the driver. Could you open a separate issue, please? (Of course a PR would be great if you have time, but an issue is all I'm asking for. Thanks!)

[disarmer]

So, in the general case, we can't assume that either ERE(+extensions) or PCRE is a subset of the other.

We can't guess type of regexes, currently it's user responsibility. So i can see several ways:

1. leave as is
2. add way for user to define type (standard/PCRE) for all regexes (i.e. flag $ZSH_HIGHLIGHT_REGEXP_USE_ERE or something) and use some type by default.
3. add way for user to define type per-regexp. See below
4. force one type of regex (seems PCRE will be best choice, because other types are platform-dependent). Break backward compability, but gain regexp/config portability

Item 2 and 3 still require user intervention, because we can't guess current regexp types, so it will break compability. And it still will be user responsibility to set right type.

IMHO per-pattern PCRE/ERE flag will be too complex

Could you elaborate on this?

Current user interface is just associative aray with regexp/color as key/value:

zsh-syntax-highlighting/highlighters/regexp/regexp-highlighter.zsh

Line 32 in fb929ed

typeset -gA ZSH_HIGHLIGHT_REGEXP

There are no way to add third element in associative array. There are only two options: prefixes:
ZSH_HIGHLIGHT_REGEXP+=('PCRE:\bsudo\b' fg=123,bold)
or another associative array with types:

ZSH_HIGHLIGHT_REGEXP+=('\bsudo\b' fg=123,bold)
ZSH_HIGHLIGHT_REGEXP_TYPES+=('\bsudo\b' PCRE)

, but they both are complex for user and ugly.

Good catch. Other highlighters are affected too. I guess we should just set that option locally in the driver.

Ah, it already set here: https://github.com/zsh-users/zsh-syntax-highlighting/blob/master/zsh-syntax-highlighting.zsh#L110
I had outdated version, current version works fine, sorry

[danielshahaf]

(Aside: I edited your comment to add an empty line after every > quote line, including nested quotes. Without an empty line, the two lines > foo bar render identically to the single line > foo bar.)

[danielshahaf]

We can't guess type of regexes, currently it's user responsibility. So i can see several ways:

Agreed.

Item 2 and 3 still require user intervention, because we can't guess current regexp types, so it will break compability.

Did you mean 2 and 4? Option (3) doesn't have this problem: all we have to do is continue to accept patterns that don't indicate their type and evaluate them using =~ (or ${zsyh_user_options[rematchpcre]}), exactly as we do today.

And it still will be user responsibility to set right type.

Sure. I don't think this is a problem. PEP20: "In the face of ambiguity, refuse the temptation to guess." After all, if we don't ask the user to specify the flavour, they might still run into errors if they use the wrong flavour. Better to have the time specified explicitly (which is another PEP20 rule of thumb).

(In retrospect, I'm not sure adding a shell option to change the meaning of the =~ operator was such a good design decision, but that's water under the bridge.)

There are no way to add third element in associative array. There are only two options:

There is any number of alternatives here that you haven't considered; for example:

• ZSH_HIGHLIGHT_REGEXP+=('PCRE^\bsudo\b' fg=123,bold)
• ZSH_HIGHLIGHT_REGEXP+=($'PCRE\0''\bsudo\b' fg=123,bold)
• ZSH_HIGHLIGHT_REGEXP+=('\bsudo\b' PCRE:fg=123,bold)
• ZSH_HIGHLIGHT_REGEXP+=('\bsudo\b' PCRE$'\0'fg=123,bold)
• ZSH_HIGHLIGHT_REGEXP+=('\bsudo\b' fg=123,bold,__zsh_syntax_highlighting_backend=pcre)
• have two separate variables: ZSH_HIGHLIGHT_REGEXP_ERE and ZSH_HIGHLIGHT_REGEXP_PCRE
• Have a plain (non-associative) array with elements of the form '\bsudo\b'$'\0'"pcre"$'\0'"fg=123,bold". (And yes, that's unreadable, but the NUL can be put in a variable, and then '\bsudo\b'${NUL}'pcre'${NUL}'fg=123,bold' is, well, not quite as bad, at least if viewed in an editor with syntax highlighting enabled.)

There are only two options: prefixes:
ZSH_HIGHLIGHT_REGEXP+=('PCRE:\bsudo\b' fg=123,bold)
or another associative array with types:

ZSH_HIGHLIGHT_REGEXP+=('\bsudo\b' fg=123,bold)
ZSH_HIGHLIGHT_REGEXP_TYPES+=('\bsudo\b' PCRE)

, but they both are complex for user and ugly.

Agreed that two associative arrays with the same keys is not an elegant solution.

However, I disagree that PCRE:\bsudo\b is "complex" or "ugly". It's self-documenting, obvious, explicit, human- and machine-readable. It mirrors the syntaxes of many URI schemes, of remote files in scp, of next-hop transport and table lookups in Postfix main.cf files, of XML namespaces, of constructor invocation in many object-oriented languages…

In fact, I'm having trouble thinking what downsides this syntax has, other than the fact that spelling mistakes in the prefix will not be caught at compile time but at runtime. Coupling the type and the pattern into a single atomic value seems like a Good Thing.

Examples of Postfix's syntax

See, for example, https://wiki.dovecot.org/HowTo/PostfixDovecotLMTP#Postfix_main.cf_Configuration. In my configuration I actually have a slightly more pathological variant of that: mailbox_transport_maps = static:lmtp:unix:private/dovecot-lmtp. That uses three instances of this pattern simultaneously: static is a table lookup with argument lmtp:unix:private/dovecot-lmtp; lmtp is a transport with argument unix:private/dovecot-lmtp; and so on.

Ah, it already set here: https://github.com/zsh-users/zsh-syntax-highlighting/blob/master/zsh-syntax-highlighting.zsh#L110

I know this link was valid when you posted it, but it already points to a different line. The line you referred to is now

zsh-syntax-highlighting/zsh-syntax-highlighting.zsh

Line 168 in 810c2dc

setopt localoptions warncreateglobal nobashrematch

. (See workers/37966.)

I had outdated version, current version works fine, sorry

No worries.

[disarmer]

There is any number of alternatives here that you haven't considered; for example:

Hm, some of them are very similar, prefix in value is almost the same. But what if we implement prefixes? Unprefixed values are still depends on environment, and same config works differently in different envrironments.

have two separate variables: ZSH_HIGHLIGHT_REGEXP_ERE and ZSH_HIGHLIGHT_REGEXP_PCRE

I like this idea, but it breaks compability too...

[danielshahaf]

Unprefixed values are still depends on environment, and same config works differently in different envrironments.

This is how unprefixed values behave today, and this behaviour should be retain (if only as legacy/deprecated) for backwards compatibility.

it breaks compability too...

No, it doesn't. It's backwards compatible, since the two new variables would be in addition to the existing (legacy) one, not instead of it.