Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid emptying the whole of region_highlight #418

Closed
okapia opened this issue Feb 23, 2017 · 16 comments
Closed

Avoid emptying the whole of region_highlight #418

okapia opened this issue Feb 23, 2017 · 16 comments

Comments

@okapia
Copy link
Contributor

okapia commented Feb 23, 2017

By emptying region_highlight, z-sy-h can't be used so easily together with other plugins that use highlighting. Ideally, it would only remove it's own entries.

See, e.g okapia/zsh-viexchange#1

As discussed over e-mail a potential solution could take advantage of zsh ignoring unknown keys. e.g

region_highlight+=( '1 3 fg=red,owner=z-sy-h' )

@danielshahaf
Copy link
Member

danielshahaf commented Mar 5, 2017

That idea wouldn't work: region_highlight+=( '0 1 fg=green,owner=z-sy-h' ) causes $region_highlight to contain 0 1 fg=green; the owner is dropped.

The region_highlight serializer (get_region_highlight()) is entirely injection-free; there's no way to have it echo a user-specified string.

@danielshahaf
Copy link
Member

Issue sweep: any suggestions on how to approach this? Brainstorming:

  • Patch zsh to support the owner key.
  • Separation of concerns: have a mode in which _zsh_highlight only returns the list of entries to be appended to $region_highlight, and <handwave>something</handwave> is responsible for clearing it between _zsh_highlight invocations. (TBD whether that something would be a plugin, or a zle feature, or …)
  • (Something else?)

@okapia
Copy link
Contributor Author

okapia commented Apr 9, 2020

I like the idea of the owner key. And actually ignoring unknown keys makes some sense.

@danielshahaf
Copy link
Member

Ack. Don't have brainwidth to drive that currently, though.

danielshahaf added a commit to danielshahaf/zsh that referenced this issue May 3, 2020
This is to allow plugins to easily remove only entries they had added.
See zsh-users/zsh-syntax-highlighting#418 and
the cross-referenced issues.
danielshahaf added a commit to danielshahaf/zsh that referenced this issue May 3, 2020
This is to allow plugins to easily remove only entries they had added.
See zsh-users/zsh-syntax-highlighting#418 and
the cross-referenced issues.
@danielshahaf
Copy link
Member

danielshahaf commented May 3, 2020

Right, here it is: zsh-users/zsh#57

And the corresponding z-sy-h patch:

