[PATCH] _pick_variant: Update builtin check

[PATCH] _pick_variant: Update builtin check

[Matthew Martin]

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'll split out the _builtin change before commit. Wasn't large enough to
get its own mail but necessary if anyone is testing.

- Matthew Martin

---
 Completion/Base/Utility/_pick_variant | 15 +++++++++------
 Completion/Zsh/Command/_builtin       |  2 ++
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/Completion/Base/Utility/_pick_variant b/Completion/Base/Utility/_pick_variant
index 9099e3599..f4ceb7462 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 -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,15 +15,16 @@ 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 (( $+opts[-b] && ! ${#precommands:|builtin_precommands} &&
+    ( $+builtins[$opts[-c]] || $precommands[(I)builtin] ) )); then
+  (( $+opts[-r] )) && eval "${opts[-r]}=$opts[-b]"
   return 0
 fi
 
-if [[ $+opts[-b] -eq 1 && -n $builtins[$opts[-c]] ]]; then
-  _cmd_variant[$opts[-c]]=$opts[-b]
+if (( $+_cmd_variant[$opts[-c]] )); then
   (( $+opts[-r] )) && eval "${opts[-r]}=${_cmd_variant[$opts[-c]]}"
+  [[ $_cmd_variant[$opts[-c]] = "$1" ]] && return 1
   return 0
 fi
 
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

Re: [PATCH] _pick_variant: Update builtin check

[Matthew Martin]

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).

Sorry for the noise.

Re: [PATCH] _pick_variant: Update builtin check

[Matthew Martin]

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

Re: [PATCH] _pick_variant: Update builtin check

[Daniel Shahaf]

Matthew Martin wrote on Wed, Mar 20, 2019 at 07:52:39 -0500:
> 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]])
⋮
> +++ 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)

May I suggest a comment here documenting the semantics of this variable?
For example, why doesn't it list the 'command' precommand (presumably
becaus that one doesn't preserve builtins, but this info should be in
the comment, not in the list archives)?

(And since I'm replying already, style nit: the array could be declared
readonly.)

>  (( $+_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]"

Should that be «"${opts[-r]}=${(q)opts[-b]}"» with quoting to counter
the eval?  (Also with the preëxisting assignments-in-eval in other lines)

>    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

Cheers,

Daniel

Re: [PATCH] _pick_variant: Update builtin check

[Matthew Martin]

On Wed, Mar 20, 2019 at 01:05:23PM +0000, Daniel Shahaf wrote:
> Matthew Martin wrote on Wed, Mar 20, 2019 at 07:52:39 -0500:
> > 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]])
> ⋮
> > +++ 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)
> 
> May I suggest a comment here documenting the semantics of this variable?
> For example, why doesn't it list the 'command' precommand (presumably
> becaus that one doesn't preserve builtins, but this info should be in
> the comment, not in the list archives)?

How's
+# Precommands which allow the command to be builtin (unlike command and sudo).
+local -ar builtin_precommands=(- builtin eval exec nocorrect noglob time)

Which of course raises the point that I need to add precommands+=(sudo)
to _sudo (and most users of _normal).

> (And since I'm replying already, style nit: the array could be declared
> readonly.)

Indeed; fixed.

> >  (( $+_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]"
> 
> Should that be «"${opts[-r]}=${(q)opts[-b]}"» with quoting to counter
> the eval?  (Also with the preëxisting assignments-in-eval in other lines)

I was planning on switching them all to ${(P)opts[-r]::=...} next.

Thanks for the review!

Re: [PATCH] _pick_variant: Update builtin check

[Daniel Shahaf]

Matthew Martin wrote on Thu, 21 Mar 2019 00:17 +00:00:
> On Wed, Mar 20, 2019 at 01:05:23PM +0000, Daniel Shahaf wrote:
> > Matthew Martin wrote on Wed, Mar 20, 2019 at 07:52:39 -0500:
> > > +builtin_precommands=(- builtin eval exec nocorrect noglob time)
> > 
> > May I suggest a comment here documenting the semantics of this variable?
> > For example, why doesn't it list the 'command' precommand (presumably
> > becaus that one doesn't preserve builtins, but this info should be in
> > the comment, not in the list archives)?
> 
> How's
> +# Precommands which allow the command to be builtin (unlike command and sudo).
> +local -ar builtin_precommands=(- builtin eval exec nocorrect noglob time)

I understand what you mean in the context of this thread, but I think
the two uses of the term 'command' might be confusing to someone who
opens the file in a year or three.  How about:

# Precommands which allow their wrapped command to be a builtin.
# All of these are necessarily builtins or reserved words themselves, but not all builtin precommands are listed here: for one, the 'command' builtin is excluded.

Cheers,

Daniel

Re: [PATCH] _pick_variant: Update builtin check

[Matthew Martin]

On Thu, Mar 21, 2019 at 07:57:38AM -0400, Daniel Shahaf wrote:
> I understand what you mean in the context of this thread, but I think
> the two uses of the term 'command' might be confusing to someone who
> opens the file in a year or three.  How about:
> 
> # Precommands which allow their wrapped command to be a builtin.
> # All of these are necessarily builtins or reserved words themselves, but not all builtin precommands are listed here: for one, the 'command' builtin is excluded.

Works for me. Unless there are other comments I'll commit later today.

- Matthew Martin

From 06741ae0671c4a29d2700afa46470a8024ddbefb Mon Sep 17 00:00:00 2001
From: Matthew Martin <phy1729@gmail.com>
Date: Tue, 19 Mar 2019 20:42:35 -0500
Subject: [PATCH] _pick_variant: Update builtin check

---
 Completion/Base/Utility/_pick_variant | 30 ++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/Completion/Base/Utility/_pick_variant b/Completion/Base/Utility/_pick_variant
index 9099e3599..872e8f583 100644
--- a/Completion/Base/Utility/_pick_variant
+++ b/Completion/Base/Utility/_pick_variant
@@ -1,9 +1,15 @@
 #autoload
 
-local output cmd pat
+local output cmd pat pre
 local -a var
 local -A opts
 
+# Precommands which allow their wrapped command to be a builtin.
+# All of these are necessarily builtins or reserved words themselves,
+# but not all builtin precommands are listed here:
+# for one, the 'command' builtin is excluded.
+local -ar builtin_precommands=(- builtin eval exec nocorrect noglob time)
+
 (( $+_cmd_variant )) || typeset -gA _cmd_variant
 
 zparseopts -D -A opts b: c: r:
@@ -13,19 +19,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[(I)builtin] || $+builtins[$opts[-c]] ) )); then
+  (( $+opts[-r] )) && eval "${opts[-r]}=$opts[-b]"
   return 0
+elif (( $precommands[(I)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 +50,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
-- 
2.21.0