Re: [PATCH] fix option -A of _arguments

[Oliver Kiddle <opk@xxxxxxx>]

Jun T wrote:
> Consider the following example:
>
> % _cmd () { _arguments -A '-*' : -a -b '*: :_file' }
> % compdef _cmd cmd
> cmd -x -<TAB>
> No match for: `file'
>
> But zshcompsys(1) says:
>
> -A pat
>   Do not complete options after the first non-option argument on the line.
>   pat is a pattern matching all strings which are not to be taken as arguments.
>   For example, to make _arguments stop completing options after the first
>   normal argument, but ignoring all strings starting with a hyphen even if they
>   are not described by one of the optspecs, the form is `-A "-*"'.
>
> In the example above, -x should not be taken as a normal argument since it
> matches the pattern '-*' (although it is not in the optspecs), and the
> options -a and -b should still be offered here (if I understand the document
> correctly).

I agree with your understanding.

Experimenting with this, I found further issues with -A. I hope you
don't mind but the resulting patch to account for them also covers the
issue you describe but with a somewhat different fix. I've also added a
number of test cases.

In terms of behaviour of your example, I think -x should also not be
treated as the first normal argument. If you replace '*: :_file' with
'1: :_file' in your example, the -x should not exclude the first (1)
argument. It is generally best in completions if the behaviour
gracefully copes with options that it doesn't know about and the
interface was designed with a pattern that matches options (-*) not the
converse (^-*) so it is reasonable to expect that -x in your example is
a new option our completion doesn't know about rather than an unusual
normal argument. We can't know for sure but I think this is more useful
in practice.

