[PATCH] Improved _shadow / _unshadow

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Tue, Jul 4, 2023 at 12:06 AM Marlon Richert <marlon.richert@xxxxxxxxx> wrote:
>
> Perhaps then at least add documentation for _shadow to compsys.yo?

I did that the other day (7/26, ChangeLog "unposted (cf. 51899):
Doc/Zsh/compsys.yo: document _shadow") and after reading my own
documentation, decided I didn't like what it said.

Consequently I've produced the attached patch.  The most significant
visible change is that _unshadow no longer requires any arguments (and
ignores any that it gets), instead it just unwinds anything done by
the most recent previous call to _shadow.  This includes a special
case for more than one call to _shadow with the same -s suffix
argument, removing my objection to Marlon's _approximate patch in
workers/51861.

On Thu, Jun 15, 2023 at 7:29 AM Marlon Richert <marlon.richert@xxxxxxxxx> wrote:
>
> I would find _shadow useful in my own Zsh code, too, not just completions. How
> about moving it from Completion/ to Functions/?

To address this, this patch adds Functions/Misc/mkshadow, which
defines wrappers around _shadow and _unshadow.  Although I didn't
explicitly mention the reasoning in the doc, these are "nofork-ready"
in that they return the suffix in $REPLY, so once those patches are
applied one can do
  local suffix=${| mkshadow ... }
This is actually implemented via _shadow except that it won't clobber
an existing $REPLY whereas mkshadow unsets REPLY on the way in.

Other changes under the hood:
* Use namespaces for the global variables used to track state
* Keep track of whether the shadowed command is a function, builtin,
or external, so that the placeholder "command@suffix" function is
consistently created/removed
* Handle many pathological cases such as wanting to shadow the "-"
precommand modifier (although there's still no way to fully duplicate
the precommand behavior)

And, update the doc for all of this.

diff --git a/Completion/Base/Utility/_shadow b/Completion/Base/Utility/_shadow
index 5b0f79c36..b5a8acb24 100644
--- a/Completion/Base/Utility/_shadow
+++ b/Completion/Base/Utility/_shadow
@@ -8,7 +8,7 @@
 #    }
 #    # Invoke callers of fname
 #  } always {
-#    _unshadow fname
+#    _unshadow
 #  }
 ## Alternate usage:
 # {
@@ -19,7 +19,7 @@
 #   }
 #   # Invoke callers of fname
 # } always {
-#   _unshadow -s suffix fname
+#   _unshadow
 # }
 ##
 
@@ -33,36 +33,62 @@ zmodload zsh/parameter # Or what?
 # This probably never comes up, but protect ourself from recursive call
 # chains that may duplicate the top elements of $funcstack by creating
 # a counter of _shadow calls and using it to make shadow names unique.
-typeset -gHi _shadowdepth=0
+builtin typeset -gHi .shadow.depth=0
+builtin typeset -gHa .shadow.stack
 
 # Create a copy of each fname so that a caller may redefine
 _shadow() {
-  local -A fsfx=( -s ${funcstack[2]}:${functrace[2]}:$((_shadowdepth+1)) )
-  local fname
+  emulate -L zsh
+  local -A fsfx=( -s ${funcstack[2]}:${functrace[2]}:$((.shadow.depth+1)) )
+  local fname shadowname
+  local -a fnames
   zparseopts -K -A fsfx -D s:
   for fname; do
-    local shadowname=${fname}@${fsfx[-s]}
-    (( ${+functions[$fname]} )) &&
-      builtin functions -c $fname $shadowname
+    shadowname=${fname}@${fsfx[-s]}
+    if (( ${+functions[$shadowname]} ))
+    then
+      # Called again with the same -s, just ignore it
+      continue
+    elif (( ${+functions[$fname]} ))
+    then
+      builtin functions -c -- $fname $shadowname
+      fnames+=(f@$fname)
+    elif (( ${+builtins[$fname]} ))
+    then
+      eval "function -- $shadowname { builtin $fname \"\$@\" }"
+      fnames+=(b@$fname)
+    else
+      eval "function -- $shadowname { command $fname \"\$@\" }"
+      fnames+=(c@$fname)
+    fi
   done
-  ((_shadowdepth++))
+  [[ -z $REPLY ]] && REPLY=${fsfx[-s]}
+  builtin set -A .shadow.stack ${fsfx[-s]} $fnames -- ${.shadow.stack}
+  ((.shadow.depth++))
 }
 
 # Remove the redefined function and shadowing name
 _unshadow() {
-  local -A fsfx=( -s ${funcstack[2]}:${functrace[2]}:${_shadowdepth} )
-  local fname
-  zparseopts -K -A fsfx -D s:
-  for fname; do
-    local shadowname=${fname}@${fsfx[-s]}
-    if (( ${+functions[$shadowname]} )); then
-      builtin functions -c $shadowname $fname
-      builtin unfunction $shadowname
-    elif (( ${+functions[$fname]} )); then
-      builtin unfunction $fname
+  emulate -L zsh
+  local fname shadowname fsfx=${.shadow.stack[1]}
+  local -a fnames
+  [[ -n $fsfx ]] || return 1
+  shift .shadow.stack
+  while [[ ${.shadow.stack[1]?no shadows} != -- ]]; do
+    fname=${.shadow.stack[1]#?@}
+    shadowname=${fname}@${fsfx}
+    if (( ${+functions[$fname]} )); then
+      builtin unfunction -- $fname
     fi
+    case ${.shadow.stack[1]} in
+      (f@*) builtin functions -c -- $shadowname $fname ;&
+      ([bc]@*) builtin unfunction -- $shadowname ;;
+    esac
+    shift .shadow.stack
   done
-  ((_shadowdepth--))
+  [[ -z $REPLY ]] && REPLY=$fsfx
+  shift .shadow.stack
+  ((.shadow.depth--))
 }
 
 # This is tricky.  When we call _shadow recursively from autoload,
diff --git a/Doc/Zsh/compsys.yo b/Doc/Zsh/compsys.yo
index 33baeab49..3f708eb5a 100644
--- a/Doc/Zsh/compsys.yo
+++ b/Doc/Zsh/compsys.yo
@@ -5229,13 +5229,12 @@ and hence is not normally called explicitly.
 )
 findex(_shadow)
 findex(_unshadow)
-xitem(tt(_shadow) [ tt(-s) var(suffix) ] var(command_name) ...)
-item(tt(_unshadow) [ tt(-s) var(suffix) ] var(command_name) ...)(
+xitem(tt(_shadow) [ tt(-s) var(suffix) ] [ -- ] var(command_name) ...)
+item(tt(_unshadow))(
 The tt(_shadow) function creates a copy of each of the shell functions
 in the var(command_name) arguments.  The original functions can then
-be replaced by new implementations.  A later call to tt(_unshadow),
-with the same var(command_name) list, removes the new implementations,
-if any, and restores the originals.
+be replaced by new implementations.  A later call to tt(_unshadow)
+removes the new implementations, if any, and restores the originals.
 
 Recommended usage is to pair tt(_shadow) and tt(_unshadow) calls by
 use of an `tt(always)' block:
@@ -5246,30 +5245,38 @@ example({
   }
   # Invoke callers of fname
 } always {
-  _unshadow fname
+  _unshadow
 })
 
-Any var(command_name) may instead be a builtin, but in that case no
-copy is created.  The expectation is that an initial tt(_shadow) is
-followed by creating a wrapper function, and therafter any nested or
-recursive calls thus copy and replace the wrapper function.
+The var(suffix), if supplied, is prepended by an `tt(@)' character and
+then appended to each var(command_name) to create the copy.  Thus
+example(_shadow -s XX foo)
+creates a function named `tt(foo@XX)'.  This provides a well-known
+name for the original implementation if the new implementation needs
+to call it as a wrapper.  If a nested call to  tt(_shadow) uses the
+same var(suffix), em(no new copy is made).  The presumption thus is
+that suffixes and new implementations correspond one to one.
+
+If var(command_name) is a builtin or external command, and there has been
+no preceding tt(_shadow) replacement made, the function so created calls
+the shadowed name prefixed by the tt(builtin) or tt(command) keywords as
+appropriate.
 example({
-  _shadow compadd
-  compadd LPAR()RPAR() { builtin compadd -O tmparr "$@" }
+  _shadow -s wrap compadd
+  compadd LPAR()RPAR() {
+    # compadd@wrap runs builtin compadd
+    compadd@wrap -O tmparr "$@" }
 } always {
-  _unshadow compadd
+  _unshadow
 })
 
-The var(suffix), if supplied, is prepended by an `tt(@)' character and
-then appended to each var(command_name) to create the copy.  Thus
-example(_shadow -s XX foo)
-creates a function named `tt(foo@XX)' (unless `tt(foo)' is a builtin).
-Note that a nested call to tt(_shadow) with the same var(suffix) may
-result in name collisions and unexpected results, but this provides a
-well-known name for the original function if the new implementation
-needs to call it as a wrapper.  The same var(suffix) must be used in
-the call to tt(_unshadow).  When no var(suffix) is present,
-tt(_shadow) creates a unique suffix to avoid name collisions.
+When no var(suffix) argument is present, tt(_shadow) creates a unique
+suffix to avoid name collisions.
+
+Arguments of tt(_unshadow) are ignored.  Every listed var(command_name)
+for the most recent call to tt(_shadow) is removed.  This differs from
+an early implementation that required tt(_unshadow) to receive the
+same var(suffix) and var(command_name) list as tt(_shadow).
 )
 findex(_store_cache)
 item(tt(_store_cache) var(cache_identifier) var(param) ...)(
diff --git a/Doc/Zsh/contrib.yo b/Doc/Zsh/contrib.yo
index 96de5aa9b..ef11d77ad 100644
--- a/Doc/Zsh/contrib.yo
+++ b/Doc/Zsh/contrib.yo
@@ -4336,6 +4336,27 @@ example(is-at-least 3.1.6-15 && setopt NO_GLOBAL_RCS
 is-at-least 3.1.0 && setopt HIST_REDUCE_BLANKS
 is-at-least 2.6-17 || print "You can't use is-at-least here.")
 )
+findex(mkshadow)
+findex(rmshadow)
+xitem(tt(mkshadow) [ tt(-s) var(suffix) ] [ -- ] var(command_name) ...)
+item(tt(rmshadow))(
+These functions are an interface to the tt(_shadow) and tt(_unshadow)
+completion utilities to make them more easily accessible in other
+contexts.  Usage is exactly as for the completion utility:
+example({
+  mkshadow fname
+  function fname {
+    # Do your new thing
+  }
+  # Invoke callers of fname
+} always {
+  rmshadow
+})
+
+Upon return, the value of tt($REPLY) is the suffix used to create a
+copy of the original var(command_name), so var(command_name)tt(@$REPLY)
+invokes that original.
+)
 findex(nslookup)
 item(tt(nslookup) [ var(arg) ... ])(
 This wrapper function for the tt(nslookup) command requires the