[PATCH] Misc. named reference updates

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

This patch covers a bunch of odds and ends referenced or revealed by
Jun T.'s commentary over the last few days.

1) Document the behavior of "typeset -n existing_var",  (via Jun T. comment)
2) Prohibit "typeset -nm pattern" because, well, it's insane.  Add test.
3) Improve doc for ${(!)ref} including ${{t!)ref} (Jun T.)
4) Fix doc for how-to unset of a named ref (Jun T.)
5) Allow "typeset +r -n ref" and "typeset +r +n ref" (Jun T.)
6) Fix "typeset -r -n ref=param" to create readonly references
7) Avoid accidental removal of PM_UNSET flag (Jun T.)  and update test
8) Fix "typeset -gn ref=value" and add a test for it
9) Add tests for read-only reference behavior
10) Fix infinite recursion when resolving scope of an unset local
named reference, add test.

RE #5:  Not explicitly called out anywhere yet (suggestions?) but
notice that parameter type conversions occur before parameter
assignments when using "typeset [options] var=val".  See K01 test.

RE #6: There is an explicit exception to the above for "typeset -r
var=val".  I'm not entirely happy with the way this had to be fixed
for -n, but it's necessary to prevent following the reference.

RE #10: Previously if a local named reference was unset and an
assignment was made to the name, resolve_nameref() could find a
reference of the same name in the enclosing scope and end up in an
infinite loop.  Aside from the loop, looking up-scope is wrong just
from expected behavior of locals.  The fix results in assigning a new
referent to the (thereafter not unset) reference.

diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index 5393cb149..33b13ac16 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -2060,6 +2060,11 @@ function unless `tt(-g -n)' is specified, and any local parameter (of
 any type) with the same var(name) supplants a named reference from a
 surrounding scope.
 
+A scalar parameter, including an existing named reference, may be
+converted to a new named reference by `tt(typeset -n )var(name)', so
+the `tt(-p)' option must be included to display the value of a
+specific named reference var(name).
+
 If no attribute flags are given, and either no var(name) arguments are
 present or the flag tt(+m) is used, then each parameter name printed is
 preceded by a list of the attributes of that parameter (tt(array),
@@ -2104,7 +2109,8 @@ is not used in this case).
 
 If the tt(+g) flag is combined with tt(-m), a new local parameter is
 created for every matching parameter that is not already local.  Otherwise
-tt(-m) applies all other flags or assignments to the existing parameters.
+tt(-m) applies all other flags or assignments to the existing parameters,
+except that the tt(-n) option cannot create named references in this way.
 
 Except when assignments are made with var(name)tt(=)var(value), using
 tt(+m) forces the matching parameters and their attributes to be printed,
diff --git a/Doc/Zsh/expn.yo b/Doc/Zsh/expn.yo
index 7bc736470..f87832e75 100644
--- a/Doc/Zsh/expn.yo
+++ b/Doc/Zsh/expn.yo
@@ -987,6 +987,11 @@ means the same thing as the more readable `(tt(%%qqq))'.  The
 following flags are supported:
 
 startitem()
