[PATCH] Fix assigning/typesetting of unset references (and related bugs)

[Philippe Altherr <philippe.altherr@xxxxxxxxx>]

Assigning an unset local reference R, wrongly resurrects the parameter R as a new reference instead of resurrecting it as a new string parameter R:

$ () { typeset -g -n gref=foo; unset -n gref; gref=bar; typeset -p gref }
typeset -g gref=bar
$ () { typeset    -n lref=foo; unset -n lref; lref=bar; typeset -p lref }

typeset -n lref=bar

The same bug occurs if one typesets R instead of assigning it:

() { typeset -g -n gref=foo; unset -n gref; typeset -g gref=bar; typeset -p gref }

typeset -g gref=bar
() { typeset    -n lref=foo; unset -n lref; typeset    lref=bar; typeset -p lref }
typeset -n lref=bar

The following patch fixes these bugs as well as a number of related bugs:

Fix assigning/typesetting of unset references (and related bugs)

For more details about the related bugs have a look on GitHub at the tests added in the first commit of the patch and look for all the "F:BUG" comments in the expected output of the tests.

Philippe

diff --git a/Src/builtin.c b/Src/builtin.c
index c269dc1c6..7c095149d 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2038,6 +2038,7 @@ typeset_single(char *cname, char *pname, Param pm, int func,
 	if ((pm = resolve_nameref(pm)))
 	    pname = pm->node.nam;
 	if (pm && (pm->node.flags & PM_NAMEREF) &&
+	    (!(pm->node.flags & PM_UNSET) || (pm->node.flags & PM_DECLARED)) &&
 	    (on & ~(PM_NAMEREF|PM_LOCAL|PM_READONLY))) {
 	    /* Changing type of PM_SPECIAL|PM_AUTOLOAD is a fatal error.  *
 	     * Should this be a fatal error as well, rather than warning? */
diff --git a/Src/params.c b/Src/params.c
index 3199fd17b..afc67eb14 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -1044,7 +1044,9 @@ createparam(char *name, int flags)
 	if (oldpm && !(flags & PM_NAMEREF) &&
 	    (oldpm->level == locallevel ?
 	     !(oldpm->node.flags & PM_RO_BY_DESIGN) : !(flags & PM_LOCAL)) &&
-	    (oldpm->node.flags & PM_NAMEREF)) {
+	    (oldpm->node.flags & PM_NAMEREF) &&
+	    (!(oldpm->node.flags & PM_UNSET) ||
+	     (oldpm->node.flags & PM_DECLARED))) {
 	    /**
 	     * Here we only have to deal with namerefs that refer to
 	     * not-yet-defined or unset variable. All other namerefs
@@ -1054,14 +1056,18 @@ createparam(char *name, int flags)
 	     **/
 	    Param lastpm = resolve_nameref_rec(oldpm, NULL, 1);
 	    if (lastpm) {
-		if (lastpm->node.flags & PM_NAMEREF) {
+		if (lastpm->node.flags & PM_NAMEREF &&
+		    (!(lastpm->node.flags & PM_UNSET) ||
+		     (lastpm->node.flags & PM_DECLARED))) {
 		    char *refname = GETREFNAME(lastpm);
 		    if (refname && *refname) {
+			/* nameref pointing to a not-yet-defined variable */
 			name = refname;
 			oldpm = NULL;
 		    } else {
+			/* nameref pointing to an uninitialized nameref */
 			if (!(lastpm->node.flags & PM_READONLY)) {
-			    if (flags) {
+			    if (flags & ~PM_LOCAL) {
 				/* Only plain scalar assignment allowed */
 				zerr("%s: can't change type of named reference",
 				     name);	/* Differs from ksh93u+ */
@@ -3386,6 +3392,10 @@ assignaparam(char *s, char **val, int flags)
 	if (!(v = fetchvalue(&vbuf, &s, 1, SCANPM_ASSIGNING))) {
 	    createparam(t, PM_ARRAY);
 	    created = 1;
+	} else if (v->pm->node.flags & PM_NAMEREF) {
+	    zwarn("%s: can't change type of a named reference", t);
+	    unqueue_signals();
+	    return NULL;
 	} else if (!(PM_TYPE(v->pm->node.flags) & (PM_ARRAY|PM_HASHED)) &&
 		   !(v->valflags & VALFLAG_REFSLICE) &&
 		   !(v->pm->node.flags & (PM_SPECIAL|PM_TIED))) {
@@ -6321,35 +6331,30 @@ resolve_nameref(Param pm)
 static Param
 resolve_nameref_rec(Param pm, const Param stop, int keep_lastref)
 {
-    Param hn = pm;
-    if (pm && (pm->node.flags & PM_NAMEREF)) {
-	char *refname = GETREFNAME(pm);
-	if (pm->node.flags & PM_TAGGED) {
-	    zerr("%s: invalid self reference", pm->node.nam);
-	    return NULL;
-	} else if (pm->node.flags & PM_UNSET) {
-	    /* Semaphore with createparam() */
-	    pm->node.flags &= ~PM_UNSET;
-	    return pm;
-	}
+    Param ref = pm;
+    char *refname;
+    if (!pm || !(pm->node.flags & PM_NAMEREF) || (pm->node.flags & PM_UNSET)
 	/* pm->width is the offset of any subscript */
 	/* If present, it has to be the end of any chain, see fetchvalue() */
-	if (refname && *refname && !pm->width) {
-	    queue_signals();
-	    if ((hn = (Param)gethashnode2(realparamtab, refname))) {
-		if ((hn = loadparamnode(paramtab, upscope(hn, pm), refname)) &&
-		    hn != stop && !(hn->node.flags & PM_UNSET)) {
-		    /* user can't tag a nameref, safe for loop detection */
-		    pm->node.flags |= PM_TAGGED;
-		    hn = resolve_nameref_rec(hn, stop, keep_lastref);
-		    pm->node.flags &= ~PM_TAGGED;
-		}
-	    } else if (keep_lastref)
-		hn = pm;
-	    unqueue_signals();
-	}
+	|| pm->width || !(refname = GETREFNAME(pm)) || !*refname)
+	return pm;
+    if (pm->node.flags & PM_TAGGED) {
+	zerr("%s: invalid self reference", pm->node.nam);
+	return NULL;
     }
-    return hn;
+    queue_signals();
+    if ((pm = (Param)gethashnode2(realparamtab, refname))) {
+	if ((pm = loadparamnode(paramtab, upscope(pm, ref), refname)) &&
+	    pm != stop && !(pm->node.flags & PM_UNSET)) {
+	    /* user can't tag a nameref, safe for loop detection */
+	    ref->node.flags |= PM_TAGGED;
+	    pm = resolve_nameref_rec(pm, stop, keep_lastref);
+	    ref->node.flags &= ~PM_TAGGED;
+	}
+    } else if (keep_lastref)
+	pm = ref;
+    unqueue_signals();
+    return pm;
 }
 
 /**/
diff --git a/Src/zsh.h b/Src/zsh.h
index 0dc83ded4..dd58c0816 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1927,7 +1927,10 @@ struct tieddata {
 #define PM_DONTIMPORT	(1<<22)	/* do not import this variable              */
 #define PM_DECLARED	(1<<22) /* explicitly named with typeset            */
 #define PM_RESTRICTED	(1<<23) /* cannot be changed in restricted mode     */
-#define PM_UNSET	(1<<24)	/* has null value                           */
+#define PM_UNSET	(1<<24)	/* If PM_DECLARED is also present, parameter
+				 * has null value. Otherwise, parameter was
+				 * unset.
+				 */
 #define PM_DEFAULTED	(PM_DECLARED|PM_UNSET)
 #define PM_REMOVABLE	(1<<25)	/* special can be removed from paramtab     */
 #define PM_AUTOLOAD	(1<<26) /* autoloaded from module                   */
diff --git a/Test/K01nameref.ztst b/Test/K01nameref.ztst
index e0f7b1879..8885fcd2d 100644
--- a/Test/K01nameref.ztst
+++ b/Test/K01nameref.ztst
@@ -187,11 +187,13 @@
 >typeset -n ptr=gvar
 
  typeset -n ptr
+ typeset -p ptr
  typeset -t ptr
  typeset -p ptr
 0:change type of a placeholder
 F:Other type changes are fatal errors, should this also be?
->typeset -n ptr=''
+>typeset -n ptr
+>typeset -n ptr
 *?*ptr: can't change type of a named reference
 
  typeset -n ptr=var
@@ -1659,4 +1661,231 @@ F:converting from association/array to string should work here too
 1:self reference via upper reference
 ?(anon):3: ref1: invalid self reference
 
+ # The only purpose of this test is to define the helper functions
+ # needed by the tests below about assigning and typesetting named
+ # references in different contexts.
+ #
+ # Runs the code passed in "$1" for all combinations of the following
+ # variables:
+ #
+ # - K: Kind of target reference. The assigned/typset reference is one
+ #   of the following kind:
+ #   - "i": unInitialized reference:
+ #     e.g., "ref0" in "typeset -n ref0"
+ #   - "s": unSet reference:
+ #     e.g., "ref0" in "typeset -n ref0; unset ref0"
+ #   - "d": reference to not-yet-Defined variable:
+ #     e.g., "ref0" in "typeset -n ref0=var" where "var" isn't defined
+ #
+ # - G: Global vs local references
+ #   - "-g": All test variables/references are global ones.
+ #   - ""  : All test variables/references are local ones.
+ #
+ # - N: direct vs indirect references
+ #   - "0": The target reference "ref0" is directly assigned/typeset.
+ #   - "1": The target reference "ref1" is assigned/typeset via a
+ #          named reference "ref1" that refers to "ref0".
+ test-all() {
+   local K G N;
+   for K in i s d; do
+     for G in -g ""; do
+       for N in 0 1; do
+         test-one $1
+       done
+     done
+   done
+   unset -n ref0 ref1 var
+ }
+ # Runs the code passed in "$1" for the context specified by the
+ # variables "K", "G", and "N".
+ test-one() {
+   unset -n ref0 ref1 var
+   # In several contexts different code paths are involved depending
+   # on whether the target reference "ref0" is assigned/typeset
+   # directly or via a named reference that referes to it. However, it
+   # doesn't seem to make a difference whether the chain of references
+   # is short or long. Therefore only chains of one reference are
+   # tested. One can manually double check that longer chains work the
+   # same by replacing the following line by one of the commented one
+   # just below and checking that all tests still pass.
+   ((N < 1)) || typeset $G -n ref1=ref0
+   # ((N < 1)) || typeset $G -n refX=ref0 ref1=refX
+   # ((N < 1)) || typeset $G -n refX=ref0 refY=refX ref1=refY
+   local kind print
+   case $K in
+     i) kind="uninitialized reference"     ; check=ref0; typeset $G -n ref0;;
+     s) kind="unset reference"             ; check=ref0; typeset $G -n ref0; unset -n ref0;;
+     d) kind="reference to not-yet-defined"; check=var ; typeset $G -n ref0=var;;
+     *) echo "Unrecognized case: $case"; return;;
+   esac
+   echo "# $K:$kind - ${${G:-local}/-g/global} - ref$N"
+   { eval ${(e)1}; typeset -p $check } always { TRY_BLOCK_ERROR=0 } 2>&1
+ }
+0:define the helper functions needed by the tests below
+
+ # The call to "test-all 'ref$N=str'" expands to the equivalent of the
+ # following:
+ #
+ # # Test uninitialized references
+ # typeset -g -n ref0          ;                ref0=str; typeset -p ref0; unset -n ref0
+ # typeset -g -n ref0 ref1=ref0;                ref1=str; typeset -p ref0; unset -n ref0 ref1
+ # typeset    -n ref0          ;                ref0=str; typeset -p ref0; unset -n ref0
+ # typeset    -n ref0 ref1=ref0;                ref1=str; typeset -p ref0; unset -n ref0 ref1
+ # # Test unset references
+ # typeset -g -n ref0          ; unset -n ref0; ref0=str; typeset -p ref0; unset -n ref0
+ # ...
+ # # Test references to not-yet-defined variables
+ # typeset -g -n ref0=var      ;                ref0=str; typeset -p var ; unset -n ref0 var
+ # ...
+ test-all 'ref$N=str'
+0:assign with string
+># i:uninitialized reference - global - ref0
+>typeset -g -n ref0=str
+># i:uninitialized reference - global - ref1
+>typeset -g -n ref0=str
+># i:uninitialized reference - local - ref0
+>typeset -n ref0=str
+># i:uninitialized reference - local - ref1
+>typeset -n ref0=str
+># s:unset reference - global - ref0
+>typeset -g ref0=str
+># s:unset reference - global - ref1
+>typeset -g ref0=str
+># s:unset reference - local - ref0
+>typeset ref0=str
+># s:unset reference - local - ref1
+>typeset ref0=str
+># d:reference to not-yet-defined - global - ref0
+>typeset -g var=str
+># d:reference to not-yet-defined - global - ref1
+>typeset -g var=str
+># d:reference to not-yet-defined - local - ref0
+>typeset -g var=str
+># d:reference to not-yet-defined - local - ref1
+>typeset -g var=str
+
+ test-all 'ref$N=( foo bar )'
+0:assign with array
+># i:uninitialized reference - global - ref0
+>(eval):1: ref0: can't change type of a named reference
+>typeset -g -n ref0
+># i:uninitialized reference - global - ref1
+>(eval):1: ref1: can't change type of a named reference
+>typeset -g -n ref0
+># i:uninitialized reference - local - ref0
+>(eval):1: ref0: can't change type of a named reference
+>typeset -n ref0
+># i:uninitialized reference - local - ref1
+>(eval):1: ref1: can't change type of a named reference
+>typeset -n ref0
+># s:unset reference - global - ref0
+>typeset -g -a ref0=( foo bar )
+># s:unset reference - global - ref1
+>typeset -g -a ref0=( foo bar )
+># s:unset reference - local - ref0
+>typeset -a ref0=( foo bar )
+># s:unset reference - local - ref1
+>typeset -a ref0=( foo bar )
+># d:reference to not-yet-defined - global - ref0
+>typeset -g -a var=( foo bar )
+># d:reference to not-yet-defined - global - ref1
+>typeset -g -a var=( foo bar )
+># d:reference to not-yet-defined - local - ref0
+>typeset -g -a var=( foo bar )
+># d:reference to not-yet-defined - local - ref1
+>typeset -g -a var=( foo bar )
+
+ test-all 'typeset $G ref$N=str'
+0:typeset with string
+># i:uninitialized reference - global - ref0
+>typeset -g -n ref0=str
+># i:uninitialized reference - global - ref1
+>typeset -g -n ref0=str
+># i:uninitialized reference - local - ref0
+>typeset -n ref0=str
+># i:uninitialized reference - local - ref1
+>typeset -n ref0=str
+># s:unset reference - global - ref0
+>typeset -g ref0=str
+># s:unset reference - global - ref1
+>typeset -g ref0=str
+># s:unset reference - local - ref0
+>typeset ref0=str
+># s:unset reference - local - ref1
+>typeset ref0=str
+># d:reference to not-yet-defined - global - ref0
+>typeset -g var=str
+># d:reference to not-yet-defined - global - ref1
+>typeset -g var=str
+># d:reference to not-yet-defined - local - ref0
+>typeset var=str
+># d:reference to not-yet-defined - local - ref1
+>typeset var=str
+
+ test-all 'typeset $G -a ref$N=( foo bar )'
+0:typeset with array
+># i:uninitialized reference - global - ref0
+>(eval):typeset:1: ref0: can't change type of a named reference
+>typeset -g -n ref0
+># i:uninitialized reference - global - ref1
+>(eval):typeset:1: ref0: can't change type of a named reference
+>typeset -g -n ref0
+># i:uninitialized reference - local - ref0
+>(eval):typeset:1: ref0: can't change type of a named reference
+>typeset -n ref0
+># i:uninitialized reference - local - ref1
+>(eval):typeset:1: ref0: can't change type of a named reference
+>typeset -n ref0
+># s:unset reference - global - ref0
+>typeset -g -a ref0=( foo bar )
+># s:unset reference - global - ref1
+>typeset -g -a ref0=( foo bar )
+># s:unset reference - local - ref0
+>typeset -a ref0=( foo bar )
+># s:unset reference - local - ref1
+>typeset -a ref0=( foo bar )
+># d:reference to not-yet-defined - global - ref0
+>typeset -g -a var=( foo bar )
+># d:reference to not-yet-defined - global - ref1
+>typeset -g -a var=( foo bar )
+># d:reference to not-yet-defined - local - ref0
+>typeset -a var=( foo bar )
+># d:reference to not-yet-defined - local - ref1
+>typeset -a var=( foo bar )
+
+ typeset i42=42
+ test-all 'typeset $G -i ref$N=i42'
+0:typeset with integer
+># i:uninitialized reference - global - ref0
+>(eval):typeset:1: ref0: can't change type of a named reference
+>typeset -g -n ref0
+># i:uninitialized reference - global - ref1
+>(eval):typeset:1: ref0: can't change type of a named reference
+>typeset -g -n ref0
+># i:uninitialized reference - local - ref0
+>(eval):typeset:1: ref0: can't change type of a named reference
+>typeset -n ref0
+># i:uninitialized reference - local - ref1
+>(eval):typeset:1: ref0: can't change type of a named reference
+>typeset -n ref0
+># s:unset reference - global - ref0
+>typeset -g -i ref0=42
+># s:unset reference - global - ref1
+>typeset -g -i ref0=42
+># s:unset reference - local - ref0
+>typeset -i ref0=42
+># s:unset reference - local - ref1
+>typeset -i ref0=42
+># d:reference to not-yet-defined - global - ref0
+>typeset -g -i var=42
+># d:reference to not-yet-defined - global - ref1
+>typeset -g -i var=42
+># d:reference to not-yet-defined - local - ref0
+>typeset -i var=42
+># d:reference to not-yet-defined - local - ref1
+>typeset -i var=42
+
+ unfunction test-one test-all
+0:delete the helper functions used in the tests above
+
 %clean
diff --git a/Test/V10private.ztst b/Test/V10private.ztst
index 4abbbf5c9..256844be1 100644
--- a/Test/V10private.ztst
+++ b/Test/V10private.ztst
@@ -370,7 +370,7 @@ F:See K01nameref.ztst up-reference part 5
 F:Here ptr1 finds private ptr2 by scope mismatch
 >typeset -n ptr1=ptr2
 >typeset -hn ptr2
-*?*read-only variable: ptr2
+?(anon):1: read-only variable: ptr2
 
  () {
    typeset -n ptr1=ptr2
@@ -378,7 +378,7 @@ F:Here ptr1 finds private ptr2 by scope mismatch
    typeset -p ptr1 ptr2
    typeset val=LOCAL
    () {
-     ptr1=val || print -u2 ptr1: assignment failed
+     ptr1=val		# Test dies here as ptr2 is private and uninitialized
      typeset -n
      printf "%s=%s\n" ptr1 "$ptr1" ptr2 "$ptr2"
    }
@@ -390,13 +390,7 @@ F:See K01nameref.ztst up-reference part 5
 F:Here ptr1 finds private ptr2 by scope mismatch
 >typeset -n ptr1=ptr2
 >typeset -hn ptr2=''
->ptr1=ptr2
->ptr1=
->ptr2=
->typeset -n ptr1=ptr2
->typeset -hn ptr2=''
-*?*ptr1: assignment failed
-*?*no such variable: ptr2
+?(anon):1: ptr1: can't modify read-only parameter
 
  typeset ptr2
  () {