[PATCH] Fix (mainly nameref) issues in builtin "unset"

[Philippe Altherr <philippe.altherr@xxxxxxxxx>]

The patch fixes a number of issues in the implementation of the builtin "unset". Most of them are related to named references. For example, "unset ref" fails to correctly unset the parameter referred to by "ref" if that parameter is in an enclosing scope of the current one and "unset -m ref" fails to unset the referred parameter if there is a chain of references. For a full list of all the issues look for occurrences of "F:BUG:" in the first commit of the GitHub change linked below.

The patch also changes a minor behavior of "unset -m". Readonly parameters can't be unset. The same is true for restricted parameters in restricted mode. The current implementation of "unset -m" contains code to skip restricted parameters in restricted mode. However, that code doesn't work correctly for references. Since there is no such equivalent code for readonly parameters, and since the restricted mode is about to get eliminated (workers/54166), I decided to eliminate that behavior rather than to fix it.

- Fix (mainly nameref) issues in builtin "unset"

For reviewing the patch, I strongly recommend looking at the individual commits in the GitHub change linked above.

Philippe

diff --git a/Src/builtin.c b/Src/builtin.c
index 7c095149d..83a52d3ab 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -3857,13 +3857,10 @@ bin_unset(char *name, char **argv, Options ops, int func)
 		    for (pm = (Param) paramtab->nodes[i]; pm; pm = next) {
 			/* record pointer to next, since we may free this one */
 			next = (Param) pm->node.next;
-			if ((!(pm->node.flags & PM_RESTRICTED) ||
-			     unset(RESTRICTED)) &&
-			    pattry(pprog, pm->node.nam)) {
-			    if (!OPT_ISSET(ops,'n') &&
-				(pm->node.flags & PM_NAMEREF) && pm->u.str)
-				unsetparam(pm->u.str);
-			    else
+			if (pattry(pprog, pm->node.nam)) {
+			    if (OPT_ISSET(ops,'n') ||
+				((pm = resolve_nameref(pm)) &&
+				 !(pm->node.flags & PM_NAMEREF)))
 				unsetparam_pm(pm, 0, 1);
 			    match++;
 			}
@@ -3912,10 +3909,7 @@ bin_unset(char *name, char **argv, Options ops, int func)
 	 */
 	if (!pm)
 	    continue;
-	else if ((pm->node.flags & PM_RESTRICTED) && isset(RESTRICTED)) {
-	    zerrnam(name, "%s: restricted", pm->node.nam);
-	    returnval = 1;
-	} else if (ss) {
+	else if (ss) {
 	    if ((pm->node.flags & PM_NAMEREF) &&
 		(!(pm = resolve_nameref(pm)) || pm->width)) {
 		/* warning? */
@@ -3959,22 +3953,11 @@ bin_unset(char *name, char **argv, Options ops, int func)
 		zerrnam(name, "%s: invalid element for unset", s);
 		returnval = 1;
 	    }
-	} else {
-	    if (!OPT_ISSET(ops,'n')) {
-		int ref = (pm->node.flags & PM_NAMEREF);
-		if (!(pm = resolve_nameref(pm)))
-		    continue;
-		if (ref && pm->level < locallevel &&
-		    !(pm->node.flags & PM_READONLY)) {
-		    /* Just mark unset, do not remove from table */
-		    stdunsetfn(pm, 0);
-		    pm->node.flags |= PM_DECLARED;
-		    continue;
-		}
-	    }
+	} else if (OPT_ISSET(ops,'n') ||
+		   ((pm = resolve_nameref(pm)) &&
+		    !(pm->node.flags & PM_NAMEREF)))
 	    if (unsetparam_pm(pm, 0, 1))
 		returnval = 1;
-	}
 	if (ss)
 	    *ss = '[';
     }
diff --git a/Src/params.c b/Src/params.c
index 461e02acf..22300aa25 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -499,6 +499,16 @@ static Param argvparam;
  * times in the same list. Non of that is harmful as long as only
  * instances that are still references referring to the ending scope
  * are updated when the scope ends.
+ *
+ * The list corresponding to the global scope never receives any of
+ * the named references described above. Instead, it's used to track
+ * global parameters that were unset via a named reference while in a
+ * scope where they were hidden by a nested parameter with the same
+ * name. In such cases, the global parameter's Param instance can't be
+ * deleted as usual. Instead, it's marked as unset and added to the
+ * global scope's list. Each time a scope ends, the list is traversed
+ * and parameters that are still unset but no longer hidden are
+ * deleted.
  */
 static LinkList *scoperefs = NULL;
 static int scoperefs_num = 0;
@@ -3934,6 +3944,24 @@ unsetparam_pm(Param pm, int altflag, int exp)
 	(pm->node.flags & (PM_SPECIAL|PM_REMOVABLE)) == PM_SPECIAL)
 	return 0;
 
