Re: [PATCH] _pick_variant: Update builtin check

[Matthew Martin <phy1729@xxxxxxxxx>]

On Tue, Mar 19, 2019 at 10:38:15PM -0500, Matthew Martin wrote:
> On Tue, Mar 19, 2019 at 09:05:13PM -0500, Matthew Martin wrote:
> > Move the builtin check prior to the cache lookup and don't cache
> > builtins if -b is provided. This corrects the result if a user uses both
> > a builtin and external version of a command in one shell session.
> > 
> > The command may be builtin only if all precommands are
> > builtin-preserving which are enumerated in builtin_precommands.
> > Since a builtin-preserving precommand must be itself builtin, I figure
> > it's easier to maintain as a white list. This corrects the case Daniel
> > brought up: command chmod <tab>.
> 
> I've realized that I was only testing on a system that uses the fallback
> variant and the patch will give incorrect results on GNU systems since
> _call_program may call a builtin. I'll rework the patch (although I'm
> open to suggestions for fixing it).

There are four booleans in play:
- If command (or a non-builtin-preserving precommand) is specified
  (${#precommands:|builtin_precommands})
- If builtin is specified ($+precommands[(r)builtin])
- If -b is passed to _pick_variant ($+opts[-b])
- If the command is a builtin ($+builtins[$opts[-c]])

If command is specified, set pre=command to force _call_program to run
the command. Eligible for caching.

If -b is passed and (builtin is specified or the command is a builtin),
return the -b variant. Ineligible for caching.

If builtin is specified, set pre=builtin to force _call_program to run
the builtin. Ineligible for caching.

This leaves 3 cases:
- command not specified, builtin not specified, -b passed, is not builtin
- command not specified, builtin not specified, -b not passed, is not builtin
- command not specified, builtin not specified, -b not passed, is builtin

In all cases, fall through to _call_program with no prefix set.
In the first two cases, the command is not a builtin, so they are
clearly caching eligible. In the last case, I'm unsure if caching should
be allowed, but one would hope there are few builtins that have
a completer that uses _pick_variant and does not pass -b. Since this
case should be rare, I've omitted it from the cache eligibility check.

- Matthew Martin

---
 Completion/Base/Utility/_pick_variant | 28 ++++++++++++++++++---------
 Completion/Zsh/Command/_builtin       |  2 ++
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/Completion/Base/Utility/_pick_variant b/Completion/Base/Utility/_pick_variant
index 9099e3599..22faa8381 100644
--- a/Completion/Base/Utility/_pick_variant
+++ b/Completion/Base/Utility/_pick_variant
@@ -1,9 +1,11 @@
 #autoload
 
-local output cmd pat
-local -a var
+local output cmd pat pre
+local -a builtin_precommands var
 local -A opts
 
+builtin_precommands=(- builtin eval exec nocorrect noglob time)
+
 (( $+_cmd_variant )) || typeset -gA _cmd_variant
 
 zparseopts -D -A opts b: c: r:
@@ -13,19 +15,27 @@ while [[ $1 = *=* ]]; do
   var+=( "${1%%\=*}" "${1#*=}" )
   shift
 done
-if (( $+_cmd_variant[$opts[-c]] )); then
-  (( $+opts[-r] )) && eval "${opts[-r]}=${_cmd_variant[$opts[-c]]}"
-  [[ $_cmd_variant[$opts[-c]] = "$1" ]] && return 1
+
+if (( ${#precommands:|builtin_precommands} )); then
+  pre=command
+elif (( $+opts[-b] && ( $precommands[(r)builtin] || $+builtins[$opts[-c]] ) )); then
+  (( $+opts[-r] )) && eval "${opts[-r]}=$opts[-b]"
   return 0
+elif (( $precommands[(r)builtin] )); then
+  pre=builtin
+else
+  # Neither builtin nor command-forcing precommand specified,
+  # so no prefix is needed
+  pre=
 fi
 
-if [[ $+opts[-b] -eq 1 && -n $builtins[$opts[-c]] ]]; then
-  _cmd_variant[$opts[-c]]=$opts[-b]
+if [[ $pre != builtin ]] && (( $+_cmd_variant[$opts[-c]] )); then
   (( $+opts[-r] )) && eval "${opts[-r]}=${_cmd_variant[$opts[-c]]}"
+  [[ $_cmd_variant[$opts[-c]] = "$1" ]] && return 1
   return 0
 fi
 
-output="$(_call_program variant $opts[-c] "${@[2,-1]}" </dev/null 2>&1)"
+output="$(_call_program variant $pre $opts[-c] "${@[2,-1]}" </dev/null 2>&1)"
 
 for cmd pat in "$var[@]"; do
   if [[ $output = *$~pat* ]]; then
@@ -36,6 +46,6 @@ for cmd pat in "$var[@]"; do
 done
 
 (( $+opts[-r] )) && eval "${opts[-r]}=$1"
-_cmd_variant[$opts[-c]]="$1"
+[[ $pre != builtin ]] && _cmd_variant[$opts[-c]]="$1"
 
 return 1
diff --git a/Completion/Zsh/Command/_builtin b/Completion/Zsh/Command/_builtin
index 9fb6acf7b..a77af9879 100644
--- a/Completion/Zsh/Command/_builtin
+++ b/Completion/Zsh/Command/_builtin
@@ -1,5 +1,7 @@
 #compdef builtin
 
+precommands+=(builtin)
+
 if (( $CURRENT > 2 )); then
   shift words
   (( CURRENT -- ))
-- 
2.21.0