PATCH: Simplify resolve_nameref and setscope

[Philippe Altherr <philippe.altherr@xxxxxxxxx>]

The attached patch significantly decreases the size and complexity of resolve_nameref and setscope (and fixes a few small bugs). It was derived mainly by deleting dead and/or redundant code from these two functions. The many steps can be observed here.

The patch is built on top of a number of my other patches and thus can only be applied once those have been applied.

Philippe

diff --git a/Src/Modules/param_private.c b/Src/Modules/param_private.c
index 044617190..f2f5f4857 100644
--- a/Src/Modules/param_private.c
+++ b/Src/Modules/param_private.c
@@ -606,7 +606,7 @@ getprivatenode(HashTable ht, const char *nam)
     /* resolve nameref after skipping private parameters */
     if (pm && (pm->node.flags & PM_NAMEREF) &&
 	(pm->u.str || (pm->node.flags & PM_UNSET)))
-	pm = (Param) resolve_nameref(pm, NULL);
+	pm = resolve_nameref(pm, NULL, false);
 
     return (HashNode)pm;
 }
diff --git a/Src/builtin.c b/Src/builtin.c
index a39e7f33b..640d9e302 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2032,7 +2032,7 @@ typeset_single(char *cname, char *pname, Param pm, int func,
 
     if (pm && (pm->node.flags & PM_NAMEREF) && !((off|on) & PM_NAMEREF) &&
 	(pm->level == locallevel || !(on & PM_LOCAL))) {
-	if ((pm = (Param)resolve_nameref(pm, NULL)))
+	if ((pm = resolve_nameref(pm, NULL, false)))
 	    pname = pm->node.nam;
 	if (pm && (pm->node.flags & PM_NAMEREF) &&
 	    (on & ~(PM_NAMEREF|PM_LOCAL|PM_READONLY))) {
@@ -3913,7 +3913,8 @@ bin_unset(char *name, char **argv, Options ops, int func)
 	    returnval = 1;
 	} else if (ss) {
 	    if ((pm->node.flags & PM_NAMEREF) &&
-		(!(pm = (Param)resolve_nameref(pm, NULL)) || pm->width)) {
+		(!(pm = resolve_nameref(pm, NULL, false)) ||
+		 pm->width)) {
 		/* warning? */
 		continue;
 	    }
@@ -3957,7 +3958,7 @@ bin_unset(char *name, char **argv, Options ops, int func)
 	} else {
 	    if (!OPT_ISSET(ops,'n')) {
 		int ref = (pm->node.flags & PM_NAMEREF);
-		if (!(pm = (Param)resolve_nameref(pm, NULL)))
+		if (!(pm = resolve_nameref(pm, NULL, false)))
 		    continue;
 		if (ref && pm->level < locallevel &&
 		    !(pm->node.flags & PM_READONLY)) {
diff --git a/Src/params.c b/Src/params.c
index 46e790771..c093f34b7 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -556,7 +556,7 @@ getparamnode(HashTable ht, const char *nam)
 {
     HashNode hn = getparamnode_nofollow(ht, nam);
     if (hn && ht == realparamtab && !(hn->flags & PM_UNSET))
-	hn = resolve_nameref((Param)hn, NULL);
+	hn = (HashNode)resolve_nameref((Param)hn, NULL, false);
     return hn;
 }
 
@@ -1046,12 +1046,14 @@ createparam(char *name, int flags)
 	    (oldpm->level == locallevel ?
 	     !(oldpm->node.flags & PM_RO_BY_DESIGN) : !(flags & PM_LOCAL)) &&
 	    (oldpm->node.flags & PM_NAMEREF)) {
-	    Param lastpm;
-	    struct asgment stop;
-	    stop.flags = PM_NAMEREF | (flags & PM_LOCAL);
-	    stop.name = "";
-	    stop.value.scalar = NULL;
-	    lastpm = (Param)resolve_nameref(oldpm, &stop);
+	    /**
+	     * Here we only have to deal with namerefs that refer to
+	     * not-yet-defined or unset variable. All other namerefs
+	     * have already been taken care of by the resolve_nameref
+	     * in typeset_single. It's unclear why these can't be
+	     * handled there too.
+	     **/
+	    Param lastpm = resolve_nameref(oldpm, NULL, true);
 	    if (lastpm) {
 		if (lastpm->node.flags & PM_NAMEREF) {
 		    char *refname = GETREFNAME(lastpm);
@@ -3325,11 +3327,7 @@ assignsparam(char *s, char *val, int flags)
 	}
     }
 
-    if (v->pm->node.flags & PM_NAMEREF)
-	v->pm->node.flags |= PM_NEWREF;
     assignstrvalue(v, val, flags);
-    if (v->pm->node.flags & PM_NAMEREF)
-	v->pm->node.flags &= ~PM_NEWREF;
     unqueue_signals();
     return v->pm;
 }
@@ -6271,79 +6269,45 @@ printparamnode(HashNode hn, int printflags)
 }
 
 /**/
-mod_export HashNode
-resolve_nameref(Param pm, const Asgment stop)
+mod_export Param
+resolve_nameref(Param pm, const Param stop, const int keep_nameref)
 {
-    HashNode hn = (HashNode)pm;
-    const char *seek = stop ? stop->value.scalar : NULL;
-
-    if (pm && (pm->node.flags & PM_NAMEREF)) {
-	char *refname = GETREFNAME(pm);
-	if (pm->node.flags & (PM_UNSET|PM_TAGGED)) {
-	    /* Semaphore with createparam() */
-	    pm->node.flags &= ~PM_UNSET;
-	    if (pm->node.flags & PM_NEWREF)	/* See setloopvar() */
-		return NULL;
-	    if (refname && *refname && (pm->node.flags & PM_TAGGED))
-		pm->node.flags |= PM_SELFREF;	/* See setscope() */
-	    return (HashNode) pm;
-	} else if (refname) {
-	    if ((pm->node.flags & PM_TAGGED) ||
-		(stop && strcmp(refname, stop->name) == 0)) {
-		/* zwarnnam(refname, "invalid self reference"); */
-		return stop ? (HashNode)pm : NULL;
-	    }
-	    if (*refname)
-		seek = refname;
-	}
-    }
-    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 */
-	if (pm && (pm->node.flags & PM_NAMEREF) && pm->width) {
-	    if (stop) {
-		if (stop->flags & PM_NAMEREF)
-		    hn = (HashNode)pm;
-		else
-		    hn = NULL;
-	    } else {
-		/* this has to be the end of any chain */
-		hn = (HashNode)pm;	/* see fetchvalue() */
-	    }
-	} else if ((hn = gethashnode2(realparamtab, seek))) {
-	    if (pm) {
-		if (!(stop && (stop->flags & (PM_LOCAL)))) {
-		    if ((pm->node.flags & PM_NAMEREF) &&
-			(pm->node.flags & PM_UPPER))
-			hn = (HashNode)upscope_upper((Param)hn, pm->level - 1);
-		    else
-			hn = (HashNode)upscope((Param)hn,
-					       (pm->node.flags & PM_NAMEREF) ?
-					       (pm->base) : ((Param)hn)->level);
-		}
-		/* user can't tag a nameref, safe for loop detection */
-		pm->node.flags |= PM_TAGGED;
-	    }
-	    if (hn) {
-		if (hn->flags & PM_AUTOLOAD)
-		    hn = getparamnode(realparamtab, seek);
-		if (!(hn->flags & PM_UNSET))
-		    hn = resolve_nameref((Param)hn, stop);
-	    }
-	    if (pm)
-		pm->node.flags &= ~PM_TAGGED;
-	} else if (stop && (stop->flags & PM_NAMEREF))
-	    hn = (pm && (pm->node.flags & PM_NEWREF)) ? NULL : (HashNode)pm;
-	unqueue_signals();
+    if (!(pm->node.flags & PM_NAMEREF))
+	return (HashNode)pm;
+    if ((pm->node.flags & (PM_UNSET|PM_TAGGED))) {
+	/* Semaphore with createparam() */
+	pm->node.flags &= ~PM_UNSET;
+	return pm;
     }
 
-    return hn;
+    char *refname = GETREFNAME(pm);
+    if (!refname || !*refname)
+	return pm;
+
+    HashNode hn;
+    queue_signals();
+    /* pm->width is the offset of any subscript */
+    if (pm->width) {
+	/* this has to be the end of any chain */
+	hn = (HashNode)pm;	/* see fetchvalue() */
+    } else if ((hn = gethashnode2(realparamtab, refname))) {
+	if ((pm->node.flags & PM_UPPER))
+	    hn = (HashNode)upscope_upper((Param)hn, pm->level - 1);
+	else
+	    hn = (HashNode)upscope((Param)hn, pm->base);
+	if (hn && hn != (HashNode)stop) {
+	    /* user can't tag a nameref, safe for loop detection */
+	    pm->node.flags |= PM_TAGGED;
+	    if (hn->flags & PM_AUTOLOAD)
+		hn = getparamnode(realparamtab, refname);
+	    if (!(hn->flags & PM_UNSET))
+		hn = (HashNode)resolve_nameref((Param)hn, stop, keep_nameref);
+	    pm->node.flags &= ~PM_TAGGED;
+	}
+    } else if (keep_nameref)
+	hn = (HashNode)pm;
+    unqueue_signals();
+    return (Param)hn;
 }
 
 /**/
@@ -6361,10 +6325,7 @@ setloopvar(char *name, char *value)
       pm->base = pm->width = 0;
       SETREFNAME(pm, ztrdup(value));
       pm->node.flags &= ~PM_UNSET;
-      pm->node.flags |= PM_NEWREF;
       setscope(pm);
-      if (!errflag)
-	  pm->node.flags &= ~PM_NEWREF;
   } else
       setsparam(name, ztrdup(value));
 }
@@ -6374,9 +6335,8 @@ static void
 setscope(Param pm)
 {
     queue_signals();
-    if (pm->node.flags & PM_NAMEREF) do {
+    if (pm->node.flags & PM_NAMEREF) {
 	Param basepm;
-	struct asgment stop;
 	char *refname = GETREFNAME(pm);
 	char *t = refname ? itype_end(refname, INAMESPC, 0) : NULL;
 	int q = queue_signal_level();
@@ -6396,7 +6356,7 @@ setscope(Param pm)
 	/* Compute pm->base */
 	if (!(pm->node.flags & PM_UPPER) && refname &&
 	    (basepm = (Param)getparamnode_nofollow(realparamtab, refname)) &&
-	    (!(basepm->node.flags & PM_NEWREF) || (basepm = basepm->old))) {
+	    (!(basepm->old) || (basepm != pm) || (basepm = basepm->old))) {
 	    pm->base = basepm->level;
 	}
 	if (pm->base > pm->level) {
@@ -6410,49 +6370,18 @@ setscope(Param pm)
 	}
 
 	/* Check for self references */
-	stop.name = pm->node.nam;
-	stop.value.scalar = NULL;
-	stop.flags = PM_NAMEREF;
-	if (locallevel && !(pm->node.flags & PM_UPPER))
-	    stop.flags |= PM_LOCAL;
-	dont_queue_signals();	/* Prevent unkillable loops */
-	basepm = (Param)resolve_nameref(pm, &stop);
-	restore_queue_signals(q);
-	if (basepm) {
-	    if (basepm->node.flags & PM_NAMEREF) {
-		if (pm == basepm) {
-		    if (pm->node.flags & PM_SELFREF) {
-			/* Loop signalled by resolve_nameref() */
-			if (upscope(pm, pm->base) == pm) {
-			    zerr("%s: invalid self reference", refname);
-			    unsetparam_pm(pm, 0, 1);
-			    break;
-			}
-			pm->node.flags &= ~PM_SELFREF;
-		    } else if (pm->base == pm->level) {
-			if (refname && *refname &&
-			    strcmp(pm->node.nam, refname) == 0) {
-			    zerr("%s: invalid self reference", refname);
-			    unsetparam_pm(pm, 0, 1);
-			    break;
-			}
-		    }
-		} else if ((t = GETREFNAME(basepm))) {
-		    if (basepm->base <= basepm->level &&
-			strcmp(pm->node.nam, t) == 0) {
-			zerr("%s: invalid self reference", refname);
-			unsetparam_pm(pm, 0, 1);
-			break;
-		    }
-		}
+	if (refname && *refname) {
+	    if (basepm != pm && !(pm->width)) {
+		dont_queue_signals();	/* Prevent unkillable loops */
+		basepm = resolve_nameref(pm, pm, false);
+		restore_queue_signals(q);
+	    }
+	    if (basepm == pm) {
+		zerr("%s: invalid self reference", refname);
+		unsetparam_pm(pm, 0, 1);
 	    }
 	}
-	if (refname && upscope(pm, pm->base) == pm &&
-	    strcmp(pm->node.nam, refname) == 0) {
-	    zerr("%s: invalid self reference", refname);
-	    unsetparam_pm(pm, 0, 1);
-	}
-    } while (0);
+    };
     unqueue_signals();
 }
 
diff --git a/Src/zsh.h b/Src/zsh.h
index 4e5c02980..aa10145d2 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1935,9 +1935,6 @@ struct tieddata {
 #define PM_NAMEDDIR     (1<<29) /* has a corresponding nameddirtab entry    */
 #define PM_NAMEREF      (1<<30) /* pointer to a different parameter         */
 
-#define PM_SELFREF	PM_UNIQUE	/* Overload when namerefs resolved  */
-#define PM_NEWREF	PM_SINGLE	/* Overload in for-loop namerefs    */
-
 /* The option string corresponds to the first of the variables above */
 #define TYPESET_OPTSTR "aiEFALRZlurtxUhHT"
 
diff --git a/Test/K01nameref.ztst b/Test/K01nameref.ztst
index 584c25773..795b9e084 100644
--- a/Test/K01nameref.ztst
+++ b/Test/K01nameref.ztst
@@ -448,6 +448,18 @@ F:unexpected side-effects of previous tests
  ptr1=ptr2
  ptr2=ptr1
 1:looping assignment not allowed
+*?*invalid self reference
+
+ typeset var=foo
+ typeset var=foo
+ typeset -n ptr1=var
+ () {
+   typeset -n ptr1
+   typeset -n ptr2=ptr1
+   ptr1=ptr2
+   echo ptr1=$ptr1
+ }
+1:regression: reference loop with same name enclosing variable
 *?*invalid self reference
 
  typeset ptr1=not-a-ref
@@ -939,6 +951,17 @@ F:Checking for a bug in zmodload that affects later tests
 >typeset -n ref=two
 >typeset -n ref=var
 
+ typeset var=foo
+ typeset -n ref=var
+ () {
+   typeset -n ref
+   ref=""
+   ref=ref
+   echo $ref
+ }
+0:reference always prefers enclosing variable over itself
+>foo
+
  typeset -g .K01.scalar='RW'
  typeset -gA .K01.assoc=(x y)
  typeset -ga .K01.array=(z)