Stop depending on zsh-syntax-highlighting implementation details

[danielshahaf]

Currently, this project defines the functions _zsh_highlight and _zsh_highlight_bind_widgets if they are not already available.

Those functions are private functions of zsh-syntax-highlighting, in its namespace, and z-sy-h reserves the right to change the semantics of those functions incompatibly without notice.

As a matter of good engineering, please stop depending on implementation details of z-sy-h.

zsh-history-substring-search/zsh-history-substring-search.zsh

Lines 89 to 176 in 557d25e

	if [[ $+functions[_zsh_highlight] -eq 0 ]]; then
	#
	# Dummy implementation of _zsh_highlight() that
	# simply removes any existing highlights when the
	# user inserts printable characters into $BUFFER.
	#
	_zsh_highlight() {
	if [[ $KEYS == [[:print:]] ]]; then
	region_highlight=()
	fi
	}
	#
	# The following snippet was taken from the zsh-syntax-highlighting project:
	#
	# https://github.com/zsh-users/zsh-syntax-highlighting/blob/56b134f5d62ae3d4e66c7f52bd0cc2595f9b305b/zsh-syntax-highlighting.zsh#L126-161
	#
	# Copyright (c) 2010-2011 zsh-syntax-highlighting contributors
	# All rights reserved.
	#
	# Redistribution and use in source and binary forms, with or without
	# modification, are permitted provided that the following conditions are
	# met:
	#
	# * Redistributions of source code must retain the above copyright
	# notice, this list of conditions and the following disclaimer.
	#
	# * Redistributions in binary form must reproduce the above copyright
	# notice, this list of conditions and the following disclaimer in the
	# documentation and/or other materials provided with the distribution.
	#
	# * Neither the name of the zsh-syntax-highlighting contributors nor the
	# names of its contributors may be used to endorse or promote products
	# derived from this software without specific prior written permission.
	#
	# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
	# IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
	# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
	# PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR
	# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
	# EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
	# PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
	# PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
	# LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
	# NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
	# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
	#
	#--------------8<-------------------8<-------------------8<-----------------
	# Rebind all ZLE widgets to make them invoke _zsh_highlights.
	_zsh_highlight_bind_widgets()
	{
	# Load ZSH module zsh/zleparameter, needed to override user defined widgets.
	zmodload zsh/zleparameter 2>/dev/null || {
	echo 'zsh-syntax-highlighting: failed loading zsh/zleparameter.' >&2
	return 1
	}
	# Override ZLE widgets to make them invoke _zsh_highlight.
	local cur_widget
	for cur_widget in ${${(f)"$(builtin zle -la)"}:#(.*|_*|orig-*|run-help|which-command|beep|yank*)}; do
	case $widgets[$cur_widget] in
	# Already rebound event: do nothing.
	user:$cur_widget|user:_zsh_highlight_widget_*);;
	# User defined widget: override and rebind old one with prefix "orig-".
	user:*) eval "zle -N orig-$cur_widget ${widgets[$cur_widget]#*:}; \
	_zsh_highlight_widget_$cur_widget() { builtin zle orig-$cur_widget -- \"\$@\" && _zsh_highlight }; \
	zle -N $cur_widget _zsh_highlight_widget_$cur_widget";;
	# Completion widget: override and rebind old one with prefix "orig-".
	completion:*) eval "zle -C orig-$cur_widget ${${widgets[$cur_widget]#*:}/:/ }; \
	_zsh_highlight_widget_$cur_widget() { builtin zle orig-$cur_widget -- \"\$@\" && _zsh_highlight }; \
	zle -N $cur_widget _zsh_highlight_widget_$cur_widget";;
	# Builtin widget: override and make it call the builtin ".widget".
	builtin) eval "_zsh_highlight_widget_$cur_widget() { builtin zle .$cur_widget -- \"\$@\" && _zsh_highlight }; \
	zle -N $cur_widget _zsh_highlight_widget_$cur_widget";;
	# Default: unhandled case.
	*) echo "zsh-syntax-highlighting: unhandled ZLE widget '$cur_widget'" >&2 ;;
	esac
	done
	}
	#-------------->8------------------->8------------------->8-----------------
	_zsh_highlight_bind_widgets
	fi

[danielshahaf]

I should add: I'm the maintainer of z-sy-h, and I'd be happy to make changes on z-sy-h's side to make parts of it more reusable by other projects. Just open an issue and say what you want made reusable ☺.

[sunaku]

It's been a while but AFAIK that code was embedded here to make both plugins play well together. 💭

Could you expose a public API function that rebinds all ZLE widgets to invoke a user-specified function? 🎁 That way we can drop both _zsh_highlights() and _zsh_highlight_bind_widgets() from this codebase.

[danielshahaf]

_zsh_highlight_bind_widgets is going away: zsh 5.3 will have two new features that z-sy-h will use in lieu of _zsh_highlight_bind_widgets. (With latest zsh master: autoload -U add-zle-hook-widget && add-zle-hook-widget zle-line-pre-redraw foo where foo is a zle widget name.)