+item(tt(!))(
+When the parameter being expanded is a named reference, the reference
+itself is examined and thus is em(not) resolved to its referent.  In
+ksh emulation, the parens around this flag are optional.
+)
 item(tt(#))(
 Evaluate the resulting words as numeric expressions and interpret
 these as character codes.  Output the corresponding characters.  Note
@@ -1245,7 +1250,8 @@ item(tt(hideval))(
 for parameters with the `hideval' flag (tt(-H))
 )
 item(tt(nameref))(
-for named references having an empty value (tt(-n))
+for named references (tt(typeset -n)) either having an empty value or
+when combined with `tt(!)' as in `tt(${LPAR()!t)tt(RPAR()var(rname)})'
 )
 item(tt(special))(
 for special parameters defined by the shell
diff --git a/Doc/Zsh/params.yo b/Doc/Zsh/params.yo
index e0410d673..5653b3bc9 100644
--- a/Doc/Zsh/params.yo
+++ b/Doc/Zsh/params.yo
@@ -672,9 +672,9 @@ of var(pname) in assignments and expansions instead assign to or
 expand var(rname).  This also applies to `tt(unset )var(pname)' and to
 most subsequent uses of `tt(typeset)' with the exception of
 `tt(typeset -n)' and `tt(typeset +n)', so to remove a named reference,
-use either `tt(unset -n )var(pname)' or one of:
+use either `tt(unset -n )var(pname)' (preferred) or one of:
 ifzman()
-example(tt(typeset -n )var(pname)
+example(tt(typeset -n )var(pname=)
 tt(typeset +n )var(pname))
 
 followed by
diff --git a/Src/builtin.c b/Src/builtin.c
index 669a47092..48a5f08f3 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2248,10 +2248,14 @@ typeset_single(char *cname, char *pname, Param pm, int func,
 	    zerrnam(cname, "%s: restricted", pname);
 	    return pm;
 	}
-	if ((pm->node.flags & PM_READONLY) &&
-	    (pm->node.flags & PM_NAMEREF & off)) {
-	    zerrnam(cname, "%s: read-only reference", pname);
-	    return pm;
+	if ((pm->node.flags & PM_READONLY) && !(off & PM_READONLY) &&
+	    /* It seems as though these checks should not be specific to
+	     * PM_NAMEREF, but changing that changes historic behavior */
+	    ((on & PM_NAMEREF) != (pm->node.flags & PM_NAMEREF) ||
+	     (asg && (pm->node.flags & PM_NAMEREF)))) {
+	    zerrnam(cname, "%s: read-only %s", pname,
+		    (pm->node.flags & PM_NAMEREF) ? "reference" : "variable");
+	    return NULL;
 	}
 	if ((on & PM_UNIQUE) && !(pm->node.flags & PM_READONLY & ~off)) {
 	    Param apm;
@@ -2693,7 +2697,7 @@ bin_typeset(char *name, char **argv, LinkList assigns, Options ops, int func)
 	    off |= bit;
     }
     if (OPT_MINUS(ops,'n')) {
-	if ((on & ~PM_READONLY)|off) {
+	if ((on|off) & ~PM_READONLY) {
 	    zwarnnam(name, "no other attributes allowed with -n");
 	    return 1;
 	}
@@ -3021,6 +3025,13 @@ bin_typeset(char *name, char **argv, LinkList assigns, Options ops, int func)
     /* With the -m option, treat arguments as glob patterns */
     if (OPT_ISSET(ops,'m')) {
 	if (!OPT_ISSET(ops,'p')) {
+	    if (on & PM_NAMEREF) {
+		/* It's generally unwise to mass-change the types of
+		 * parameters, but for namerefs it would be fatal */
+		unqueue_signals();
+		zerrnam(name, "invalid reference");
+		return 1;
+	    }
 	    if (!(on|roff))
 		printflags |= PRINT_TYPE;
 	    if (!on)
@@ -3104,13 +3115,25 @@ bin_typeset(char *name, char **argv, LinkList assigns, Options ops, int func)
 	    }
 	    if (hn) {
 		/* namerefs always start over fresh */
-		if (((Param)hn)->level >= locallevel) {
+		if (((Param)hn)->level >= locallevel ||
+		    (!(on & PM_LOCAL) && ((Param)hn)->level < locallevel)) {
 		    Param oldpm = (Param)hn;
-		    if (!asg->value.scalar && oldpm->u.str)
+		    if (!asg->value.scalar &&
+			PM_TYPE(oldpm->node.flags) == PM_SCALAR &&
+			oldpm->u.str)
 			asg->value.scalar = dupstring(oldpm->u.str);
-		    unsetparam_pm((Param)hn, 0, 1);
+		    /* Defer read-only error to typeset_single() */
+		    if (!(hn->flags & PM_READONLY))
+			unsetparam_pm(oldpm, 0, 1);
 		}
-		hn = NULL;
+		/* Passing a NULL pm to typeset_single() makes the
+		 * nameref read-only before assignment, which breaks
+		 *   typeset -rn ref=var
+		 * so this is special-cased to permit that action
+		 * like assign-at-create for other parameter types.
+		 */
+		if (!(hn->flags & PM_READONLY))
+		    hn = NULL;
 	    }
 	}
 
diff --git a/Src/params.c b/Src/params.c
index f5750a4b4..5841308d7 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -546,7 +546,7 @@ getparamnode(HashTable ht, const char *nam)
 	}
     }
 
-    if (hn && ht == realparamtab)
+    if (hn && ht == realparamtab && !(hn->flags & PM_UNSET))
 	hn = resolve_nameref((Param)hn, NULL);
     return hn;
 }
@@ -3729,7 +3729,9 @@ unsetparam_pm(Param pm, int altflag, int exp)
     char *altremove;
 
     if ((pm->node.flags & PM_READONLY) && pm->level <= locallevel) {
-	zerr("read-only variable: %s", pm->node.nam);
+	zerr("read-only %s: %s",
+	     (pm->node.flags & PM_NAMEREF) ? "reference" : "variable",
+	     pm->node.nam);
 	return 1;
     }
     if ((pm->node.flags & PM_RESTRICTED) && isset(RESTRICTED)) {
@@ -6182,8 +6184,12 @@ resolve_nameref(Param pm, const Asgment stop)
 		seek = refname;
 	}
     }
-    else if (pm && !(stop && (stop->flags & PM_NAMEREF)))
-	return (HashNode)pm;
+    else if (pm) {
+	if (!(stop && (stop->flags & PM_NAMEREF)))
+	    return (HashNode)pm;
+	if (!(pm->node.flags & PM_NAMEREF))
+	    return (pm->level < locallevel ? NULL : (HashNode)pm);
+    }
     if (seek) {
 	queue_signals();
 	/* pm->width is the offset of any subscript */
diff --git a/Test/K01nameref.ztst b/Test/K01nameref.ztst
index 6a5e767df..d8c098a98 100644
--- a/Test/K01nameref.ztst
+++ b/Test/K01nameref.ztst
@@ -515,7 +515,7 @@ F:Same test, should part 5 output look like this?
 >ptr1=val
 >ptr2=
 >typeset -n ptr1=ptr2
->typeset -n ptr2=''
+>typeset -n ptr2
 >typeset ptr2=val
 
  if zmodload zsh/parameter; then
@@ -694,4 +694,72 @@ F:Checking for a bug in zmodload that affects later tests
 F:runs in `setopt noexec` so $(...) returns nothing
 *?*bad math expression: empty string
 
+ unset -n ref
+ typeset -n ref=GLOBAL
+ () {
+   typeset -gn ref=RESET
+ }
+ typeset -p ref
+0:reset global reference within function
+>typeset -n ref=RESET
+
+ unset -n ref
+ typeset -rn ref=RO
+ typeset -p ref
+ (typeset -n ref=RW)
+ print status: $? expected: 1
+ typeset +r -n ref
+ typeset -p ref
+ typeset -r +n ref
+ typeset -p ref
+ (typeset -rn ref)
+ print status: $? expected: 1
+ typeset +r -n ref=RW	# Assignment occurs after type change,
+ typeset -p ref RO	# so RO=RW here.  Potentially confusing.
+ typeset -r -n ref=RX	# No type change, so referent changes ...
+ typeset -p ref RO	# ... and previous refererent does not.
+ typeset +rn ref=RW	# Here ref=RW, again type changed first.
+ typeset -p ref
+0:add and remove readonly attribute with references
+>typeset -rn ref=RO
+*?*: ref: read-only reference
+>status: 1 expected: 1
+>typeset -n ref=RO
+>typeset -r ref=RO
+*?*: ref: read-only variable
+>status: 1 expected: 1
+>typeset -n ref=RO
+>typeset -g RO=RW
+>typeset -rn ref=RX
+>typeset -g RO=RW
+>typeset ref=RW
+
+ () {
+  typeset -n r1 r2=
+  typeset -p r1 r2
+  print -- ${(!)r1-unset}
+  print -- ${+r1}
+  typeset -p r1
+ }
+0:unset nameref remains unset when resolved
+F:relies on global TYPESET_TO_UNSET in %prep
+>typeset -n r1
+>typeset -n r2=''
+>unset
+>0
+>typeset -n r1
+
+ bar=xx
+ typeset -n foo=bar
+ () { typeset -n foo; foo=zz; foo=zz; print $bar $zz }
+ () { typeset -n foo; foo=zz; local zz; foo=zz; print $bar $zz }
+0:regression: local nameref may not in-scope a global parameter
+F:previously this could create an infinite recursion and crash
+>xx
+>xx zz
+
+ typeset -nm foo=bar
+1:create nameref by pattern match not allowed
+*?*typeset:1: invalid reference
+
 %clean