(EDIT: See next comment for a replacement z-sy-h patch. The original thing I posted here is visible in the comment edit history if it's ever needed.)

I've checked that these two, together, fix okapia/zsh-viexchange#1. Could you confirm, please?

@danielshahaf
Copy link
Member

danielshahaf commented May 4, 2020

Revised z-sy-h patch (cf. zsh-users/zsh#57 (review)):

Edit 2020-06-18: s/owner/memo/g

Revised z-sy-h patch
diff --git a/highlighters/pattern/pattern-highlighter.zsh b/highlighters/pattern/pattern-highlighter.zsh
index 054eff7..ce926f7 100644
--- a/highlighters/pattern/pattern-highlighter.zsh
+++ b/highlighters/pattern/pattern-highlighter.zsh
@@ -54,7 +54,7 @@ _zsh_highlight_pattern_highlighter_loop()
   local -a match mbegin mend
   local MATCH; integer MBEGIN MEND
   if [[ "$buf" == (#b)(*)(${~pat})* ]]; then
-    region_highlight+=("$((mbegin[2] - 1)) $mend[2] $ZSH_HIGHLIGHT_PATTERNS[$pat]")
+    region_highlight+=("$((mbegin[2] - 1)) $mend[2] $ZSH_HIGHLIGHT_PATTERNS[$pat], memo=zsh-syntax-highlighting")
     "$0" "$match[1]" "$pat"; return $?
   fi
 }
diff --git a/highlighters/regexp/regexp-highlighter.zsh b/highlighters/regexp/regexp-highlighter.zsh
index 26f9da3..c5806d0 100644
--- a/highlighters/regexp/regexp-highlighter.zsh
+++ b/highlighters/regexp/regexp-highlighter.zsh
@@ -55,7 +55,7 @@ _zsh_highlight_regexp_highlighter_loop()
   local -a match mbegin mend
   while true; do
     [[ "$buf" =~ "$pat" ]] || return;
-    region_highlight+=("$((MBEGIN - 1 + OFFSET)) $((MEND + OFFSET)) $ZSH_HIGHLIGHT_REGEXP[$pat]")
+    region_highlight+=("$((MBEGIN - 1 + OFFSET)) $((MEND + OFFSET)) $ZSH_HIGHLIGHT_REGEXP[$pat], memo=zsh-syntax-highlighting")
     buf="$buf[$(($MEND+1)),-1]"
     OFFSET=$((MEND+OFFSET));
   done
diff --git a/tests/test-highlighting.zsh b/tests/test-highlighting.zsh
index d0698d6..1ce6178 100755
--- a/tests/test-highlighting.zsh
+++ b/tests/test-highlighting.zsh
@@ -190,7 +190,7 @@ run_test_internal() {
     if
       [[ $start != $exp_start ]] ||
       [[ $end != $exp_end ]] ||
-      [[ $highlight_zone[3] != $expected_highlight_zone[3] ]]
+      [[ ${highlight_zone[3]%,} != ${expected_highlight_zone[3]%,} ]]
     then
       print -r -- "not ok $i - $desc - expected ($exp_start $exp_end ${(qqq)expected_highlight_zone[3]}), observed ($start $end ${(qqq)highlight_zone[3]}). $todo"
     else
diff --git a/zsh-syntax-highlighting.zsh b/zsh-syntax-highlighting.zsh
index bb85d33..19bed18 100644
--- a/zsh-syntax-highlighting.zsh
+++ b/zsh-syntax-highlighting.zsh
@@ -122,12 +122,32 @@ _zsh_highlight()
   # Make it read-only.  Can't combine this with the previous line when POSIX_BUILTINS may be set.
   typeset -r ret
 
+  (( ${+region_highlight} )) || typeset -ga region_highlight
+
+  # Probe the memo= feature, once.
+  (( ${+zsh_highlight__memo_feature} )) || {
+    region_highlight+=( "0 0 fg=red, memo=zsh-syntax-highlighting" )
+    if [[ ${region_highlight[-1]} == *memo=zsh-syntax-highlighting* ]]; then
+      integer -gr zsh_highlight__memo_feature=1
+    else
+      integer -gr zsh_highlight__memo_feature=0
+    fi
+    region_highlight[-1]=()
+  }
+
+  # Reset region_highlight to build it from scratch
+  if (( zsh_highlight__memo_feature )); then
+    region_highlight=( "${(@)region_highlight:#*memo=zsh-syntax-highlighting*}" )
+  else
+    # Legacy codepath.  Not very interoperable with other plugins (issue #418).
+    region_highlight=()
+  fi
+
   # Remove all highlighting in isearch, so that only the underlining done by zsh itself remains.
   # For details see FAQ entry 'Why does syntax highlighting not work while searching history?'.
   # This disables highlighting during isearch (for reasons explained in README.md) unless zsh is new enough
   # and doesn't have the pattern matching bug
   if [[ $WIDGET == zle-isearch-update ]] && { $zsh_highlight__pat_static_bug || ! (( $+ISEARCHMATCH_ACTIVE )) }; then
-    region_highlight=()
     return $ret
   fi
 
@@ -163,10 +183,6 @@ _zsh_highlight()
   # Do not highlight if there are pending inputs (copy/paste).
   [[ $PENDING -gt 0 ]] && return $ret
 
-  # Reset region highlight to build it from scratch
-  typeset -ga region_highlight
-  region_highlight=();
-
   {
     local cache_place
     local -a region_highlight_copy
@@ -285,7 +301,8 @@ _zsh_highlight_apply_zle_highlight() {
   else
     start=$second end=$first
   fi
-  region_highlight+=("$start $end $region")
+  # The comma is there to make zsh's that don't support the memo= key silently ignore it.
+  region_highlight+=("$start $end $region, memo=zsh-syntax-highlighting")
 }
 
 
@@ -325,7 +342,7 @@ _zsh_highlight_add_highlight()
   shift 2
   for highlight; do
     if (( $+ZSH_HIGHLIGHT_STYLES[$highlight] )); then
-      region_highlight+=("$start $end $ZSH_HIGHLIGHT_STYLES[$highlight]")
+      region_highlight+=("$start $end $ZSH_HIGHLIGHT_STYLES[$highlight], memo=zsh-syntax-highlighting")
       break
     fi
   done

@danielshahaf
Copy link
Member

Triage: Depends on a zsh patch, so postponing to 0.9.0. If the zsh patch is merged/released upstream before that, we can promote this to a sooner milestone.

@danielshahaf
Copy link
Member

Cross-referencing @phy1729's remark on #735

It's not pretty, but it's possible to add a noop highlight (e.g. 0 0 fg=17,bg=29) into region_highlight to demark the start and end of z-sy-h controlled highlights (assuming other plugins prepend or append to the array and don't insert in the middle). Could be used as a fall back if owner= isn't available.

That trick could fix this issue (#418) for pre-memo= (née owner=) zsh, under the stated assumption.

danielshahaf added a commit to danielshahaf/zsh that referenced this issue Jun 17, 2020
[s/owner=/memo=/g]

Support a 'memo' key in region_highlight.

This is to allow plugins to easily remove only entries they had added.
See zsh-users/zsh-syntax-highlighting#418 and
the cross-referenced issues.

This was initially worked on at zsh-users#57,
where it was initially called "owner=".
@danielshahaf
Copy link
Member

danielshahaf commented Jun 18, 2020

Revised z-sy-h patch (cf. zsh-users/zsh#57 (review)):

Revision 3 of the patch: add HACKING entry, change probing

[edit 2020-06-26: needs to be filtered through recountdiff from patchutils to fix offsets/counts in the first hunk]

diff --git a/HACKING.md b/HACKING.md
index 71d8a2e..8a41c34 100644
--- a/HACKING.md
+++ b/HACKING.md
@@ -67,6 +67,22 @@ expected_region_highlight=(
 )
 
+Memos and commas
+----------------
+
+We append to `region_highlight` as follows:
+
+
+```zsh
+region_highlight+=("$start $end $region, memo=zsh-syntax-highlighting")
+```
+
+That comma is required to cause zsh 5.8 and older to ignore the memo without
+ignoring the $region.  It's a hack, but given that no further 5.8.x patch
+releases are planned, it's been deemed acceptable.  See issue #418 and the
+cross-referenced issues.
+
+
 Miscellany
 ----------
 
diff --git a/highlighters/pattern/pattern-highlighter.zsh b/highlighters/pattern/pattern-highlighter.zsh
index 054eff7..e0422d0 100644
--- a/highlighters/pattern/pattern-highlighter.zsh
+++ b/highlighters/pattern/pattern-highlighter.zsh
@@ -54,7 +54,7 @@ _zsh_highlight_pattern_highlighter_loop()
   local -a match mbegin mend
   local MATCH; integer MBEGIN MEND
   if [[ "$buf" == (#b)(*)(${~pat})* ]]; then
-    region_highlight+=("$((mbegin[2] - 1)) $mend[2] $ZSH_HIGHLIGHT_PATTERNS[$pat]")
+    region_highlight+=("$((mbegin[2] - 1)) $mend[2] $ZSH_HIGHLIGHT_PATTERNS[$pat], memo=zsh-syntax-highlighting")
     "$0" "$match[1]" "$pat"; return $?
   fi
 }
diff --git a/highlighters/regexp/regexp-highlighter.zsh b/highlighters/regexp/regexp-highlighter.zsh
index 26f9da3..0d43aac 100644
--- a/highlighters/regexp/regexp-highlighter.zsh
+++ b/highlighters/regexp/regexp-highlighter.zsh
@@ -55,7 +55,7 @@ _zsh_highlight_regexp_highlighter_loop()
   local -a match mbegin mend
   while true; do
     [[ "$buf" =~ "$pat" ]] || return;
-    region_highlight+=("$((MBEGIN - 1 + OFFSET)) $((MEND + OFFSET)) $ZSH_HIGHLIGHT_REGEXP[$pat]")
+    region_highlight+=("$((MBEGIN - 1 + OFFSET)) $((MEND + OFFSET)) $ZSH_HIGHLIGHT_REGEXP[$pat], memo=zsh-syntax-highlighting")
     buf="$buf[$(($MEND+1)),-1]"
     OFFSET=$((MEND+OFFSET));
   done
diff --git a/tests/test-highlighting.zsh b/tests/test-highlighting.zsh
index 0e8e03e..d282f81 100755
--- a/tests/test-highlighting.zsh
+++ b/tests/test-highlighting.zsh
@@ -190,7 +190,7 @@ run_test_internal() {
     if
       [[ $start != $exp_start ]] ||
       [[ $end != $exp_end ]] ||
-      [[ $highlight_zone[3] != $expected_highlight_zone[3] ]]
+      [[ ${highlight_zone[3]%,} != ${expected_highlight_zone[3]%,} ]]
     then
       print -r -- "not ok $i - $desc - expected ($exp_start $exp_end ${(qqq)expected_highlight_zone[3]}), observed ($start $end ${(qqq)highlight_zone[3]}). $todo"
       if [[ -z $todo ]]; then (( ++print_expected_and_actual )); fi
diff --git a/zsh-syntax-highlighting.zsh b/zsh-syntax-highlighting.zsh
index 6c95e8c..258fced 100644
--- a/zsh-syntax-highlighting.zsh
+++ b/zsh-syntax-highlighting.zsh
@@ -76,12 +76,53 @@ _zsh_highlight()
   # Make it read-only.  Can't combine this with the previous line when POSIX_BUILTINS may be set.
   typeset -r ret
 
+  (( ${+region_highlight} )) || typeset -ga region_highlight
+
+  # Probe the memo= feature, once.
+  (( ${+zsh_highlight__memo_feature} )) || {
+    region_highlight+=( " 0 0 fg=red, memo=zsh-syntax-highlighting" )
+    case ${region_highlight[-1]} in
+      ("0 0 fg=red")
+        # zsh 5.8 or earlier
+        integer -gr zsh_highlight__memo_feature=0
+        ;;
+      ("0 0 fg=red memo=zsh-syntax-highlighting")
+        # zsh 5.9 or later
+        integer -gr zsh_highlight__memo_feature=1
+        ;;
+      (" 0 0 fg=red, memo=zsh-syntax-highlighting") 
+        # Either we were invoked not as a shell widget but as an ordinary
+        # function (which probably means we're running under the test suite),
+        # or we're running under a future version of zsh that has changed the
+        # serialization of $region_highlight from the underlying C struct.
+        #
+        # Assume it's the former.
+        ;&
+      (*)
+        # Fall back to a version check.
+        if is-at-least 5.8.0.3; then
+          integer -gr zsh_highlight__memo_feature=1
+        else
+          integer -gr zsh_highlight__memo_feature=0
+        fi
+        ;;
+    esac
+    region_highlight[-1]=()
+  }
+
+  # Reset region_highlight to build it from scratch
+  if (( zsh_highlight__memo_feature )); then
+    region_highlight=( "${(@)region_highlight:#*memo=zsh-syntax-highlighting*}" )
+  else
+    # Legacy codepath.  Not very interoperable with other plugins (issue #418).
+    region_highlight=()
+  fi
+
   # Remove all highlighting in isearch, so that only the underlining done by zsh itself remains.
   # For details see FAQ entry 'Why does syntax highlighting not work while searching history?'.
   # This disables highlighting during isearch (for reasons explained in README.md) unless zsh is new enough
   # and doesn't have the pattern matching bug
   if [[ $WIDGET == zle-isearch-update ]] && { $zsh_highlight__pat_static_bug || ! (( $+ISEARCHMATCH_ACTIVE )) }; then
-    region_highlight=()
     return $ret
   fi
 
@@ -117,10 +158,6 @@ _zsh_highlight()
   # Do not highlight if there are pending inputs (copy/paste).
   [[ $PENDING -gt 0 ]] && return $ret
 
-  # Reset region highlight to build it from scratch
-  typeset -ga region_highlight
-  region_highlight=();
-
   {
     local cache_place
     local -a region_highlight_copy
@@ -239,7 +276,7 @@ _zsh_highlight_apply_zle_highlight() {
   else
     start=$second end=$first
   fi
-  region_highlight+=("$start $end $region")
+  region_highlight+=("$start $end $region, memo=zsh-syntax-highlighting")
 }
 
 
@@ -279,7 +316,7 @@ _zsh_highlight_add_highlight()
   shift 2
   for highlight; do
     if (( $+ZSH_HIGHLIGHT_STYLES[$highlight] )); then
-      region_highlight+=("$start $end $ZSH_HIGHLIGHT_STYLES[$highlight]")
+      region_highlight+=("$start $end $ZSH_HIGHLIGHT_STYLES[$highlight], memo=zsh-syntax-highlighting")
       break
     fi
   done

Verified to resolve okapia/zsh-viexchange#1 in conjunction with the current snapshot of zsh-users/zsh#57.

@danielshahaf
Copy link
Member

zsh-users/zsh#57 has been merged.

Not directly related, but zsh-viexchange has also added support for the memo= feature (zsh-users/zsh#57), if available: okapia/zsh-viexchange@56fcfb6.

Should we merge the patch in the previous comment, then? On the one hand, there's the forward compatibility question (i.e., the possibility that zsh will change the memo= feature before it's released). On the other hand, with zsh-viexchange and z-sy-h both relying on the new feature, the risk of unintentional incompatibilities is reduced; someone should be dogfooding zsh-users/zsh#57 before it's released, and that someone is z-sy-h, since we proposed that change; and there's already an escape hatch: setting zsh_highlight__memo_feature to zero will bypass the new code.

Merging this to master isn't really a problem, since the patch is compatible with all zsh versions. The real question is, can we release z-sy-h with the probe enabled by default before zsh releases the memo= feature?

@okapia
Copy link
Contributor Author

okapia commented Jun 25, 2020

I'm probably not the best person to comment but I'd have thought it was fairly harmless to merge. In trying to envisage why it might be changed other requirements or issues would have to come to light. Along with the expiry feature we discussed, the only thing which comes to mind is layering the regions to give precedence. Isn't it fairly ugly that z-sy-h needs to duplicate the region highlighting?

It's only with the redrawhook branch that z-sy-h doesn't conflict with the rest of my setup so I'm not sure how much help I can be with testing.

@danielshahaf
Copy link
Member

I'm probably not the best person to comment but I'd have thought it was fairly harmless to merge. In trying to envisage why it might be changed other requirements or issues would have to come to light. Along with the expiry feature we discussed, the only thing which comes to mind is layering the regions to give precedence.

Well, I don't envisage any incompatible changes either, but for me, that doesn't imply the feature is safe to be depended on. I could be overlooking something.

What would an incompatible change be? Some zle code that'll merge adjacent entries of the region_highlights C array and will have to decide what to do with their memo values?

Isn't it fairly ugly that z-sy-h needs to duplicate the region highlighting?

Yes, it is. Filed as #745. Thanks for the reminder.

It's only with the redrawhook branch that z-sy-h doesn't conflict with the rest of my setup so I'm not sure how much help I can be with testing.

No worries. Thanks for being willing to help :-)


I'd like to be pragmatic, but I'd also like to honour the concept that something that hasn't been released isn't subject to backwards compatibility promises. Our options include:

  • hold back releasing the above patch in z-sy-h until the zsh 5.9 release
  • release it before that, with probing enabled by default, and document the very unlikely scenario in which there'd be breakage and people would need to set zsh_highlight__memo_feature=0 in their .zshrc's. The docs could go in changelog.
  • release it before that, off by default with an opt-in knob, and once memo= is released by zsh, follow up with a patch release that enables probing by default

The z-asug interoperability is a relatively high-profile bug (it's been reported independently several times), and the risk of an incompatible change on the zsh side isn't high. That being the case, I think we can permit ourselves to choose the second option, even if it feels like cutting a corner.

So, I'd like to merge the above patch, after adding a changelog hunk, and a comment explaining the magic number 5.8.0.3 in the version check.

@danielshahaf
Copy link
Member

Merging this to master isn't really a problem, since the patch is compatible with all zsh versions. The real question is, can we release z-sy-h with the probe enabled by default before zsh releases the memo= feature?

I asked -workers@ about the compatibility issue in workers/46113.

@danielshahaf
Copy link
Member

Revision 3 of the patch: add HACKING entry, change probing

Verified to resolve okapia/zsh-viexchange#1 in conjunction with the current snapshot of zsh-users/zsh#57.

Also verified to resolve #579 in conjunction with redrawhook; see #579 (comment).

@danielshahaf
Copy link
Member

Pushed in 0.7.1-158-g075c852 (075c852) and 0.7.1-159-g810c2dc (810c2dc).

Interdiff to revision 3

Revision 3 was posted in #418 (comment) above. Interdiff to it:

diff --git a/HACKING.md b/HACKING.md
index 8a41c34..6fd195c 100644
--- a/HACKING.md
+++ b/HACKING.md
@@ -74,11 +74,11 @@ We append to `region_highlight` as follows:
 
 
 ```zsh
-region_highlight+=("$start $end $region, memo=zsh-syntax-highlighting")
+region_highlight+=("$start $end $spec, memo=zsh-syntax-highlighting")
 ```
 
 That comma is required to cause zsh 5.8 and older to ignore the memo without
-ignoring the $region.  It's a hack, but given that no further 5.8.x patch
+ignoring the `$spec`.  It's a hack, but given that no further 5.8.x patch
 releases are planned, it's been deemed acceptable.  See issue #418 and the
 cross-referenced issues.
 
diff --git a/changelog.md b/changelog.md
index d68d15c..daeda05 100644
--- a/changelog.md
+++ b/changelog.md
@@ -1,5 +1,23 @@
 # Changes in HEAD
 
+## Notice about an improbable-but-not-impossible forward incompatibility
+
+Everyone can probably skip this section.
+
+The `master` branch of zsh-syntax-highlighting uses a zsh feature that has not
+yet appeared in a zsh release: the `memo=` feature, added to zsh in commit
+zsh-5.8-172-gdd6e702ee (after zsh 5.8, before zsh 5.9).  In the unlikely event
+that this zsh feature should change in an incompatible way before the next
+stable zsh release, set `zsh_highlight__memo_feature=0` in your .zshrc files to
+disable use of the new feature.
+
+z-sy-h dogfoods the new, unreleased zsh feature because that feature was
+added to zsh at z-sy-h's initiative.  The new feature is used in the fix
+to issue #418.
+
+
+## Other changes:
+
 - Document `$ZSH_HIGHLIGHT_MAXLENGTH`.
   [#698]
 
@@ -89,6 +107,12 @@
   The `assign` style remains supported and has precedence.
   [#585]
 
+- Fix interoperability issue with other plugins that use highlighting.  The fix
+  requires zsh 5.8.0.3 or newer.  (zsh 5.8.0.2-dev from the `master` branch,
+  revision zsh-5.8-172-gdd6e702ee or newer is also fine.)
+  [#418, https://github.com/okapia/zsh-viexchange/issues/1]
+
+
 # Changes in version 0.7.1
 
 - Remove out-of-date information from the 0.7.0 changelog.
diff --git a/tests/test-highlighting.zsh b/tests/test-highlighting.zsh
index d282f81..30e93b1 100755
--- a/tests/test-highlighting.zsh
+++ b/tests/test-highlighting.zsh
@@ -190,7 +190,7 @@ run_test_internal() {
     if
       [[ $start != $exp_start ]] ||
       [[ $end != $exp_end ]] ||
-      [[ ${highlight_zone[3]%,} != ${expected_highlight_zone[3]%,} ]]
+      [[ ${highlight_zone[3]%,} != ${expected_highlight_zone[3]} ]] # remove the comma that's before the memo field
     then
       print -r -- "not ok $i - $desc - expected ($exp_start $exp_end ${(qqq)expected_highlight_zone[3]}), observed ($start $end ${(qqq)highlight_zone[3]}). $todo"
       if [[ -z $todo ]]; then (( ++print_expected_and_actual )); fi
diff --git a/zsh-syntax-highlighting.zsh b/zsh-syntax-highlighting.zsh
index 258fced..f98dc4b 100644
--- a/zsh-syntax-highlighting.zsh
+++ b/zsh-syntax-highlighting.zsh
@@ -76,7 +76,12 @@ _zsh_highlight()
   # Make it read-only.  Can't combine this with the previous line when POSIX_BUILTINS may be set.
   typeset -r ret
 
-  (( ${+region_highlight} )) || typeset -ga region_highlight
+  # $region_highlight should be predefined, either by zle or by the test suite's mock (non-special) array.
+  (( ${+region_highlight} )) || {
+    echo >&2 'zsh-syntax-highlighting: error: $region_highlight is not defined'
+    echo >&2 'zsh-syntax-highlighting: (Check whether zsh-syntax-highlighting was installed according to the instructions.)'
+    return $ret
+  }
 
   # Probe the memo= feature, once.
   (( ${+zsh_highlight__memo_feature} )) || {
@@ -90,16 +95,28 @@ _zsh_highlight()
         # zsh 5.9 or later
         integer -gr zsh_highlight__memo_feature=1
         ;;
-      (" 0 0 fg=red, memo=zsh-syntax-highlighting") 
-        # Either we were invoked not as a shell widget but as an ordinary
-        # function (which probably means we're running under the test suite),
-        # or we're running under a future version of zsh that has changed the
-        # serialization of $region_highlight from the underlying C struct.
-        #
-        # Assume it's the former.
-        ;&
+      (" 0 0 fg=red, memo=zsh-syntax-highlighting") ;&
       (*)
-        # Fall back to a version check.
+        # We can get here in two ways:
+        #
+        # 1. When not running as a widget.  In that case, $region_highlight is
+        # not a special variable (= one with custom getter/setter functions
+        # written in C) but an ordinary one, so the third case pattern matches
+        # and we fall through to this block.  (The test suite uses this codepath.)
+        #
+        # 2. When running under a future version of zsh that will have changed
+        # the serialization of $region_highlight elements from their underlying
+        # C structs, so that none of the previous case patterns will match.
+        #
+        # In either case, fall back to a version check.
+        #
+        # The memo= feature was added to zsh in commit zsh-5.8-172-gdd6e702ee.
+        # The version number at the time was 5.8.0.2-dev (see Config/version.mk).
+        # Therefore, on 5.8.0.3 and newer the memo= feature is available.
+        #
+        # On zsh version 5.8.0.2 between the aforementioned commit and the
+        # first Config/version.mk bump after it (which, at the time of writing,
+        # is yet to come), this condition will false negative.
         if is-at-least 5.8.0.3; then
           integer -gr zsh_highlight__memo_feature=1
         else

@danielshahaf
Copy link
Member

For future reference, the just-pushed 0.8.0-alpha1-pre-redrawhook tag contains this change. When the next zsh release rolls around (anything newer than 5.8.0.2-dev), people who test it may want to also test a version of z-sy-h that's at least that tag or newer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants