[PATCH] Rectify "private" and nofork

[PATCH] Rectify "private" and nofork

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

This is a follow-up to workers/52510.

The patch permits nofork substitutions ("current shell command
substitutions") to access private parameters in the calling function
scope, just as other current shell constructs like brace commands are
able to.  However, private parameters declared within a nofork
substitution are not accessible by nested nofork substitutions.

Also fixed is an existing crash bug on
  explosive { exec {kaboom}>&2 }
  detonator() {
    private kaboom
    explosive
  }

Fixes/adds tests for using "private" along with TYPESET_TO_UNSET, plus
tests and doc for all of the above.

[-- Attachment #2: private-nofork.txt --]
[-- Type: text/plain, Size: 11059 bytes --]

diff --git a/Doc/Zsh/mod_private.yo b/Doc/Zsh/mod_private.yo
index 69a5f58be..08ac4cafe 100644
--- a/Doc/Zsh/mod_private.yo
+++ b/Doc/Zsh/mod_private.yo
@@ -84,9 +84,14 @@ created outside the local scope when it was not previously declared.)
 itemiz(An exported private remains in the environment of inner scopes but
 appears unset for the current shell in those scopes.  Generally, exporting
 private parameters should be avoided.)
+itemiz(Current shell command substitutions such as `tt(${|)...tt(})',
+`tt(${|)var(var)tt(|)...tt(})' and `tt(${ )...tt( })' may read and assign
+private parameters from the enclosing function.)
 itemiz(Declaring a private parameter in a current shell command substitution
-such as `tt(${ )...tt( })' limits the parameter to the scope of the command
-substitution, just as if the parameter were declared in a function.)
+limits that parameter to the scope of the command substitution, just as if
+the parameter were declared in a function.  This also prevents access by
+any enclosed current shell command substitutions, but other substitutions
+may use the private parameter because those have the same calling scope.)
 enditemize()
 
 Note that this differs from the static scope defined by compiled languages
diff --git a/Src/Modules/param_private.c b/Src/Modules/param_private.c
index 7ef6633da..5003d4627 100644
--- a/Src/Modules/param_private.c
+++ b/Src/Modules/param_private.c
@@ -298,7 +298,7 @@ pps_setfn(Param pm, char *x)
 {
     struct gsu_closure *c = (struct gsu_closure *)(pm->gsu.s);
     GsuScalar gsu = (GsuScalar)(c->g);
-    if (locallevel == pm->level)
+    if (locallevel == pm->level || locallevel > private_wraplevel)
 	gsu->setfn(pm, x);
     else
 	setfn_error(pm);
@@ -338,7 +338,7 @@ ppi_setfn(Param pm, zlong x)
 {
     struct gsu_closure *c = (struct gsu_closure *)(pm->gsu.i);
     GsuInteger gsu = (GsuInteger)(c->g);
-    if (locallevel == pm->level)
+    if (locallevel == pm->level || locallevel > private_wraplevel)
 	gsu->setfn(pm, x);
     else
 	setfn_error(pm);
@@ -378,7 +378,7 @@ ppf_setfn(Param pm, double x)
 {
     struct gsu_closure *c = (struct gsu_closure *)(pm->gsu.f);
     GsuFloat gsu = (GsuFloat)(c->g);
-    if (locallevel == pm->level)
+    if (locallevel == pm->level || locallevel > private_wraplevel)
 	gsu->setfn(pm, x);
     else
 	setfn_error(pm);
@@ -419,7 +419,7 @@ ppa_setfn(Param pm, char **x)
 {
     struct gsu_closure *c = (struct gsu_closure *)(pm->gsu.a);
     GsuArray gsu = (GsuArray)(c->g);
-    if (locallevel == pm->level)
+    if (locallevel == pm->level || locallevel > private_wraplevel)
 	gsu->setfn(pm, x);
     else
 	setfn_error(pm);
@@ -461,7 +461,7 @@ pph_setfn(Param pm, HashTable x)
 {
     struct gsu_closure *c = (struct gsu_closure *)(pm->gsu.h);
     GsuHash gsu = (GsuHash)(c->g);
-    if (locallevel == pm->level)
+    if (locallevel == pm->level || locallevel > private_wraplevel)
 	gsu->setfn(pm, x);
     else
 	setfn_error(pm);
@@ -539,19 +539,20 @@ static struct funcwrap wrapper[] = {
     WRAPDEF(wrap_private)
 };
 
+/**/
+static int private_wraplevel = 0;
+
 /**/
 static int
 wrap_private(Eprog prog, FuncWrap w, char *name)
 {
-    static int wraplevel = 0;
-
-    if (wraplevel < locallevel /* && strcmp(name, "(anon)") != 0 */) {
-	int owl = wraplevel;
-	wraplevel = locallevel;
+    if (private_wraplevel < locallevel /* && strcmp(name, "(anon)") != 0 */) {
+	int owl = private_wraplevel;
+	private_wraplevel = locallevel;
 	scanhashtable(paramtab, 0, 0, 0, scopeprivate, PM_UNSET);
 	runshfunc(prog, w, name);
 	scanhashtable(paramtab, 0, 0, 0, scopeprivate, 0);
-	wraplevel = owl;
+	private_wraplevel = owl;
 	return 0;
     }
     return 1;
@@ -573,22 +574,32 @@ getprivatenode(HashTable ht, const char *nam)
 	pm = (Param) hn;
 	/* how would an autoloaded private behave?  return here? */
     }
-    while (!fakelevel && pm && locallevel > pm->level && is_private(pm)) {
+    while (!fakelevel && pm && is_private(pm) && locallevel > pm->level) {
+	if (pm->level == private_wraplevel + 1) {
+	    /* Variable is in the current function scope */
+	    break;
+	}
+#if 0
 	if (!(pm->node.flags & PM_UNSET)) {
 	    /*
 	     * private parameters are always marked PM_UNSET before we
-	     * increment locallevel, so the only way we get here is
-	     * when createparam() wants a new parameter that is not at
-	     * the current locallevel and it has therefore cleared the
-	     * PM_UNSET flag.
+	     * increment locallevel, so there are three possible ways
+	     * to get here:
+	     *  1) createparam() wants a new parameter that is not at
+	     *  the current locallevel and it has therefore cleared the
+	     *  PM_UNSET flag
+	     *  2) locallevel has been incremented (startparamscope())
+	     *  outside the usual function call stack (private_wraplevel)
+	     *  3) dynamic scoping is fetching a value from a surrounding
+	     *  scope, we don't know if that's for assign or just expand
+	     * The first of those is now caught in createparam() when
+	     * testing PM_RO_BY_DESIGN and the second occurs only in
+	     * nofork substitution or handling of ZLE specials.  If the
+	     * third is an assignment, the GSU setfn rejects it.
 	     */
 	    DPUTS(pm->old, "BUG: PM_UNSET cleared in wrong scope");
-	    setfn_error(pm);
-	    /*
-	     * TODO: instead of throwing an error here, create a global
-	     * parameter, insert as pm->old, handle WARN_CREATE_GLOBAL.
-	     */
 	}
+#endif
 	pm = pm->old;
     }
 
diff --git a/Src/params.c b/Src/params.c
index 9f0cbcd67..a722a20f6 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -1049,7 +1049,7 @@ createparam(char *name, int flags)
 		/* POSIXBUILTINS horror: we need to retain 'export' flags */
 		(isset(POSIXBUILTINS) && (oldpm->node.flags & PM_EXPORTED))) {
 		if (oldpm->node.flags & PM_RO_BY_DESIGN) {
-		    zerr("%s: can't change parameter attribute",
+		    zerr("%s: can't modify read-only parameter",
 			 name);
 		    return NULL;
 		}
@@ -3615,9 +3615,18 @@ assignnparam(char *s, mnumber val, int flags)
 	pm = createparam(t, ss ? PM_ARRAY :
 			 isset(POSIXIDENTIFIERS) ? PM_SCALAR :
 			 (val.type & MN_INTEGER) ? PM_INTEGER : PM_FFLOAT);
-	if (!pm)
-	    pm = (Param) paramtab->getnode(paramtab, t);
-	DPUTS(!pm, "BUG: parameter not created");
+	if (errflag) {
+	    /* assume error message already output */
+	    unqueue_signals();
+	    return NULL;
+	}
+	if (!pm && !(pm = (Param) paramtab->getnode(paramtab, t))) {
+	    DPUTS(!pm, "BUG: parameter not created");
+	    if (!errflag)
+		zerr("%s: parameter not found", t);
+	    unqueue_signals();
+	    return NULL;
+	}
 	if (ss) {
 	    *ss = '[';
 	} else if (val.type & MN_INTEGER) {
diff --git a/Test/V10private.ztst b/Test/V10private.ztst
index d902cac56..9eeda0f47 100644
--- a/Test/V10private.ztst
+++ b/Test/V10private.ztst
@@ -10,6 +10,8 @@
    sed -e 's,# test_zsh_param_private,zmodload zsh/param/private,' < $ZTST_srcdir/B02typeset.ztst > private.TMP/B02
  fi
 
+ setopt TYPESET_TO_UNSET
+
 %test
 
  (zmodload -u zsh/param/private && zmodload zsh/param/private)
@@ -246,7 +248,7 @@ F:note "typeset" rather than "private" in output from outer
 1:privates are not visible in anonymous functions, part 3
 >X top level
 >array_test not set
-?(anon):4: array_test: can't change parameter attribute
+?(anon):4: array_test: can't modify read-only parameter
 F:future revision will create a global with this assignment
 
  typeset -a array_test
@@ -311,11 +313,30 @@ F:future revision will create a global with this assignment
 
  () {
    typeset -n ptr1=ptr2
-   private -n ptr2
+   private -n ptr2	# TYPESET_TO_UNSET makes this not a "placeholder"
    typeset -p ptr1 ptr2
    typeset val=LOCAL
    () {
-     ptr1=val
+     ptr1=val		# Test dies here as ptr2 is private and unset
+     typeset -n
+     printf "%s=%s\n" ptr1 "$ptr1" ptr2 "$ptr2"
+   }
+   typeset -p ptr1 ptr2
+ }
+ typeset -p ptr2
+1:up-reference for private namerefs, end unset and not in scope
+F:See K01nameref.ztst up-reference part 5
+F:Here ptr1 finds private ptr2 by scope mismatch
+>typeset -n ptr1=ptr2
+*?*read-only variable: ptr2
+
+ () {
+   typeset -n ptr1=ptr2
+   private -n ptr2=	# Assignment makes this a placeholder, not unset
+   typeset -p ptr1 ptr2
+   typeset val=LOCAL
+   () {
+     ptr1=val		# This is a silent no-op, why?
      typeset -n
      printf "%s=%s\n" ptr1 "$ptr1" ptr2 "$ptr2"
    }
@@ -323,8 +344,9 @@ F:future revision will create a global with this assignment
  }
  typeset -p ptr2
 1:up-reference for private namerefs, end not in scope
-F:See K01typeset.ztst up-reference part 5
-F:Here ptr1 finds private ptr2 by scope mismatch, assignment silently fails
+F:See K01nameref.ztst up-reference part 5
+F:Here ptr1 finds private ptr2 by scope mismatch
+F:Assignment silently fails, is that correct?
 >typeset -n ptr1=ptr2
 >ptr1=ptr2
 >ptr1=
@@ -335,11 +357,11 @@ F:Here ptr1 finds private ptr2 by scope mismatch, assignment silently fails
  typeset ptr2
  () {
    typeset -n ptr1=ptr2
-   private -n ptr2
+   private -n ptr2	# Set/unset is irrelevant, not referenced
    typeset -p ptr1 ptr2
    typeset val=LOCAL
    () {
-     ptr1=val;
+     ptr1=val
      typeset -n
      printf "%s=%s\n" ptr1 "$ptr1" ptr2 "$ptr2"
    }
@@ -401,6 +423,88 @@ F:Should we allow "public" namerefs to private parameters?
 1:private may not change parameter type
 ?(anon):private:2: can't change type of private param: x
 
+ () {
+   private fd1 fd2
+   exec {fd1}>&1
+   print OK
+   () { exec {fd2}>&2 }
+   print BAD $fd2
+ }
+1:redirection cannot assign private in wrong scope
+F:Better if caught in checkclobberparam() but exec.c doesn't know scope
+>OK
+?(anon): fd2: can't modify read-only parameter
+
+ () {
+   private z=outer
+   print ${(t)z} $z
+   print ${|
+    print ${(t)z} $z
+    REPLY=$z
+   }
+ }
+0:nofork may read private in calling function
+>scalar-local-hide-special outer
+>scalar-local-hide-special outer
+>outer
+
+ () {
+   private z=outer
+   print ${(t)z} $z
+   print ${| REPLY=${|z| z=nofork} }
+   print ${(t)z} $z
+ }
+0:nofork may write to private in calling function
+>scalar-local-hide-special outer
+>nofork
+>scalar-local-hide-special nofork
+
+ () {
+   local q=outer
+   print ${|
+     private q=nofork
+     REPLY=${| REPLY=$q}
+   }
+ }
+0:nofork cannot see private in surrounding nofork
+>outer
+
+ () {
+   private z=outer
+   print ${(t)z} $z
+   print ${|z|
+     private q
+     z=${|q| q=nofork}
+   }
+   print ${(t)z} $z
+ }
+1:nofork may not change private in surrounding nofork
+>scalar-local-hide-special outer
+*?*: q: can't modify read-only parameter
+
+ () {
+   private q=outer
+   print ${|
+     () { REPLY="{$q}" }
+   }
+   print ${|q|
+     () { q=nofork }
+   }
+ }
+1:function may not access private from inside nofork
+>{}
+*?*: q: can't modify read-only parameter
+
+ () {
+   print ${|
+     private q
+     () { q=nofork }
+   }
+ }
+1:function may not access private declared in nofork
+*?*: q: can't modify read-only parameter
+
 %clean
 
+  unsetopt TYPESET_TO_UNSET
   rm -r private.TMP