[PATCH] zparseopts: use standard option parsing

[dana <dana@xxxxxxx>]

(wasn't going to post this for 5.10 but i was encouraged to do so)

in workers/37802 bart added this to the zparseopts docs:

> The options of zparseopts itself cannot be stacked because, for
> example, the stack ‘-DEK’ is indistinguishable from a spec for the
> GNU-style long option ‘--DEK’.

respectfully i've never really bought that argument. -D by itself is
also indistinguishable from a spec for --D. currently zparseopts deals
with that by simply prioritising its own options over specs. if users
fail to guard their long-option specs with -/--, as zparseopts learns
more options for itself, there's the potential that they gradually
'consume' those specs. since i've taken an interest in it over the last
few years, that's become a real possibility

imo zparseopts should support option stacking and it should require
long-option specs to be guarded. we can have it do that just by using
the normal option-parsing facilities. this also makes the code quite a
bit simpler

obv the concern with this change is that some calls to zparseopts with
unguarded specs which were previously valid will now fail. there is
exactly one instance of this in the distribution (and it's my fault),
which i will fix. as far as user scripts, it does worry me slightly, but
as mentioned this was always a bad idea and it's only getting worse, so
unless we want to stop adding features to zparseopts i feel like we just
need to do it

as i hinted at in workers/54362, the `zparseopts -D -F -` trick will no
longer work after this change. but i doubt anyone was using that

dana



diff --git a/Doc/Zsh/mod_zutil.yo b/Doc/Zsh/mod_zutil.yo
index a18985a4b..14bf0ee2d 100644
--- a/Doc/Zsh/mod_zutil.yo
+++ b/Doc/Zsh/mod_zutil.yo
@@ -230,7 +230,7 @@ item(tt(zregexparse))(
 This implements some internals of the tt(_regex_arguments) function.
 )
 findex(zparseopts)
-item(tt(zparseopts) [ tt(-D) tt(-E) tt(-F) tt(-G) tt(-K) tt(-M) ] [ tt(-a) var(array) ] [ tt(-A) var(assoc) ] [ tt(-n) var(name) ] [ tt(-v) var(argv) ] [ tt(-) ] var(spec) ...)(
+item(tt(zparseopts) [ tt(-DEFGKM) ] [ tt(-a) var(array) ] [ tt(-A) var(assoc) ] [ tt(-n) var(name) ] [ tt(-v) var(argv) ] [ tt(-) ] var(spec) ...)(
 This builtin simplifies the parsing of options in positional parameters,
 i.e. the set of arguments given by tt($*).  Each var(spec) describes one
 option and must be of the form `var(opt)[tt(=)var(array)]'.  If an option
@@ -241,7 +241,7 @@ should be declared as a normal array and never as an associative array.
 
 At least one var(spec) must be given.  If a single spec is given which
 is either an empty string or a single hyphen (as in
-tt(zparseopts -D -F '')), it signifies that no options are recognised.
+tt(zparseopts -DF '')), it signifies that no options are recognised.
 (This is useful for future-proofing functions that might gain options
 later.)  Otherwise, it is an error to give any var(spec) without an
 `tt(=)var(array)' unless one of the tt(-a) or tt(-A) options is used.
@@ -302,14 +302,9 @@ ambiguities may arise when at least one overlapping var(spec) takes an
 argument, as in `tt(-foo: -foobar)'. In that case, the last matching
 var(spec) wins. (This is not an issue with GNU-style parsing.)
 
-The options of tt(zparseopts) itself cannot be stacked because, for
-example, the stack `tt(-DEK)' is indistinguishable from a var(spec) for
-the GNU-style long option `tt(-)tt(-DEK)'.
-
-Nevertheless, it is strongly recommended to distinguish GNU-style long
-option var(spec)s from tt(zparseopts)'s own options by preceding them by
-a `tt(-)', `tt(--)', or short option tt(spec), as in
-tt(zparseopts -a opts - -foo).  In the future this may become mandatory.
+In order to distinguish GNU-style long option var(spec)s from
+tt(zparseopts)'s own options, they em(must) be preceded by a `tt(-)',
+`tt(--)', or short option tt(spec), as in tt(zparseopts -Ga opts - -foo).
 
 The options of tt(zparseopts) itself are:
 
@@ -449,4 +444,12 @@ to have the effect of
 example(foo=(-a)
 bar=(-a '' -b xyz))
 )
+
+The following is a useful general-purpose pattern:
+
+example(local -A opts
+zparseopts -DFGA opts - x -long: || return
+(( $+opts[-x] )) && ... # handle -x
+(( $+opts[--long] )) && ... # handle --long, optarg in $opts[--long]
+)
 enditem()
diff --git a/Src/Modules/zutil.c b/Src/Modules/zutil.c
index f13ac95ac..f3cdaf3cd 100644
--- a/Src/Modules/zutil.c
+++ b/Src/Modules/zutil.c
@@ -1735,12 +1735,12 @@ zalloc_default_array(char ***aval, char *assoc, int keep, int num)
 }
 
 static int
-bin_zparseopts(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
+bin_zparseopts(char *nam, char **args, Options ops, UNUSED(int func))
 {
     char *o, *p, *n, **pp, **aval, **ap, *assoc = NULL, **cp, **np;
     char *paramsname = NULL, **params;
     char *progname = scriptname ? scriptname : (argzero ? argzero : nam);
-    int del = 0, flags = 0, extract = 0, fail = 0, gnu = 0, keep = 0;
+    int flags = 0, del, extract, fail, gnu, keep;
     Zoptdesc sopts[256], d;
     Zoptarr a, defarr = NULL;
     Zoptval v;
@@ -1749,144 +1749,57 @@ bin_zparseopts(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
     opt_arrs = NULL;
     memset(sopts, 0, 256 * sizeof(Zoptdesc));
 
-    while ((o = *args++)) {
-	if (*o == '-') {
-	    switch (o[1]) {
-	    case '\0':
-		o = NULL;
-		break;
-	    case '-':
-		if (o[2])
-		    args--;
-		/* else unreachable, default parsing removes "--" */
-		o = NULL;
-		break;
-	    case 'D':
-		if (o[2]) {
-		    args--;
-		    o = NULL;
-		    break;
-		}
-		del = 1;
-		break;
-	    case 'E':
-		if (o[2]) {
-		    args--;
-		    o = NULL;
-		    break;
-		}
-		extract = 1;
-		break;
-	    case 'F':
-		if (o[2]) {
-		    args--;
-		    o = NULL;
-		    break;
-		}
-		fail = 1;
-		break;
-	    case 'G':
-		if (o[2]) {
-		    args--;
-		    o = NULL;
-		    break;
-		}
-		gnu = 1;
-		break;
-	    case 'K':
-		if (o[2]) {
-		    args--;
-		    o = NULL;
-		    break;
-		}
-		keep = 1;
-		break;
-	    case 'M':
-		if (o[2]) {
-		    args--;
-		    o = NULL;
-		    break;
-		}
-		flags |= ZOF_MAP;
-		break;
-	    case 'n':
-		if (o[2])
-		    progname = o + 2;
-		else if (*args)
-		    progname = *args++;
-		else {
-		    zwarnnam(nam, "missing program name");
-		    return 1;
-		}
-		break;
-	    case 'a':
-		if (defarr) {
-		    zwarnnam(nam, "default array given more than once");
-		    return 1;
-		}
-		if (o[2])
-		    n = o + 2;
-		else if (*args)
-		    n = *args++;
-		else {
-		    zwarnnam(nam, "missing array name");
-		    return 1;
-		}
-		defarr = (Zoptarr) zhalloc(sizeof(*defarr));
-		defarr->name = n;
-		defarr->num = 0;
-		defarr->vals = defarr->last = NULL;
-		defarr->next = NULL;
-		opt_arrs = defarr;
-		break;
-	    case 'A':
-		if (assoc) {
-		    zwarnnam(nam, "associative array given more than once");
-		    return 1;
-		}
-		if (o[2]) 
-		    assoc = o + 2;
-		else if (*args)
-		    assoc = *args++;
-		else {
-		    zwarnnam(nam, "missing array name");
-		    return 1;
-		}
-		break;
-	    case 'v':
-		if (paramsname) {
-		    zwarnnam(nam, "argv array given more than once");
-		    return 1;
-		}
-		if (o[2])
-		    paramsname = o + 2;
-		else if (*args)
-		    paramsname = *args++;
-		else {
-		    zwarnnam(nam, "missing array name");
-		    return 1;
-		}
-		break;
-	    default:
-		/* Anything else is an option description */
-		args--;
-		o = NULL;
-		break;
-	    }
-	    if (!o) {
-		o = "";
-		break;
-	    }
-	} else {
-	    args--;
-	    break;
+    del = OPT_ISSET(ops, 'D');
+    extract = OPT_ISSET(ops, 'E');
+    fail = OPT_ISSET(ops, 'F');
+    gnu = OPT_ISSET(ops, 'G');
+    keep = OPT_ISSET(ops, 'K');
+    flags |= OPT_ISSET(ops, 'M') ? ZOF_MAP : 0;
+
+    if (OPT_ISSET(ops, 'a')) {
+	if (!*OPT_ARG(ops, 'a')) {
+	    zwarnnam(nam, "missing array name for -a");
+	    return 1;
 	}
+	defarr = (Zoptarr) zhalloc(sizeof(*defarr));
+	defarr->name = OPT_ARG(ops, 'a');
+	defarr->num = 0;
+	defarr->vals = defarr->last = NULL;
+	defarr->next = NULL;
+	opt_arrs = defarr;
+    }
+    if (OPT_ISSET(ops, 'A')) {
+	if (!*OPT_ARG(ops, 'A')) {
+	    zwarnnam(nam, "missing array name for -A");
+	    return 1;
+	}
+	assoc = OPT_ARG(ops, 'A');
+    }
+    if (OPT_ISSET(ops, 'n')) {
+	if (!*OPT_ARG(ops, 'n')) {
+	    zwarnnam(nam, "missing program name for -n");
+	    return 1;
+	}
+	progname = OPT_ARG(ops, 'n');
+    }
+    if (OPT_ISSET(ops, 'v')) {
+	if (!*OPT_ARG(ops, 'v')) {
+	    zwarnnam(nam, "missing array name for -v");
+	    return 1;
+	}
+	paramsname = OPT_ARG(ops, 'v');
+    }
+
+    params = getaparam((paramsname = paramsname ? paramsname : "argv"));
+    if (!params) {
+	zwarnnam(nam, "no such array: %s", paramsname);
+	return 1;
     }
 
     /* allow a single '' or - spec to signify no options recognised */
-    if (o && *args && !args[1] && (!**args || !strcmp(*args, "-"))) {
+    if (*args && !args[1] && (!**args || !strcmp(*args, "-"))) {
 	args++;
-    } else if (!o) {
+    } else if (!*args) {
 	zwarnnam(nam, "missing option descriptions");
 	return 1;
     }
@@ -1968,11 +1881,7 @@ bin_zparseopts(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
 	    return 1;
 	}
     }
-    params = getaparam((paramsname = paramsname ? paramsname : "argv"));
-    if (!params) {
-	zwarnnam(nam, "no such array: %s", paramsname);
-	return 1;
-    }
+
     np = cp = pp = ((extract && del) ? arrdup(params) : params);
     for (; (o = *pp); pp++) {
 	/* Not an option. With GNU style, this includes '-' */
@@ -2150,7 +2059,7 @@ bin_zparseopts(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
 
 static struct builtin bintab[] = {
     BUILTIN("zformat", 0, bin_zformat, 3, -1, 0, NULL, NULL),
-    BUILTIN("zparseopts", 0, bin_zparseopts, 1, -1, 0, NULL, NULL),
+    BUILTIN("zparseopts", 0, bin_zparseopts, 0, -1, 0, "a:A:DEFGKMn:v:", NULL),
     BUILTIN("zregexparse", 0, bin_zregexparse, 3, -1, 0, "c", NULL),
     BUILTIN("zstyle", 0, bin_zstyle, 0, -1, 0, NULL, NULL),
 };
diff --git a/Test/V12zparseopts.ztst b/Test/V12zparseopts.ztst
index 6a880b49c..938c6b386 100644
--- a/Test/V12zparseopts.ztst
+++ b/Test/V12zparseopts.ztst
@@ -34,15 +34,15 @@
 ?(anon): bad option: -x
 >-x
 
-  () { zparseopts -F } -x
-  () { zparseopts -F - } -x
   () { zparseopts -F -- } -x
+  () { zparseopts -F - } -x
+  () { zparseopts -F - a } -x
   () { zparseopts -F - a } -x
   () { zparseopts -F a } -x
-1:zparseopts without option specs, without array (may change in future)
+1:zparseopts without option specs, without array
 ?(anon):zparseopts: missing option descriptions
-?(anon): bad option: -x
-?(anon): bad option: -x
+?(anon):zparseopts: missing option descriptions
+?(anon):zparseopts: no default array defined: a
 ?(anon):zparseopts: no default array defined: a
 ?(anon):zparseopts: no default array defined: a
 
@@ -439,3 +439,50 @@
 ?fn2: bad option: -x
 ?prog: bad option: -x
 ?prog: bad option: -x
+
+  () { zparseopts -DF - '' } -x
+  () {
+    local -a optv
+    zparseopts -DFa optv - a
+    typeset -p optv
+  } -a
+0:zparseopts option stacking
+?(anon): bad option: -x
+>typeset -a optv=( -a )
+
+  () {
+    local -a optv
+    zparseopts -DFa optv -x
+  }
+  () {
+    local -a optv
+    zparseopts -DFa optv - -D
+    typeset -p optv
+  } --D
+  () {
+    local -a optv
+    zparseopts -DFa optv - -DFa
+    typeset -p optv
+  } --DFa
+0:zparseopts long-option spec guarding
+?(anon):zparseopts:2: bad option: -x
+>typeset -a optv=( --D )
+>typeset -a optv=( --DFa )
+
+  () { zparseopts -a }
+  () { zparseopts -A }
+  () { zparseopts -n }
+  () { zparseopts -v }
+  () { zparseopts -a '' }
+  () { zparseopts -A '' }
+  () { zparseopts -n '' }
+  () { zparseopts -v '' }
+1:zparseopts missing/empty optargs
+?(anon):zparseopts: argument expected: -a
+?(anon):zparseopts: argument expected: -A
+?(anon):zparseopts: argument expected: -n
+?(anon):zparseopts: argument expected: -v
+?(anon):zparseopts: missing array name for -a
+?(anon):zparseopts: missing array name for -A
+?(anon):zparseopts: missing program name for -n
+?(anon):zparseopts: missing array name for -v