RFC: region_highlight inter-plugin interoperability: echo service?

RFC: region_highlight inter-plugin interoperability: echo service?

[Daniel Shahaf]

z-sy-h would like to have a way to know which region_highlight entries
it had added, so it can remove only those.  This will help interoperability
with other plugins [1,2].

It won't suffice for the plugin to remember the values it added: if
z-sy-h adds "0 42 fg=foo" and then a self-insert occurs, the value
observed when z-sy-h next runs might be "1 43 fg=foo".

Eric, Oliver, and I discussed this on [PR57].  We considered several
options:

- Extend the region_highlight element syntax with some sort of value
  that will be echoed back when the array is next accessed.  For
  example, if a plugin sets "0 42 fg=foo memo=bar", then the next time
  the array is read there will be an element with "memo=bar" (unless
  some other plugin intervened and removed that element in the
  meantime).

  The pull request as it stands calls this feature "owner=", but
  consensus on the pull request was to rename it "memo=".

  This has one known issue [3]: the syntax "0 42 fg=foo memo=bar" will
  be parsed by existing zsh as the attribute «fg» with the value «foo
  memo=bar» unless a comma is added after "foo".  With that comma, both
  current zsh and zsh with the patch applied parse that value the same
  way (current zsh will ignore the memo, of course, but honour the fg
  attribute).