+    /*
+     * Global variables can only be deleted if they aren't hidden by a
+     * local one with the same name.
+     */
+    if (!pm->level &&
+	pm != (Param) (paramtab == realparamtab ?
+		       /* getnode2() to avoid autoloading */
+		       paramtab->getnode2(paramtab, pm->node.nam) :
+		       paramtab->getnode(paramtab, pm->node.nam))) {
+	LinkList refs;
+	if (!scoperefs)
+	    scoperefs = zshcalloc((scoperefs_num = 8) * sizeof(refs));
+	if (!scoperefs[0])
+	    scoperefs[0] = znewlinklist();
+	zpushnode(scoperefs[0], pm);
+	return 0;
+    }
+
     /* remove parameter node from table */
     paramtab->removenode(paramtab, pm->node.nam);
 
@@ -5909,6 +5937,15 @@ endparamscope(void)
 	    setscope(pm);
 	}
     }
+    /* Delete unset global variables that were hidden at unset time */
+    if ((refs = scoperefs ? scoperefs[0] : NULL)) {
+	scoperefs[0] = NULL;
+	for (Param pm; refs && (pm = (Param)getlinknode(refs));) {
+	    if ((pm->node.flags & PM_UNSET) && !(pm->node.flags & PM_DECLARED))
+		unsetparam_pm(pm, 1, 0);
+	}
+	freelinklist(refs, NULL);
+    }
     unqueue_signals();
 }
 
diff --git a/Test/K01nameref.ztst b/Test/K01nameref.ztst
index 0b4475827..7734f766d 100644
--- a/Test/K01nameref.ztst
+++ b/Test/K01nameref.ztst
@@ -1026,15 +1026,15 @@ F:Checking for a bug in zmodload that affects later tests
  typeset -p .K01.{scalar,assoc,array,integer,double,float,readonly}
  unset .K01.{scalar,assoc,array,integer,double,float}
 0:unset various types via nameref, including a readonly special
->typeset -g .K01.scalar
->typeset -g -A .K01.assoc
->typeset -g -a .K01.array
->typeset -g -i .K01.integer
->typeset -g -E .K01.double
->typeset -g -F .K01.float
 >typeset -g -r .K01.readonly=RO
 *?*read-only variable: ARGC
 *?*read-only variable: .K01.readonly
+*?*no such variable: .K01.scalar
+*?*no such variable: .K01.assoc
+*?*no such variable: .K01.array
+*?*no such variable: .K01.integer
+*?*no such variable: .K01.double
+*?*no such variable: .K01.float
 
  unset -n ref
  unset one
@@ -1933,6 +1933,257 @@ F:converting from association/array to string should work here too
 ># d:reference to not-yet-defined - local - ref1
 >typeset -i var=42
 