Encapsulating the "Use _bind_widgets under zsh<5.3 and zle-line-pre-redraw under zsh≥5.3" is an interesting question. z-sy-h will have such code written to it (tracked in zsh-users/zsh-syntax-highlighting#245). Once it's written (the dust hasn't settled on the upstream API details), we can think of how to make it reusable by any plugin. (/cc @psprint as this question is comparable to his https://github.com/psprint/allcompletions.)

However, I don't think such a library function being made available is a prerequisite for addressing my request, which was simply for z-h-s-s to stop defining functions called _zsh_highlight_*. I'm simply asking for the cached copy of the function embedded in z-h-s-s.zsh to be renamed something other than _zsh_highlight_*.

I realize that wouldn't solve the technical debt that duplicating the function definition introduced. However, it would be an improvement and a step in the right direction (it would prevent a class of bugs).

To be fair, _zsh_highlight_bind_widgets is itself believed to have a namespacing bug, zsh-users/zsh-syntax-highlighting#305, which boils down to "If you duplicate the function definition, change the orig-* prefix to something else".

Cheers!

[guidovansteen]

Hi @danielshahaf, Thanks a lot for all your work on z-sy-h! Unfortunately I do not understand the details of the issues around zle-line-pre-redraw. However, would a simple perl -p -i -e "s/orig-/zssh-/" ./zsh-history-substring-search.zsh be enough to satisfy the above request?

[danielshahaf]

Good morning, @guidovansteen.

However, would a simple perl -p -i -e "s/orig-/zssh-/" ./zsh-history-substring-search.zsh be enough to satisfy the above request?

It would solve one problem, but leave one other problem and one other edge-case problem unresolved.

The problem it would solve is coexistence of z-h-s-s and z-sy-h under zsh≤5.2.

The problem it leaves unresolved is the fact that z-h-s-s defines and invokes functions in the _zsh_highlight_* namespace, which are considered implementation details of z-sy-h.

The edge-case problem is a known bug in _bind_widgets: so long as it uses a static naming pattern, be it orig-$foo or zssh-$foo or $HOST-$foo, it is vulnerable to infinite loops after source z-h-s-s; source z-sy-h; source z-h-s-s [which is common when doing source .zshrc]: see zsh-users/zsh-syntax-highlighting#305 (comment). This bug only applied to _bind_widgets, it does not apply to pre-redraw.

So, in short: that perl pie would effect an improvement, but it wouldn't solve the issue I opened this ticket about.

Unfortunately I do not understand the details of the issues around zle-line-pre-redraw.

Short summary: in zsh 5.3, one will be able to do f() {}; zle -N f; autoload add-zle-hook-widget; add-zle-hook-widget zle-line-pre-redraw f to get f to be called "after every keypress". In 5.2 and earlier, _zsh_highlight_bind_widgets simulates the same effect by binding f to every widget.

Cheers,

[guidovansteen]

Thanks @danielshahaf. Would it help if we use something like "s/orig-/zssh-$RANDOM/"?

[danielshahaf]

I assume that'd solve the widgets issue but not the functions issue.

[mrjohannchang]

People who want to use this plugin but don't want to enable zsh-syntax-highlighting can install this fork https://github.com/changyuheng/zsh-syntax-highlighting instead. This fork has all the functions of zsh-syntax-highlighting but not enabling it.

The reason I don't want to enable zsh-syntax-highlighting is that it's performance is too bad. If you paste multiple lines of commands at once, your terminal would get stuck for a while which can be longer than 10 seconds.

[sunaku]

This zsh-history-substring-search plugin does not require zsh-syntax-highlighting! 👮‍♂️ It stands alone. 🥇
If zsh-syntax-highlighting is present, then this plugin tries to play nicely with it. That's all, nothing more.

[mrjohannchang]

@sunaku Maybe I was wrong but I got this error when I disabled zsh-syntax-highlighting.

[sunaku]

@changyuheng That might a side-effect of something else in your Zsh configuration or environment. At minimum, this is what should happen when you use this plugin (all by itself, inside a bare -f Zsh session):

[mrjohannchang]

@sunaku It seems like since I had installed zsh-syntax-highlighting before, I have to remove .zcompdump so that zsh-history-substring-search can get loaded properly.

[danielshahaf]

The reason I don't want to enable zsh-syntax-highlighting is that it's performance is too bad. If you paste multiple lines of commands at once, your terminal would get stuck for a while which can be longer than 10 seconds.

You may want to set ZSH_HIGHLIGHT_MAXLENGTH=100 in your .zshrc. If you see further issues with zsh-syntax-highlighting, please report them on the z-sy-h issue tracker (or IRC). Thanks. (I'm a z-sy-h maintainer)

[mrjohannchang]

@danielshahaf It works perfectly, thank you!