Zsh Mailing List Archive
Messages sorted by: Reverse Date, Date, Thread, Author

Re: [PATCHes] Fix issues in setscope

Here are updated patches that use the new function loadparamnode instead of getparamnode_nofollow.

- Don't follow namerefs when computing base scope of nameref (part 1).
- Don't follow namerefs when computing base scope of nameref (part 2).
- Only ever compute a base scope for non-upper references.

These are all prerequisites to the patch that simplifies resolve_nameref and setscope.

Philippe


On Tue, May 27, 2025 at 1:21 PM Philippe Altherr <philippe.altherr@xxxxxxxxx> wrote:
The attached 3 patches fix a number of issues in setscope. The second patch mainly reorders the computations in setscope. The two others should be relatively self-explanatory.

Philippe

diff --git a/Src/params.c b/Src/params.c
index dd30e5297..6945e73e9 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2239,10 +2239,10 @@ fetchvalue(Value v, char **pptr, int bracks, int flags)
 		}
 		Param p1 = (Param)gethashnode2(paramtab, ref);
 		if (p1) {
-		    int scope = ((pm->node.flags & PM_NAMEREF) ?
-				 ((pm->node.flags & PM_UPPER) ? -(pm->base) :
-				  pm->base) : locallevel);
-		    pm = upscope(p1, scope);
+		    if (pm->node.flags & PM_UPPER)
+			pm = upscope_upper(p1, pm->level - 1);
+		    else
+			pm = upscope(p1, pm->base);
 		    pm = (Param)loadparamnode(paramtab, pm, ref);
 		}
 		if (!(p1 && pm) ||
@@ -6318,12 +6318,13 @@ resolve_nameref(Param pm, const Asgment stop)
 	} else if ((hn = gethashnode2(realparamtab, seek))) {
 	    if (pm) {
 		if (!(stop && (stop->flags & (PM_LOCAL)))) {
-		    int scope = ((pm->node.flags & PM_NAMEREF) ?
-				 ((pm->node.flags & PM_UPPER) ?
-				  /* pm->base == 0 means not set yet */
-				  -(pm->base ? pm->base : pm->level) :
-				  pm->base) : ((Param)hn)->level);
-		    hn = (HashNode)upscope((Param)hn, scope);
+		    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);
 		}
 		hn = loadparamnode(paramtab, (Param)hn, seek);
 		/* user can't tag a nameref, safe for loop detection */
@@ -6391,14 +6392,11 @@ setscope(Param pm)
 	}
 
 	/* Compute pm->base */