+ test-unset() {
+   typeset var0=foo
+   typeset -n ref1=var0 ref2=ref1
+   typeset cmd=(unset $@); echo "#" $cmd; $cmd
+   typeset -p var0 ref1 ref2
+ }
+ test-unset -n    ref1
+ test-unset -n    ref2
+ test-unset -n -m ref1
+ test-unset -n -m ref2
+ unfunction test-unset
+0:unsetting references with -n unsets the references
+># unset -n ref1
+>typeset var0=foo
+>typeset -n ref2=ref1
+># unset -n ref2
+>typeset var0=foo
+>typeset -n ref1=var0
+># unset -n -m ref1
+>typeset var0=foo
+>typeset -n ref2=ref1
+># unset -n -m ref2
+>typeset var0=foo
+>typeset -n ref1=var0
+
+ test-unset() {
+   typeset var0=foo
+   typeset -n ref1=var0 ref2=ref1
+   typeset cmd=(unset $@); echo "#" $cmd; $cmd
+   typeset -p var0 ref1 ref2
+ }
+ test-unset    ref1
+ test-unset    ref2
+ test-unset -m ref1
+ test-unset -m ref2
+ unfunction test-unset
+0:unsetting references without -n unsets the referred parameters
+># unset ref1
+>typeset -n ref1=var0
+>typeset -n ref2=ref1
+># unset ref2
+>typeset -n ref1=var0
+>typeset -n ref2=ref1
+># unset -m ref1
+>typeset -n ref1=var0
+>typeset -n ref2=ref1
+># unset -m ref2
+>typeset -n ref1=var0
+>typeset -n ref2=ref1
+
+ test-unset() {
+   typeset var0=12345
+   typeset -n ref1=var0 ref2=ref1
+   typeset cmd=(unset $@); echo "#" $cmd; $cmd
+   typeset -p var0
+ }
+ test-unset    ref1"[3]"
+ test-unset    ref2"[3]"
+ test-unset -n ref1"[3]"
+ test-unset -n ref2"[3]"
+ unfunction test-unset
+0:unsetting subscripted references unsets the referred elements
+># unset ref1[3]
+>typeset var0=1245
+># unset ref2[3]
+>typeset var0=1245
+># unset -n ref1[3]
+>typeset var0=1245
+># unset -n ref2[3]
+>typeset var0=1245
+
+ test-unset() {
+   typeset -r var=foo
+   typeset -n ref=var
+   typeset cmd=(unset $@); echo "#" $cmd; { $cmd 2>&1 } always { TRY_BLOCK_ERROR=0 }
+   typeset -p var
+ }
+ test-unset    var
+ test-unset -m var
+ test-unset    ref
+ test-unset -m ref
+ test-unset    var"[2]"
+ test-unset    ref"[2]"
+ test-unset -n ref"[2]"
+ unfunction test-unset
+0:unsetting read-only parameter triggers an error
+># unset var
+>test-unset:3: read-only variable: var
+>typeset -r var=foo
+># unset -m var
+>test-unset:3: read-only variable: var
+>typeset -r var=foo
+># unset ref
+>test-unset:3: read-only variable: var
+>typeset -r var=foo
+># unset -m ref
+>test-unset:3: read-only variable: var
+>typeset -r var=foo
+># unset var[2]
+>test-unset:3: read-only variable: var
+>typeset -r var=foo
+># unset ref[2]
+>test-unset:3: read-only variable: var
+>typeset -r var=foo
+># unset -n ref[2]
+>test-unset:3: read-only variable: var
+>typeset -r var=foo
+
+ test-unset() (
+   setopt restricted
+   typeset -n ifs=IFS
+   typeset cmd=(unset $@); echo "#" $cmd; { $cmd 2>&1 } always { TRY_BLOCK_ERROR=0 }
+   typeset -p IFS
+ )
+ test-unset    IFS
+ test-unset -m IFS
+ test-unset    ifs
+ test-unset -m ifs
+ test-unset    IFS"[2]"
+ test-unset    ifs"[2]"
+ test-unset -n ifs"[2]"
+ unfunction test-unset
+0:unsetting restricted parameter triggers an error
+># unset IFS
+>test-unset:3: IFS: restricted
+>typeset -g IFS=$' \t\n\C-@'
+># unset -m IFS
+>test-unset:3: IFS: restricted
+>typeset -g IFS=$' \t\n\C-@'
+># unset ifs
+>test-unset:3: IFS: restricted
+>typeset -g IFS=$' \t\n\C-@'
+># unset -m ifs
+>test-unset:3: IFS: restricted
+>typeset -g IFS=$' \t\n\C-@'
+># unset IFS[2]
+>test-unset:3: IFS: restricted
+>typeset -g IFS=$' \t\n\C-@'
+># unset ifs[2]
+>test-unset:3: IFS: restricted
+>typeset -g IFS=$' \t\n\C-@'
+># unset -n ifs[2]
+>test-unset:3: IFS: restricted
+>typeset -g IFS=$' \t\n\C-@'
+
+ test-unset() {
+   typeset -n ref1 ref2=ref1
+   typeset cmd=(unset $@); echo "#" $cmd; $cmd
+   typeset -p ref1 ref2
+ }
+ test-unset    ref1
+ test-unset    ref2
+ test-unset -m ref1
+ test-unset -m ref2
+ unfunction test-unset
+0:unsetting placeholder references or their referents has no effect
+># unset ref1
+>typeset -n ref1
+>typeset -n ref2=ref1
+># unset ref2
+>typeset -n ref1
+>typeset -n ref2=ref1
+># unset -m ref1
+>typeset -n ref1
+>typeset -n ref2=ref1
+># unset -m ref2
+>typeset -n ref1
+>typeset -n ref2=ref1
+
+ test-unset() {
+   typeset -n ref1=undefined ref2=ref1
+   typeset cmd=(unset $@); echo "#" $cmd; $cmd
+   typeset -p ref1 ref2
+ }
+ typeset -p undefined 2>&1
+ test-unset    ref1
+ test-unset    ref2
+ test-unset -m ref1
+ test-unset -m ref2
+ unfunction test-unset
+0:unsetting references to not-yet-defined variables or their referents has no effect
+>(eval):typeset:6: no such variable: undefined
+># unset ref1
+>typeset -n ref1=undefined
+>typeset -n ref2=ref1
+># unset ref2
+>typeset -n ref1=undefined
+>typeset -n ref2=ref1
+># unset -m ref1
+>typeset -n ref1=undefined
+>typeset -n ref2=ref1
+># unset -m ref2
+>typeset -n ref1=undefined
+>typeset -n ref2=ref1
+
+ test-unset() {
+   typeset -n refg1=g1 refl1=l1
+   () {
+     typeset -g g1=glb1 g2=glb2
+     typeset    l1=lcl1 l2=lcl2
+     () {
+       typeset -n refg2=g2 refl2=l2
+       typeset cmd=(unset $@ refg1 refg2 refl1 refl2); echo "#" $cmd; $cmd
+     } $@
+     typeset -p g1 g2 l1 l2 2>&1
+   } $@
+   unset g1 g2
+ }
+ test-unset
+ test-unset -m
+ unfunction test-unset
+0:unsetting references referring to parameters in enclosing scopes unsets the parameters
+># unset refg1 refg2 refl1 refl2
+>(anon):typeset:7: no such variable: g1
+>(anon):typeset:7: no such variable: g2
+># unset -m refg1 refg2 refl1 refl2
+>(anon):typeset:7: no such variable: g1
+>(anon):typeset:7: no such variable: g2
+
+ test-unset() {
+   typeset -g g=glb
+   typeset    l=lcl
+   typeset -n refg=g refl=l
+   () {
+     typeset g=hide-g
+     typeset l=hide-l
+     typeset cmd=(unset $@ refg refl); echo "#" $cmd; $cmd
+     echo "# inner scope"
+     typeset -p g l 2>&1
+   } $@
+   echo "# outer scope"
+   typeset -p g l 2>&1
+   unset g
+ }
+ test-unset
+ test-unset -m
+ unfunction test-unset
+0:unsetting references referring to hidden parameters unsets the hidden parameters
+># unset refg refl
+># inner scope
+>typeset g=hide-g
+>typeset l=hide-l
+># outer scope
+>test-unset:typeset:12: no such variable: g
+># unset -m refg refl
+># inner scope
+>typeset g=hide-g
+>typeset l=hide-l
+># outer scope
+>test-unset:typeset:12: no such variable: g
+
  typeset -n ref1
  typeset -n ref2
  typeset -n ref3=ref2