[PATCH] handle options of _arguments correctly

[PATCH] handle options of _arguments correctly

[Jun T.]

Among the options of _arguments, -S, -A and -M are not handled
by the while loop at lines 295-306 of _arguments. This causes
the following problem:

% compdef '_arguments -S -C : -a' foo
% foo -<TAB>

this offers -C in addition to -a.
The reason is that, in _arguments, since -S is not handled by the while
loop, both -S and -C remain in "$@" and passed to

   comparguments -i "$autod" "$singopt[@]" "$@"   (line 321)

Then "comparguments -i" interprets -S as an option to _arguments,
but treats -C as an optspec (i.e., as an option of foo). Moreover,
$usecc is not set to "yes" and -C does not have the expeced effect.
So we need to handle -S -A and -M in the while loop and move them
to $singopt (so that $usecc is set to yes and -C is removed from "$@").

But even with this change, there is another problem:

% bar() { echo '\t-a\n\t-s' }
% compdef '_arguments --' bar
% bar -<TAB>

this correctly offers -a and -s. But

% unset _args_cache_bar              # or restart another zsh
% compdef '_arguments -s --' bar     # or '_arguments -s : --'
% bar -<TAB>

now -s is not offered.
This is because, at line 18 of _arguments, $tmpargv is set to '-s',
which is used at line 144 to remove -s from $lopts; i.e., -s is
considered as an optspec, not an option to _arguments itself.
A simple fix would be to move the while loop (lines 295-306) to the top
of _arguments.

This leads to the following patch. Are there any unexpected side
effects of this?


diff --git a/Completion/Base/Utility/_arguments b/Completion/Base/Utility/_arguments
index 1f35e8d..551587b 100644
--- a/Completion/Base/Utility/_arguments
+++ b/Completion/Base/Utility/_arguments
@@ -8,6 +8,25 @@ local oldcontext="$curcontext" hasopts rawret optarg singopt alwopt
 local setnormarg start rest
 local -a match mbegin mend
 
+subopts=()
+singopt=()
+while [[ "$1" = -([AMO]*|[CRSWnsw]) ]]; do
+  case "$1" in
+  -C)  usecc=yes; shift ;;
+  -O)  subopts=( "${(@P)2}" ); shift 2 ;;
+  -O*) subopts=( "${(@P)${1[3,-1]}}" ); shift ;;
+  -R)  rawret=yes; shift;;
+  -n)  setnormarg=yes; NORMARG=-1; shift;;
+  -w)  optarg=yes; shift;;
+  -W)  alwopt=arg; shift;;
+  -[Ss])  singopt+=( $1 ); shift;;
+  -[AM])  singopt+=( $1 $2 ); shift 2 ;;
+  -[AM]*) singopt+=( $1 ); shift ;;
+  esac
+done
+
+[[ "$PREFIX" = [-+] ]] && alwopt=arg
+
 long=$argv[(I)--]
 if (( long )); then
   local name tmp tmpargv
@@ -290,23 +309,6 @@ if (( long )); then
   set -- "$tmpargv[@]" "${(@P)name}"
 fi
 
-subopts=()
-singopt=()
-while [[ "$1" = -(O*|[CRWnsw]) ]]; do
-  case "$1" in
-  -C)  usecc=yes; shift ;;
-  -O)  subopts=( "${(@P)2}" ); shift 2 ;;
-  -O*) subopts=( "${(@P)${1[3,-1]}}" ); shift ;;
-  -R)  rawret=yes; shift;;
-  -n)  setnormarg=yes; NORMARG=-1; shift;;
-  -w)  optarg=yes; shift;;
-  -s)  singopt=(-s); shift;;
-  -W)  alwopt=arg; shift;;
-  esac
-done
-
-[[ "$PREFIX" = [-+] ]] && alwopt=arg
-
 zstyle -s ":completion:${curcontext}:options" auto-description autod
 
 if (( $# )) && comparguments -i "$autod" "$singopt[@]" "$@"; then

Re: [PATCH] handle options of _arguments correctly

[Bart Schaefer]

On Sep 29,  1:55am, Jun T. wrote:
}
} This is because, at line 18 of _arguments, $tmpargv is set to '-s',
} which is used at line 144 to remove -s from $lopts; i.e., -s is
} considered as an optspec, not an option to _arguments itself.
} A simple fix would be to move the while loop (lines 295-306) to the top
} of _arguments.
} 
} This leads to the following patch. Are there any unexpected side
} effects of this?

I don't see anything immediate (though this renders the name "singsub"
for the array somewhat nonsensical).

The stuff on line 144 came from this change:

+2000-08-02  Sven Wischnowsky  <wischnow@zsh.org>
+
+       * 12475: Completion/Base/_arguments: prefer user-defined specs
+       over ones derived from --help output
+       

I'm guessing that code should have looked for a ":" in $tmpargv and only
treat as specs, those positions that follow either the ":" or that follow
any option letter that can't appear to the left of a ":".  I haven't run
it down to this level, but it's possible that code pre-dates _arguments
having any switches of its own.

E.g.

    _arguments -s -w -q -s --
and
    _arguments -s -w : -q -s --

