Re: A solution to fix hidden references in reference chains

[Philippe Altherr <philippe.altherr@xxxxxxxxx>]

Here is a patch that implements this (built on top of the patches to fix loading of autoload variable accessed via a reference and to fix issues in setscope):

- Fix handling of dangling references to work for all of them.

Philippe

On Sat, May 24, 2025 at 11:38 AM Philippe Altherr <philippe.altherr@xxxxxxxxx> wrote:

Whenever the "base" field of a "Param" is set, if "base" is strictly greater than the "level" field, we know the "Param" refers to a nested variable. We also know that its "base" will have to be updated when the "base" scope is exited. We can therefore add the "Param" to a list linked to the "base" scope.

Whenever a scope is exited, first check whether any "Param"s were added to its list. If yes, the "base" fields of these are updated (if the "Param"s are still named references). Then call a "scanendscope" that doesn't do anything special for named references. Or do it in the opposite order?

This assumes that "Params" are only ever freed by "scanendscope". If that's not the case, things get more complicated as one would also sometimes have to remove "Param"s from the scope lists.

I could try to prepare a patch that implements this but I would first need advice on the preferred way to implement the lists of "Param"s (linked list, new "Param" field, ...) and where/how to store them (global array, under special names in the parameter table, ...).

As a reminder, this would fix "ref2" in this test such that the output for "ref3" is the same as the one for "ref1".

Philippe

diff --git a/Src/params.c b/Src/params.c
index 6945e73e9..9f79831be 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -487,6 +487,24 @@ static initparam argvparam_pm = IPDEF9("", &pparams, NULL, \
 
 static Param argvparam;
 
+/*
+ * Lists of references to nested variables ("Param" instances) indexed
+ * by scope. Whenever the "base" scope of a named reference is set to
+ * refer to a variable more deeply nested than the reference itself
+ * ("base > level"), the "base" scope has to be updated once the
+ * "base" scope ends. The "scoperefs" lists keep track of these
+ * references. Since "Param" instances get reused when variables with
+ * the same name are redefined in the same scope, listed "Param"
+ * instances may no longer be references when the scope ends or may
+ * refer to a different "base" scope. A given "Param" instance may
+ * also be included in multiple lists at the same time or multiple
+ * 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.
+ */
+static LinkList *scoperefs = NULL;
+static int scoperefs_num = 0;
+
 /* "parameter table" - hash table containing the parameters
  *
  * realparamtab always points to the shell's global table.  paramtab is sometimes
@@ -5806,7 +5824,12 @@ static int lc_update_needed;
 mod_export void
 endparamscope(void)
 {
+    LinkList refs = locallevel < scoperefs_num ? scoperefs[locallevel] : NULL;
     queue_signals();
+    for (Param pm; refs && (pm = (Param)getlinknode(refs));) {
+	if ((pm->node.flags & PM_NAMEREF) && pm->base == locallevel)
+	    setscope_base(pm, locallevel - 1);
+    }
     locallevel--;
     /* This pops anything from a higher locallevel */
     saveandpophiststack(0, HFILE_USE_OPTIONS);
@@ -5841,9 +5864,7 @@ static void
 scanendscope(HashNode hn, UNUSED(int flags))
 {
     Param pm = (Param)hn;
-    Param hidden = NULL;
     if (pm->level > locallevel) {
-	hidden = pm->old;
 	if ((pm->node.flags & (PM_SPECIAL|PM_REMOVABLE)) == PM_SPECIAL) {
 	    /*
 	     * Removable specials are normal in that they can be removed
@@ -5906,14 +5927,6 @@ scanendscope(HashNode hn, UNUSED(int flags))
 		export_param(pm);
 	} else
 	    unsetparam_pm(pm, 0, 0);
-	pm = NULL;
-    }
-    if (hidden)
-	pm = hidden;
-    if (pm && (pm->node.flags & PM_NAMEREF) &&
-	       pm->base >= pm->level && pm->base >= locallevel) {
-	/* Should never get here for a -u reference */
-	pm->base = locallevel;
     }
 }
 
@@ -6396,7 +6409,7 @@ setscope(Param pm)
 	    (basepm = (Param)gethashnode2(realparamtab, refname)) &&
 	    (basepm = (Param)loadparamnode(realparamtab, basepm, refname)) &&
 	    (!(basepm->node.flags & PM_NEWREF) || (basepm = basepm->old))) {
-	    pm->base = basepm->level;
+	    setscope_base(pm, basepm->level);
 	}
 	if (pm->base > pm->level) {
 	    if (EMULATION(EMULATE_KSH)) {
@@ -6455,6 +6468,25 @@ setscope(Param pm)
     unqueue_signals();
 }
 
+/**/
+static void
+setscope_base(Param pm, int base)
+{
+    if ((pm->base = base) > pm->level) {
+	LinkList refs;
+	if (base >= scoperefs_num) {
+	    int old_num = scoperefs_num;
+	    int new_num = scoperefs_num = MAX(2 * base, 8);
+	    scoperefs = zrealloc(scoperefs, new_num * sizeof(refs));
+	    memset(scoperefs + old_num, 0, (new_num - old_num) * sizeof(refs));
+	}
+	refs = scoperefs[base];
+	if (!refs)
+	    refs = scoperefs[base] = znewlinklist();
+	zpushnode(refs, pm);
+    }
+}
+
 /**/
 mod_export Param
 upscope(Param pm, int reflevel)
diff --git a/Test/K01nameref.ztst b/Test/K01nameref.ztst
index b03487d03..5abc5c666 100644
--- a/Test/K01nameref.ztst
+++ b/Test/K01nameref.ztst
@@ -1133,8 +1133,8 @@ F:previously this could create an infinite recursion and crash
 0:Transitive references with scoping changes
 >f4: ref1=f4 ref2=XX ref3=f4
 >f3: ref1=f3 ref2=XX ref3=f3
->g5: ref1=f3 ref2=XX ref3=g4
->g4: ref1=f3 ref2=XX ref3=g4
+>g5: ref1=f3 ref2=XX ref3=f3
+>g4: ref1=f3 ref2=XX ref3=f3
 >f3: ref1=f3 ref2=XX ref3=f3
 >f2: ref1=f1 ref2=XX ref3=f1
 >f1: ref1=f1 ref2=f1 ref3=f1