-	if (pm->base < locallevel && refname &&
+	if (!(pm->node.flags & PM_UPPER) && refname &&
 	    (basepm = (Param)gethashnode2(realparamtab, refname)) &&
 	    (basepm = (Param)loadparamnode(realparamtab, basepm, refname)) &&
 	    (!(basepm->node.flags & PM_NEWREF) || (basepm = basepm->old))) {
 	    pm->base = basepm->level;
-	    if ((pm->node.flags & PM_UPPER) &&
-		(basepm = upscope(basepm, -(pm->level))))
-		pm->base = basepm->level;
 	}
 	if (pm->base > pm->level) {
 	    if (EMULATION(EMULATE_KSH)) {
@@ -6463,13 +6461,18 @@ upscope(Param pm, int reflevel)
 {
     Param up = pm->old;
     while (up && up->level >= reflevel) {
-	if (reflevel < 0 && up->level < -(reflevel))
-	    break;
 	pm = up;
 	up = up->old;
     }
-    if (reflevel < 0 && locallevel > 0)
-	return pm->level == locallevel ? up : pm;
+    return pm;
+}
+
+/**/
+mod_export Param
+upscope_upper(Param pm, int reflevel)
+{
+    while (pm && pm->level > reflevel)
+	pm = pm->old;
     return pm;
 }
 
diff --git a/Test/K01nameref.ztst b/Test/K01nameref.ztst
index 14b3e8b95..b03487d03 100644
--- a/Test/K01nameref.ztst
+++ b/Test/K01nameref.ztst
@@ -1100,15 +1100,15 @@ F:previously this could create an infinite recursion and crash
 >h:1: rs= - ra= - rs1= - ra1=
 >h:2: rs= - ra= - rs1= - ra1=
 >i:1: rs= - ra= - rs1= - ra1=
->i:2: rs=g - ra=g - rs1=g - ra1=g
->j:1: rs=g - ra=g - rs1=g - ra1=g
->j:2: rs=g - ra=g - rs1=g - ra1=g
->i:3: rs=g - ra=g - rs1=g - ra1=g
->k:1: rs=g - ra=g - rs1=g - ra1=g
->k:2: rs=g - ra=g - rs1=g - ra1=g
->h:3: rs=g - ra=g - rs1=g - ra1=g
->k:1: rs=g - ra=g - rs1=g - ra1=g
->k:2: rs=g - ra=g - rs1=g - ra1=g
+>i:2: rs=f - ra=f - rs1=f - ra1=f
+>j:1: rs=f - ra=f - rs1=f - ra1=f
+>j:2: rs=f - ra=f - rs1=f - ra1=f
+>i:3: rs=f - ra=f - rs1=f - ra1=f
+>k:1: rs=f - ra=f - rs1=f - ra1=f
+>k:2: rs=f - ra=f - rs1=f - ra1=f
+>h:3: rs=f - ra=f - rs1=f - ra1=f
+>k:1: rs=f - ra=f - rs1=f - ra1=f
+>k:2: rs=f - ra=f - rs1=f - ra1=f
 >g:3: rs=f - ra=f - rs1=f - ra1=f
 
  e '' 6

diff --git a/Src/params.c b/Src/params.c
index 70042a417..e0d883597 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -6426,7 +6426,9 @@ setscope(Param pm)
 		    pm->base = basepm->level;
 	    }
 	} else if (pm->base < locallevel && refname &&
-		   (basepm = (Param)getparamnode(realparamtab, refname))) {
+		   (basepm = (Param)gethashnode2(realparamtab, refname)) &&
+		   (basepm = (Param)loadparamnode(realparamtab, basepm, refname)) &&
+		   (!(basepm->node.flags & PM_NEWREF) || (basepm = basepm->old))) {
 	    pm->base = basepm->level;
 	    if ((pm->node.flags & PM_UPPER) &&
 		(basepm = upscope(basepm, -(pm->level))))
diff --git a/Test/K01nameref.ztst b/Test/K01nameref.ztst
index 52aed0af0..80c985798 100644
--- a/Test/K01nameref.ztst
+++ b/Test/K01nameref.ztst
@@ -1237,4 +1237,21 @@ F:previously this could create an infinite recursion and crash
 ?(eval):zsh/random:6: error when adding parameter `SRANDOM'
 ?(eval):6: autoloading module zsh/random failed to define parameter: SRANDOM
 
+ () {
+   typeset var1=var1
+   typeset var2=var2
+   typeset -n ref1=var1
+   echo "ref1=$ref1";
+   () {
+     typeset -n ref1=var2
+     typeset -n ref2=ref1
+     echo "ref1=$ref1";
+     echo "ref2=$ref2";
+   }
+ }
+0:regression: don't follow references when computing base scope - part 1
+>ref1=var1
+>ref1=var2
+>ref2=var2
+
 %clean

diff --git a/Src/params.c b/Src/params.c
index e0d883597..dd30e5297 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -3324,7 +3324,11 @@ 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;
 }
@@ -6374,12 +6378,40 @@ setscope(Param pm)
 	char *t = refname ? itype_end(refname, INAMESPC, 0) : NULL;
 	int q = queue_signal_level();
 
+	/* Compute pm->width */
 	/* Temporarily change nameref to array parameter itself */
 	if (t && *t == '[')
 	    *t = 0;
 	else
 	    t = 0;
-	stop.name = "";
+	if (t) {
+	    pm->width = t - refname;
+	    *t = '[';
+	    refname = dupstrpfx(refname, pm->width);
+	}
+
+	/* Compute pm->base */
+	if (pm->base < locallevel && refname &&
+	    (basepm = (Param)gethashnode2(realparamtab, refname)) &&
+	    (basepm = (Param)loadparamnode(realparamtab, basepm, refname)) &&
+	    (!(basepm->node.flags & PM_NEWREF) || (basepm = basepm->old))) {
+	    pm->base = basepm->level;
+	    if ((pm->node.flags & PM_UPPER) &&
+		(basepm = upscope(basepm, -(pm->level))))
+		pm->base = basepm->level;
+	}
+	if (pm->base > pm->level) {
+	    if (EMULATION(EMULATE_KSH)) {
+		zerr("%s: global reference cannot refer to local variable",
+		      pm->node.nam);
+		unsetparam_pm(pm, 0, 1);
+	    } else if (isset(WARNNESTEDVAR))
+		zwarn("reference %s in enclosing scope set to local variable %s",
+		      pm->node.nam, refname);
+	}
+
+	/* Check for self references */
+	stop.name = pm->node.nam;
 	stop.value.scalar = NULL;
 	stop.flags = PM_NAMEREF;
 	if (locallevel && !(pm->node.flags & PM_UPPER))
@@ -6387,11 +6419,6 @@ setscope(Param pm)
 	dont_queue_signals();	/* Prevent unkillable loops */
 	basepm = (Param)resolve_nameref(pm, &stop);
 	restore_queue_signals(q);
-	if (t) {
-	    pm->width = t - refname;
-	    *t = '[';
-	    refname = dupstrpfx(refname, pm->width);
-	}
 	if (basepm) {
 	    if (basepm->node.flags & PM_NAMEREF) {
 		if (pm == basepm) {
@@ -6419,29 +6446,7 @@ setscope(Param pm)
 			break;
 		    }
 		}
-	    } else if (!pm->base) {
-		pm->base = basepm->level;
-		if ((pm->node.flags & PM_UPPER) &&
-		    (basepm = upscope(basepm, -(pm->level))))
-		    pm->base = basepm->level;
 	    }
-	} else if (pm->base < locallevel && refname &&
-		   (basepm = (Param)gethashnode2(realparamtab, refname)) &&
-		   (basepm = (Param)loadparamnode(realparamtab, basepm, refname)) &&
-		   (!(basepm->node.flags & PM_NEWREF) || (basepm = basepm->old))) {
-	    pm->base = basepm->level;
-	    if ((pm->node.flags & PM_UPPER) &&
-		(basepm = upscope(basepm, -(pm->level))))
-		pm->base = basepm->level;
-	}
-	if (pm->base > pm->level) {
-	    if (EMULATION(EMULATE_KSH)) {
-		zerr("%s: global reference cannot refer to local variable",
-		      pm->node.nam);
-		unsetparam_pm(pm, 0, 1);
-	    } else if (isset(WARNNESTEDVAR))
-		zwarn("reference %s in enclosing scope set to local variable %s",
-		      pm->node.nam, refname);
 	}
 	if (refname && upscope(pm, pm->base) == pm &&
 	    strcmp(pm->node.nam, refname) == 0) {
diff --git a/Test/K01nameref.ztst b/Test/K01nameref.ztst
index 80c985798..14b3e8b95 100644
--- a/Test/K01nameref.ztst
+++ b/Test/K01nameref.ztst
@@ -1252,6 +1252,24 @@ F:previously this could create an infinite recursion and crash
 0:regression: don't follow references when computing base scope - part 1
 >ref1=var1
 >ref1=var2
+>ref2=var2
+
+ () {
+   typeset var1=var1
+   typeset -n ref1=var1
+   echo ref1=$ref1;
+   () {
+     typeset var2=var2
+     typeset -n ref1
+     typeset -n ref2=ref1
+     ref1=var2
+     echo ref1=$ref1;
+     echo ref2=$ref2;
+   }
+ }
+0:regression: don't follow references when computing base scope - part 2
+>ref1=var1
+>ref1=var2
 >ref2=var2
 
 %clean
Messages sorted by: Reverse Date, Date, Thread, Author