> -		 (!napat || cur <= compcurrent || !pattry(napat, line))) {
> +		 (!napat || (notmatch = !pattry(napat, line)) ||
> +		  cur <= compcurrent)) {

This logic can be reorganised to keep the slow pattry() call
after the fast cur <= compcurrent test. If cur <= compcurrent then
we won't have done the pattry() so repeating the call won't result in it
being called twice.

However, after trying this with some further cases I think we can
further limit when we check the pattern. It doesn't make much sense to
compare it against the current (incomplete) word. Or subsequent words
once a word has failed to match. Or to words following -- with -S.
Similarly for duplicated -- words, only the first should count.

I also noticed the following odd behaviour:
_cmd() { _arguments -a -b -c -d '(-a)1:one' '(-b)2:two' '(-c)*:extra' }
cmd -<tab> x y z
one
option
-b  -c  -d

It actually calls ca_inactive to disable -a for each of the x, y and z
arguments.

Normally, exclusions only apply to later options. This is useful because
options can exclude each other mutually and one sided exclusions can
occur. Exclusions mostly aren't applied backwards from later words on
the command-line because it is tricky to implement and not always
desirable.

Finally, I think it is an error that inopt is initialised to 1 because
we're not completing options to an argument at the beginning. Starting
it at 0 still passes tests and cuts out some needless steps.

Oliver

diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index e08788e89..b311d6c5f 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -2031,9 +2031,9 @@ ca_parse_line(Cadef d, Cadef all, int multi, int first)
     state.def = state.ddef = NULL;
     state.curopt = state.dopt = NULL;
     state.argbeg = state.optbeg = state.nargbeg = state.restbeg = state.actopts =
-	state.nth = state.inopt = state.inarg = state.opt = state.arg = 1;
+	state.nth = state.inarg = state.opt = state.arg = 1;
     state.argend = argend = arrlen(compwords) - 1;
-    state.singles = state.oopt = 0;
+    state.inopt = state.singles = state.oopt = 0;
     state.curpos = compcurrent;
     state.args = znewlinklist();
     state.oargs = (LinkList *) zalloc(d->nopts * sizeof(LinkList));
@@ -2080,9 +2080,14 @@ ca_parse_line(Cadef d, Cadef all, int multi, int first)
         remnulargs(line);
         untokenize(line);
 
-	ca_inactive(d, argxor, cur - 1, 0);
-	if ((d->flags & CDF_SEP) && cur != compcurrent && !strcmp(line, "--")) {
+	if (argxor) {
+	    ca_inactive(d, argxor, cur - 1, 0);
+	    argxor = NULL;
+	}
+	if ((d->flags & CDF_SEP) && cur != compcurrent && state.actopts &&
+		!strcmp(line, "--")) {
 	    ca_inactive(d, NULL, cur, 1);
+	    state.actopts = 0;
 	    continue;
 	}
 
@@ -2235,11 +2240,17 @@ ca_parse_line(Cadef d, Cadef all, int multi, int first)
 	} else if (multi && (*line == '-' || *line == '+') && cur != compcurrent
 		&& (ca_foreign_opt(d, all, line)))
 	    return 1;
-	else if (state.arg &&
-		 (!napat || cur <= compcurrent || !pattry(napat, line))) {
+	else if (state.arg && cur <= compcurrent) {
 	    /* Otherwise it's a normal argument. */
-	    if (napat && cur <= compcurrent)
+
+	    /* test pattern passed to -A. if it matches treat this as an unknown
+	     * option and continue to the next word */
+	    if (napat && cur < compcurrent && state.actopts) {
+		if (pattry(napat, line))
+		    continue;
 		ca_inactive(d, NULL, cur + 1, 1);
+		state.actopts = 0;
+	    }
 
 	    arglast = 1;
 	    /* if this is the first normal arg after an option, may have been
@@ -2293,7 +2304,7 @@ ca_parse_line(Cadef d, Cadef all, int multi, int first)
             if (adef)
                 state.oopt = adef->num - state.nth;
 
-	    if (state.def)
+	    if (state.def && cur != compcurrent)
 		argxor = state.def->xor;
 
 	    if (state.def && state.def->type != CAA_NORMAL &&
diff --git a/Test/Y03arguments.ztst b/Test/Y03arguments.ztst
index bf41aead5..ef7a0ec56 100644
--- a/Test/Y03arguments.ztst
+++ b/Test/Y03arguments.ztst
@@ -359,6 +359,12 @@
 0:allowed option before --
 >line: {tst -x }{ --}
 
+ tst_arguments -S '1:one' '2:two'
+ comptest $'tst -- -- \t'
+0:only first of duplicate -- is ignored
+>line: {tst -- -- }{}
+>DESCRIPTION:{two}
+
  tst_arguments -x :word
  comptest $'tst word -\t'
 0:option after a word
@@ -390,6 +396,25 @@
 0:continue completion after rest argument that looks like an option
 >line: {tst -a -x more }{}
 
+ tst_arguments -A '-*' -a -b '*: :(words)'
+ comptest $'tst -x -\t'
+0:word matching -A pattern doesn't exclude options
+>line: {tst -x -}{}
+>DESCRIPTION:{option}
+>NO:{-a}
+>NO:{-b}
+
+ tst_arguments -A '-*' -a -b '1:word:(word)'
+ comptest $'tst -x \t'
+0:unrecognised word matching -A pattern not treated as a rest argument
+>line: {tst -x word }{}
+
+ tst_arguments -A "-*" '(3)-a' '1:one' '2:two' '3:three' '4:four' '*:extra'
+ comptest $'tst x -a \t'
+0:exclusion from option following word matching -A pattern should not apply
+>line: {tst x -a }{}
+>DESCRIPTION:{three}
+
  tst_arguments '*-v'
  comptest $'tst -v -\t'
 0:repeatable options
@@ -478,6 +503,16 @@
 >NO:{-b}
 >NO:{-v}
 
+ tst_arguments -a -b -c '(-a)1:one' '(-b)2:two' '(-c)*:extra'
+ comptest $'tst  x y z\e6\C-b-\t'
+0:exclude option from normal argument to the right of the cursor
+>line: {tst -}{ x y z}
+>DESCRIPTION:{one}
+>DESCRIPTION:{option}
+>NO:{-a}
+>NO:{-b}
+>NO:{-c}
+
  tst_arguments -a - set1 -d - set2 '(set2)-m' -n -o ':arg:(x)' - set2 -x
  comptest $'tst -m \t'
 0:exclude own set from an option