- Extend the region_highlight element syntax with some sort of
  "expire_upon=before-next-redraw" feature, implemented by zle itself.
  Consensus on the PR was to punt on this as it can continue to be
  implemented in script code.  Furthermore, there are plugins (such as
  Oliver's vi-exchange) that don't want their highlights to expire
  automatically, but would still benefit from adding a memo=* feature.

- A proposal to encapsulate region_highlight with a builtin command that
  provides CRUD semantics and an opportunity for running C code during
  CRUD operations was raised, but consensus on the PR was that the
  existing interface is preferable and that no compelling use-case for
  running C code was identified.

Thus, we propose to add a memo=* feature to region_highlight, as
outlined in the first bullet above, based on the patch that is PR57.

Comments, please?

Cheers,

Daniel

P.S.  We've discussed this for a long time across multiple tickets and
projects, so apologies on my behalf if I've omitted some context.  If
so, just ask.

[1] https://github.com/zsh-users/zsh-syntax-highlighting/issues/418
[2] https://github.com/zsh-users/zsh-syntax-highlighting/issues/579
[PR57] https://github.com/zsh-users/zsh/pull/57
[3] https://github.com/zsh-users/zsh/pull/57#discussion_r419548695

[PATCH] Re: RFC: region_highlight inter-plugin interoperability: echo service?

[Daniel Shahaf]

Daniel Shahaf wrote on Sat, 06 Jun 2020 07:20 +0000:
> Thus, we propose to add a memo=* feature to region_highlight, as
> outlined in the first bullet above, based on the patch that is PR57.

Here's a patch.  I've verified that, in conjunction with a z-sy-h patch
posted to [1], it fixes a plugin interoperability issue [4] reported
against Oliver's viexchange plugin.

[[[
diff --git a/Doc/Zsh/zle.yo b/Doc/Zsh/zle.yo
index c928b8ca2..909abeb49 100644
--- a/Doc/Zsh/zle.yo
+++ b/Doc/Zsh/zle.yo
@@ -974,27 +974,39 @@ of the non-editable parts of the command line in tt(PREDISPLAY)
 and tt(POSTDISPLAY) are possible, but note that the tt(P) flag
 is needed for character indexing to include tt(PREDISPLAY).
 
-Each string consists of the following parts:
+Each string consists of the following whitespace-separated parts:
 
 startitemize()
 itemiz(Optionally, a `tt(P)' to signify that the start and end offset that
 follow include any string set by the tt(PREDISPLAY) special parameter;
 this is needed if the predisplay string itself is to be highlighted.
-Whitespace may follow the `tt(P)'.)
-itemiz(A start offset in the same units as tt(CURSOR), terminated by
-whitespace.)
-itemiz(An end offset in the same units as tt(CURSOR), terminated by
-whitespace.)
+Whitespace between the `tt(P)' and the start offset is optional.)
+itemiz(A start offset in the same units as tt(CURSOR).)
+itemiz(An end offset in the same units as tt(CURSOR).)
 itemiz(A highlight specification in the same format as
 used for contexts in the parameter tt(zle_highlight), see
 ifnzman(noderef(Character Highlighting))\
 ifzman(the section `Character Highlighting' below);
-for example, tt(standout) or tt(fg=red,bold)).
+for example, tt(standout) or tt(fg=red,bold).)
+itemiz(Optionally, a string of the form `tt(memo=)var(token)'.
+The var(token) consists of everything between the `tt(=)' and the next
+whitespace, comma, NUL, or the end of the string.
+The var(token) is preserved verbatim but not parsed in any way.
+
+Plugins may use this to identify array elements they have added: for example,
+a plugin might set var(token) to its (the plugin's) name and then use
+`tt(region_highlight=+LPAR() ${region_highlight:#*memo=)var(token)tt(} +RPAR())'
+in order to remove array elements it have added.
+
+(This example uses the `tt(${)var(name)tt(:#)var(pattern)tt(})' array-grepping
+syntax described in
+ifzman(the section `Parameter Expansion' in zmanref(zshexpn))\
+ifnzman(noderef(Parameter Expansion)).))
 enditemize()
 
 For example, 
 
-example(region_highlight=("P0 20 bold"))
+example(region_highlight=("P0 20 bold memo=foobar"))
 
 specifies that the first twenty characters of the text including
 any predisplay string should be highlighted in bold.
@@ -1002,6 +1014,14 @@ any predisplay string should be highlighted in bold.
 Note that the effect of tt(region_highlight) is not saved and disappears
 as soon as the line is accepted.
 
+Note that zsh 5.8 and older do not support the `tt(memo=)var(token)' field
+and may misparse the third (highlight specification) field when a memo
+is given.
+COMMENT(The syntax `tt(0 20 bold, memo=foobar)' (with an auxiliary comma)
+happens to work on both zsh <=5.8 and zsh 5.9-to-be, but that seems to be more of
+an accident of implementation than something we should make a first-class-citizen
+API promise.  It's mentioned in the "Incompatibilities" section of README.)
+
 The final highlighting on the command line depends on both tt(region_highlight)
 and tt(zle_highlight); see
 ifzman(the section CHARACTER HIGHLIGHTING below)\
diff --git a/README b/README
index 8ae615153..9b1b1605f 100644
--- a/README
+++ b/README
@@ -83,6 +83,15 @@ affects you, make the implied colons in the first pattern explicit, as in:
     zstyle ':foo:*:baz:*' style value2
 This will use value1 in both 5.8 and 5.9.
 
+Elements of the region_highlight array have gained a fourth space-separated
+field.  Code written against 5.9 that sets the new field may break under 5.8:
+for example, the element "0 20 bold memo=foo", which is valid under 5.9, would
+not work under 5.8.  (Under the hood, 5.8 does not recognize the space as
+terminating the highlighting specification.)  On the other hand, code that does
+not set the new, fourth field will continue to work under both 5.8 and 5.9.
+(As it happens, adding a comma after "bold" will make both 5.8 and 5.9 do the
+right thing, but this should be viewed as an unsupported hack.)
+
 Incompatibilities between 5.7.1 and 5.8
 ---------------------------------------
 
diff --git a/Src/Zle/zle.h b/Src/Zle/zle.h
index 609493f8c..391586c4a 100644
--- a/Src/Zle/zle.h
+++ b/Src/Zle/zle.h
@@ -447,6 +447,10 @@ struct region_highlight {
      * Any of the flags defined above.
      */
     int flags;
+    /*
+     * User-settable "memo" key.  Metafied.
+     */
+    const char *memo;
 };
 
 /*
diff --git a/Src/Zle/zle_refresh.c b/Src/Zle/zle_refresh.c
index 7b8593dec..d9d9503e2 100644
--- a/Src/Zle/zle_refresh.c
+++ b/Src/Zle/zle_refresh.c
@@ -212,9 +212,9 @@ static zattr default_atr_on, special_atr_on;
 
 /*
  * Array of region highlights, no special termination.
- * The first element (0) always describes the region between
- * point and mark.  Any other elements are set by the user
- * via the parameter region_highlight.
+ * The first N_SPECIAL_HIGHLIGHTS elements describe special uses of
+ * highlighting, documented under N_SPECIAL_HIGHLIGHTS.
+ * Any other elements are set by the user via the parameter region_highlight.
  */
 
 /**/
@@ -414,16 +414,19 @@ get_region_highlight(UNUSED(Param pm))
 	 arrsize--;
 	 rhp++, arrp++) {
 	char digbuf1[DIGBUFSIZE], digbuf2[DIGBUFSIZE];
-	int atrlen = 0, alloclen;
+	int atrlen, alloclen;
+	const char memo_equals[] = "memo=";
 
 	sprintf(digbuf1, "%d", rhp->start);
 	sprintf(digbuf2, "%d", rhp->end);
 
 	atrlen = output_highlight(rhp->atr, NULL);
 	alloclen = atrlen + strlen(digbuf1) + strlen(digbuf2) +
-	    3; /* 2 spaces, 1 0 */
+	    3; /* 2 spaces, 1 terminating NUL */
 	if (rhp->flags & ZRH_PREDISPLAY)
 	    alloclen += 2; /* "P " */
+	if (rhp->memo)
+	    alloclen += 1 /* space */ + strlen(memo_equals) + strlen(rhp->memo);
 	*arrp = (char *)zhalloc(alloclen * sizeof(char));
 	/*
 	 * On input we allow a space after the flags.
@@ -436,6 +439,12 @@ get_region_highlight(UNUSED(Param pm))
 		(rhp->flags & ZRH_PREDISPLAY) ? "P" : "",
 		digbuf1, digbuf2);
 	(void)output_highlight(rhp->atr, *arrp + strlen(*arrp));
+
+	if (rhp->memo) {
+	    strcat(*arrp, " ");
+	    strcat(*arrp, memo_equals);
+	    strcat(*arrp, rhp->memo);
+	}
     }
     *arrp = NULL;
     return retarr;
@@ -460,6 +469,8 @@ set_region_highlight(UNUSED(Param pm), char **aval)
 	/* no null termination, but include special highlighting at start */
 	int newsize = len + N_SPECIAL_HIGHLIGHTS;
 	int diffsize = newsize - n_region_highlights;
+
+	free_region_highlights_memos();
 	region_highlights = (struct region_highlight *)
 	    zrealloc(region_highlights,
 		     sizeof(struct region_highlight) * newsize);
@@ -476,6 +487,7 @@ set_region_highlight(UNUSED(Param pm), char **aval)
 	 *aval;
 	 rhp++, aval++) {
 	char *strp, *oldstrp;
+	const char memo_equals[] = "memo=";
 
 	oldstrp = *aval;
 	if (*oldstrp == 'P') {
@@ -502,7 +514,44 @@ set_region_highlight(UNUSED(Param pm), char **aval)
 	while (inblank(*strp))
 	    strp++;
 
-	match_highlight(strp, &rhp->atr);
+	strp = (char*) match_highlight(strp, &rhp->atr);
+
+	while (inblank(*strp))
+	    strp++;
+
+	if (strpfx(memo_equals, strp)) {
+	    const char *memo_start = strp + strlen(memo_equals);
+	    const char *i, *memo_end;
+
+	    /* 
+	     * Forward compatibility: end parsing at a comma or whitespace to
+	     * allow the following extensions:
+	     *
+	     * - A fifth field: "0 20 bold memo=foo bar".
+	     *
+	     * - Additional attributes in the fourth field: "0 20 bold memo=foo,bar"
+	     *   and "0 20 bold memo=foo\0bar".
+	     *
+	     * For similar reasons, we don't flag an error if the fourth field
+	     * doesn't start with "memo=" as we expect.
+	     */
+	    i = memo_start;
+
+	    /* ### TODO: Consider optimizing the common case that memo_start to
+	     *           end-of-string is entirely ASCII */
+	    while (1) {
+		int nbytes;
+		convchar_t c = unmeta_one(i, &nbytes);
+
+		if (c == '\0' || c == ',' || inblank(c)) {
+		    memo_end = i;
+		    break;
+		} else
+		    i += nbytes;
+	    }
+	    rhp->memo = ztrduppfx(memo_start, memo_end - memo_start);
+	} else
+	    rhp->memo = NULL;
     }
 
     freearray(av);
@@ -2797,6 +2846,7 @@ zle_refresh_finish(void)
 
     if (region_highlights)
     {
+	free_region_highlights_memos();
 	zfree(region_highlights,
 	      sizeof(struct region_highlight) * n_region_highlights);
 	region_highlights = NULL;
diff --git a/Src/Zle/zle_utils.c b/Src/Zle/zle_utils.c
index 2b306fdcd..ea18aafde 100644
--- a/Src/Zle/zle_utils.c
+++ b/Src/Zle/zle_utils.c
@@ -557,6 +557,22 @@ zlegetline(int *ll, int *cs)
 }
 
 
+/*
+ * free() the 'memo' elements of region_highlights.
+ */
+
+/**/
+void
+free_region_highlights_memos(void)
+{
+    struct region_highlight *rhp;
+    for (rhp = region_highlights;
+	 rhp < region_highlights + n_region_highlights;
+	 rhp++) {
+	zsfree(rhp->memo);
+    }
+}
+
 /* Forward reference */
 struct zle_region;
 
@@ -568,6 +584,7 @@ struct zle_region  {
     int start;
     int end;
     int flags;
+    const char *memo;
 };
 
 /* Forward reference */
@@ -632,6 +649,7 @@ zle_save_positions(void)
 	    newrhp->next = NULL;
 	    newrhp->atr = rhp->atr;
 	    newrhp->flags = rhp->flags;
+	    newrhp->memo = ztrdup(rhp->memo);
 	    if (zlemetaline) {
 		newrhp->start = rhp->start_meta;
 		newrhp->end = rhp->end_meta;
@@ -682,6 +700,7 @@ zle_restore_positions(void)
 	     nreg++, oldrhp = oldrhp->next)
 	    ;
 	if (nreg + N_SPECIAL_HIGHLIGHTS != n_region_highlights) {
+	    free_region_highlights_memos();
 	    n_region_highlights = nreg + N_SPECIAL_HIGHLIGHTS;
 	    region_highlights = (struct region_highlight *)
 		zrealloc(region_highlights,
@@ -694,6 +713,7 @@ zle_restore_positions(void)
 
 	    rhp->atr = oldrhp->atr;
 	    rhp->flags = oldrhp->flags;
+	    rhp->memo = oldrhp->memo; /* transferring ownership of the permanently-allocated memory */
 	    if (zlemetaline) {
 		rhp->start_meta = oldrhp->start;
 		rhp->end_meta = oldrhp->end;
@@ -707,6 +727,7 @@ zle_restore_positions(void)
 	    rhp++;
 	}
     } else if (region_highlights) {
+	free_region_highlights_memos();
 	zfree(region_highlights, sizeof(struct region_highlight) *
 	      n_region_highlights);
 	region_highlights  = NULL;
diff --git a/Src/mem.c b/Src/mem.c
index 5951e57ed..a20e3f729 100644
--- a/Src/mem.c
+++ b/Src/mem.c
@@ -1655,7 +1655,7 @@ free(FREE_ARG_T p)
 
 /**/
 mod_export void
-zsfree(char *p)
+zsfree(const char *p)
 {
     if (p)
 	zfree(p, strlen(p) + 1);
diff --git a/Src/prompt.c b/Src/prompt.c
index b65bfb86b..bc9734720 100644
--- a/Src/prompt.c
+++ b/Src/prompt.c
@@ -1724,10 +1724,11 @@ match_colour(const char **teststrp, int is_fg, int colour)
 /*
  * Match a set of highlights in the given teststr.
  * Set *on_var to reflect the values found.
+ * Return a pointer to the first character not consumed.
  */
 
 /**/
-mod_export void
+mod_export const char *
 match_highlight(const char *teststr, zattr *on_var)
 {
     int found = 1;
@@ -1745,7 +1746,7 @@ match_highlight(const char *teststr, zattr *on_var)
 	    atr = match_colour(&teststr, is_fg, 0);
 	    if (*teststr == ',')
 		teststr++;
-	    else if (*teststr)
+	    else if (*teststr && *teststr != ' ')
 		break;
 	    found = 1;
 	    /* skip out of range colours but keep scanning attributes */
@@ -1758,7 +1759,7 @@ match_highlight(const char *teststr, zattr *on_var)
 
 		    if (*val == ',')
 			val++;
-		    else if (*val)
+		    else if (*val && *val != ' ')
 			break;
 
 		    *on_var |= hl->mask_on;
@@ -1769,6 +1770,8 @@ match_highlight(const char *teststr, zattr *on_var)
 	    }
 	}
     }
+
+    return teststr;
 }
 
 /*
diff --git a/Test/X04zlehighlight.ztst b/Test/X04zlehighlight.ztst
index 475a2e309..7ab050bee 100644
--- a/Test/X04zlehighlight.ztst
+++ b/Test/X04zlehighlight.ztst
@@ -94,6 +94,50 @@
 0:basic region_highlight with 8 colors
 >0m27m24mCDE|32|trueCDE|39|
 
+  zpty_start
+  zpty_input 'rh_widget() { region_highlight+=( "0 4 fg=green memo=someplugin" ); typeset -p region_highlight }'
+  zpty_input 'zle -N rh_widget'
+  zpty_input 'bindkey "\C-a" rh_widget'
+  zpty_enable_zle
+  zpty_input $'\C-a'
+  zpty_line
+  zpty_stop
+0:region_highlight memo information round trips
+>typeset -a region_highlight=( '0 4 fg=green memo=someplugin' )
+
+  zpty_start
+  zpty_input 'rh_widget() { region_highlight+=( "0 4 fg=green memo=someplugin,futureattribute=futurevalue" ); typeset -p region_highlight }'
+  zpty_input 'zle -N rh_widget'
+  zpty_input 'bindkey "\C-a" rh_widget'
+  zpty_enable_zle
+  zpty_input $'\C-a'
+  zpty_line
+  zpty_stop
+0:region_highlight memo information forward compatibility, #1
+>typeset -a region_highlight=( '0 4 fg=green memo=someplugin' )
+
+  zpty_start
+  zpty_input 'rh_widget() { region_highlight+=( "0 4 fg=green memo=someplugin futurefifthfield" ); typeset -p region_highlight }'
+  zpty_input 'zle -N rh_widget'
+  zpty_input 'bindkey "\C-a" rh_widget'
+  zpty_enable_zle
+  zpty_input $'\C-a'
+  zpty_line
+  zpty_stop
+0:region_highlight memo information forward compatibility, #2
+>typeset -a region_highlight=( '0 4 fg=green memo=someplugin' )
+
+  zpty_start
+  zpty_input 'rh_widget() { region_highlight+=( "0 4 fg=green memo=some'$'\0''plugin" ); typeset -p region_highlight }'
+  zpty_input 'zle -N rh_widget'
+  zpty_input 'bindkey "\C-a" rh_widget'
+  zpty_enable_zle
+  zpty_input $'\C-a'
+  zpty_line
+  zpty_stop
+0:region_highlight memo information forward compatibility, #3: NULs
+>typeset -a region_highlight=( '0 4 fg=green memo=some' )
+
   zpty_start
   zpty_input 'rh_widget() { BUFFER="true"; region_highlight+=( "0 4 fg=#040810" ); }'
   zpty_input 'zle -N rh_widget'
]]]

Cheers,

Daniel

> [1] https://github.com/zsh-users/zsh-syntax-highlighting/issues/418
> [2] https://github.com/zsh-users/zsh-syntax-highlighting/issues/579
> [PR57] https://github.com/zsh-users/zsh/pull/57
> [3] https://github.com/zsh-users/zsh/pull/57#discussion_r419548695

[4] https://github.com/okapia/zsh-viexchange/issues/1

Re: [PATCH] Re: RFC: region_highlight inter-plugin interoperability: echo service?

[Daniel Shahaf]

Daniel Shahaf wrote on Thu, 18 Jun 2020 10:33 +0000:
> +++ b/Src/Zle/zle_refresh.c
> @@ -414,16 +414,19 @@ get_region_highlight(UNUSED(Param pm))
>  	 arrsize--;
>  	 rhp++, arrp++) {
>  	char digbuf1[DIGBUFSIZE], digbuf2[DIGBUFSIZE];
> -	int atrlen = 0, alloclen;
> +	int atrlen, alloclen;
> +	const char memo_equals[] = "memo=";
>  
>  	sprintf(digbuf1, "%d", rhp->start);
>  	sprintf(digbuf2, "%d", rhp->end);
>  
>  	atrlen = output_highlight(rhp->atr, NULL);
>  	alloclen = atrlen + strlen(digbuf1) + strlen(digbuf2) +
> -	    3; /* 2 spaces, 1 0 */
> +	    3; /* 2 spaces, 1 terminating NUL */
>  	if (rhp->flags & ZRH_PREDISPLAY)
>  	    alloclen += 2; /* "P " */
> +	if (rhp->memo)
> +	    alloclen += 1 /* space */ + strlen(memo_equals) + strlen(rhp->memo);
>  	*arrp = (char *)zhalloc(alloclen * sizeof(char));
>  	/*
>  	 * On input we allow a space after the flags.
> @@ -436,6 +439,12 @@ get_region_highlight(UNUSED(Param pm))
>  		(rhp->flags & ZRH_PREDISPLAY) ? "P" : "",
>  		digbuf1, digbuf2);
>  	(void)output_highlight(rhp->atr, *arrp + strlen(*arrp));
> +
> +	if (rhp->memo) {
> +	    strcat(*arrp, " ");
> +	    strcat(*arrp, memo_equals);
> +	    strcat(*arrp, rhp->memo);
> +	}
>      }  
>      *arrp = NULL;
>      return retarr;
> +++ b/Src/Zle/zle_utils.c
> @@ -557,6 +557,22 @@ zlegetline(int *ll, int *cs)
> +/*
> + * free() the 'memo' elements of region_highlights.
> + */
> +
> +/**/
> +void
> +free_region_highlights_memos(void)
> +{
> +    struct region_highlight *rhp;
> +    for (rhp = region_highlights;
> +	 rhp < region_highlights + n_region_highlights;
> +	 rhp++) {
> +	zsfree(rhp->memo);

s/zsfree/zfree/

since the calculation of alloclen in get_region_highlight() can result
in an alloclen that's different to strlen(rhp->memo)+1 — at least when
the ZRH_PREDISPLAY bit is set.

(This only matters for --enable-zsh-mem builds, and even then only if
alloclen happens to be exactly the one value that will cause zfree()'s
fr_rec condition's truth value to flip, but still.)

Cheers,

Daniel

> +    }
> +}  

Re: [PATCH] Re: RFC: region_highlight inter-plugin interoperability: echo service?

[Daniel Shahaf]

Daniel Shahaf wrote on Thu, 18 Jun 2020 10:33 +0000:
> Daniel Shahaf wrote on Sat, 06 Jun 2020 07:20 +0000:
> > Thus, we propose to add a memo=* feature to region_highlight, as
> > outlined in the first bullet above, based on the patch that is PR57.  
> 
> Here's a patch.  I've verified that, in conjunction with a z-sy-h patch
> posted to [1], it fixes a plugin interoperability issue [4] reported
> against Oliver's viexchange plugin.

Merged.

Oliver had already added support for this to his viexchange plugin.
I expect the comparable z-sy-h patch to be merged to z-sy-h master
soon.

I'd like z-sy-h to dogfood the new feature, but I'm also concerned
about compatibility, as zsh hasn't released the feature yet.  I don't
see a problem with turning the functionality on by default in z-sy-h's
master branch, but I'm not so sure what to do in z-sy-h releases.

Would it be okay for the next z-sy-h release to use the memo= feature
by default, if available?  Or should z-sy-h not use the memo= feature
by default until zsh releases it?

(The next z-sy-h release might be sooner than the next zsh release.)

Cheers,

Daniel