are equivalent becaue of the -q (even though I'd hope no one wrote the
former in an actual completion function).  Which I think is what your
patch accomplishes (the -s and -w would be gobbled up and then the loop
ends at either the ":" or the -q whichever comes first).

Re: [PATCH] handle options of _arguments correctly

[Jun T.]

2015/09/29 08:48, Bart Schaefer <schaefer@brasslantern.com> wrote:

> I don't see anything immediate (though this renders the name "singsub"
> for the array somewhat nonsensical).

Do you mean 'singopt'? I guess it means 'single-letter option'.
It may not be a good name because all the options of _arugments are
single-letter. It only saves the options -s -S -A -M which need be
passed to 'comparguments -i' (line 2499 of Zle/computil.c).

>    _arguments -s -w -q -s --
> and
>    _arguments -s -w : -q -s --
> 
> are equivalent becaue of the -q (even though I'd hope no one wrote the
> former in an actual completion function).  Which I think is what your
> patch accomplishes

Yes, but it was not sufficient.

% bar() { echo '\t-S\n\t-a' }
% compdef '_arguments --' bar
% bar -<TAB>

this only offers -a. '_arguments : --' will offer both -S and -a.

When 'comparguments -i' is called, $singopt is empty and "$@" is "-S" "-a".
But comparguments considers the first -S as an options to _arguments
(line 1203 and below in computil.c).

I think we need explicitly pass ':' to comparguments.

The following is a patch against the current master and includes my previous
one. I also dropped the "if [[ long -eq 1 ]]; then ...", because this always
evaluates to false (it should have been $long, but anyway $argv[1,0] expands
to null string and long==1 need not be treated specially).


diff --git a/Completion/Base/Utility/_arguments b/Completion/Base/Utility/_arguments
index 1f35e8d..95ef621 100644
--- a/Completion/Base/Utility/_arguments
+++ b/Completion/Base/Utility/_arguments
@@ -8,15 +8,33 @@ local oldcontext="$curcontext" hasopts rawret optarg singopt alwopt
 local setnormarg start rest
 local -a match mbegin mend
 
+subopts=()
+singopt=()
+while [[ "$1" = -([AMO]*|[CRSWnsw]) ]]; do
+  case "$1" in
+  -C)  usecc=yes; shift ;;
+  -O)  subopts=( "${(@P)2}" ); shift 2 ;;
+  -O*) subopts=( "${(@P)${1[3,-1]}}" ); shift ;;
+  -R)  rawret=yes; shift;;
+  -n)  setnormarg=yes; NORMARG=-1; shift;;
+  -w)  optarg=yes; shift;;
+  -W)  alwopt=arg; shift;;
+  -[Ss])  singopt+=( $1 ); shift;;
+  -[AM])  singopt+=( $1 $2 ); shift 2 ;;
+  -[AM]*) singopt+=( $1 ); shift ;;
+  esac
+done
+
+[[ $1 == ':' ]] && shift
+singopt+=( ':' )  # always end with ':' to indicate the end of options
+
+[[ "$PREFIX" = [-+] ]] && alwopt=arg
+
 long=$argv[(I)--]
 if (( long )); then
   local name tmp tmpargv
 
-  if [[ long -eq 1 ]]; then
-    tmpargv=()
-  else
-    tmpargv=( "${(@)argv[1,long-1]}" )
-  fi
+  tmpargv=( "${(@)argv[1,long-1]}" )  # optspec's before --, if any
 
   name=${~words[1]}
   [[ "$name" = [^/]*/* ]] && name="$PWD/$name"
@@ -290,23 +308,6 @@ if (( long )); then
   set -- "$tmpargv[@]" "${(@P)name}"
 fi
 
-subopts=()
-singopt=()
-while [[ "$1" = -(O*|[CRWnsw]) ]]; do
-  case "$1" in
-  -C)  usecc=yes; shift ;;
-  -O)  subopts=( "${(@P)2}" ); shift 2 ;;
-  -O*) subopts=( "${(@P)${1[3,-1]}}" ); shift ;;
-  -R)  rawret=yes; shift;;
-  -n)  setnormarg=yes; NORMARG=-1; shift;;
-  -w)  optarg=yes; shift;;
-  -s)  singopt=(-s); shift;;
-  -W)  alwopt=arg; shift;;
-  esac
-done
-
-[[ "$PREFIX" = [-+] ]] && alwopt=arg
-
 zstyle -s ":completion:${curcontext}:options" auto-description autod
 
 if (( $# )) && comparguments -i "$autod" "$singopt[@]" "$@"; then

Re: [PATCH] handle options of _arguments correctly

[Bart Schaefer]

On Sep 30,  1:12am, Jun T. wrote:
} Subject: Re: [PATCH] handle options of _arguments correctly
} 
} 2015/09/29 08:48, Bart Schaefer <schaefer@brasslantern.com> wrote:
} 
} > I don't see anything immediate (though this renders the name "singsub"
} > for the array somewhat nonsensical).
} 
} Do you mean 'singopt'? I guess it means 'single-letter option'.

Yes (subst.c on the brain), and yes, which seems specifically to mean
the "-s" switch ... but not very important.

} I think we need explicitly pass ':' to comparguments.

I was actually going to suggest that, and then stopped short because I
wasn't convinced it was necessary.  I think it's a good idea.