Re: [BUG] zsh/param/private scoping error

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Wed, Sep 1, 2021 at 10:35 AM Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx> wrote:
>
> Something else has changed since the implementation of private, I
> think, because this is not what I expected:
>
> top () { private foo=top; mid }
> mid () { typeset -g foo=middle; bot }
> bot () { print $foo }
> functions -t top

The attached patch addresses this, producing the output I expected:

+top:0> private foo=top
+top:0> mid
+mid:0> typeset -g foo=middle
+mid:0> bot
+bot:0> print middle
middle

It also improves the error message in the event that a typeset
attempts to change the scope or properties of a private parameter.

> Even
> without private, "typeset -g" only reaches as far upwards as the most
> recent local declaration of the name.

A documentation patch to better explain this is included.  As a bonus,
there's also an explanation of why some parameters do not appear in
"typeset -p" output.

diff --git a/Doc/Zsh/mod_private.yo b/Doc/Zsh/mod_private.yo
index 78aee0acf..6da18f814 100644
--- a/Doc/Zsh/mod_private.yo
+++ b/Doc/Zsh/mod_private.yo
@@ -10,12 +10,16 @@ ifnzman()
 startitem()
 findex(private)
 cindex(private parameter, creating)
-item(tt(private) [ {tt(PLUS())|tt(-)}tt(AHUahlprtux) ] \
+item(tt(private) [ {tt(PLUS())|tt(-)}tt(AHUahlmrtux) ] \
 [ {tt(PLUS())|tt(-)}tt(EFLRZi) [ var(n) ] ] [ var(name)[tt(=)var(value)] ... ])(
 The tt(private) builtin accepts all the same options and arguments as tt(local)
 (ifzman(zmanref(zshbuiltins))ifnzman(noderef(Shell Builtin Commands))) except
 for the `tt(-)tt(T)' option.  Tied parameters may not be made private.
 
+The `tt(-)tt(p)' option is presently a no-op because the state of
+private parameters cannot be reliably be reloaded.  This also applies
+to printing private parameters with `tt(typeset -p)'.
+
 If used at the top level (outside a function scope), tt(private) creates a
 normal parameter in the same manner as tt(declare) or tt(typeset).  A
 warning about this is printed if tt(WARN_CREATE_GLOBAL) is set
@@ -75,7 +79,8 @@ itemiz(Within any other function called by the declaring function, the
 private parameter does em(NOT) hide other parameters of the same name, so
 for example a global parameter of the same name is visible and may be
 assigned or unset.  This includes calls to anonymous functions, although
-that may also change in the future.)
+that may also change in the future.  However, the private name may not be
+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.)
diff --git a/Doc/Zsh/params.yo b/Doc/Zsh/params.yo
index b514eb072..a88e44d4f 100644
--- a/Doc/Zsh/params.yo
+++ b/Doc/Zsh/params.yo
@@ -632,6 +632,14 @@ In the parameter lists that follow, the mark `<S>' indicates that the
 parameter is special.  `<Z>' indicates that the parameter does not exist
 when the shell initializes in tt(sh) or tt(ksh) emulation mode.
 
+The parameters `tt(!)', `tt(#)', `tt(*)', `tt(-)', `tt(?)', `tt(@)',
+`tt($)', `tt(ARGC)', `tt(HISTCMD)', `tt(LINENO)', `tt(PPID)',
+`tt(status)', `tt(TTYIDLE)', `tt(zsh_eval_context)',
+`tt(ZSH_EVAL_CONTEXT)', and `tt(ZSH_SUBSHELL)' are read-only and thus
+cannot be restored by the user, so they are not output by
+`tt(typeset -p)'.  This also applies to many read-only parameters loaded
+from modules.
+
 The following parameters are automatically set by the shell:
 
 startitem()
diff --git a/Src/Modules/param_private.c b/Src/Modules/param_private.c
index 24545819d..c53839152 100644
--- a/Src/Modules/param_private.c
+++ b/Src/Modules/param_private.c
@@ -125,7 +125,7 @@ makeprivate(HashNode hn, UNUSED(int flags))
 	    break;
 	}
 	/* PM_HIDE so new parameters in deeper scopes do not shadow */
-	pm->node.flags |= (PM_HIDE|PM_SPECIAL|PM_REMOVABLE);
+	pm->node.flags |= (PM_HIDE|PM_SPECIAL|PM_REMOVABLE|PM_RO_BY_DESIGN);
 	pm->level -= 1;
     }
 }
@@ -171,7 +171,7 @@ bin_private(char *nam, char **args, LinkList assigns, Options ops, int func)
 {
     int from_typeset = 1;
     int ofake = fakelevel;	/* paranoia in case of recursive call */
-    int hasargs = *args != NULL || (assigns && firstnode(assigns));
+    int hasargs = /* *args != NULL || */ (assigns && firstnode(assigns));
     makeprivate_error = 0;
 
     if (!OPT_ISSET(ops, 'P')) {
@@ -190,8 +190,10 @@ bin_private(char *nam, char **args, LinkList assigns, Options ops, int func)
 	return bin_typeset("private", args, assigns, ops, func);
     }
 
-    ops->ind['g'] = 2;	/* force bin_typeset() to behave as "local" */
-    if (OPT_ISSET(ops, 'p') || (!hasargs && OPT_ISSET(ops, '+'))) {
+    if (!(OPT_ISSET(ops, 'm') || OPT_ISSET(ops, '+')))
+	ops->ind['g'] = 2;	/* force bin_typeset() to behave as "local" */
+    if (OPT_ISSET(ops, 'p') || OPT_ISSET(ops, 'm') ||
+	(!hasargs && OPT_ISSET(ops, '+'))) {
 	return bin_typeset("private", args, assigns, ops, func);
     }
 
@@ -559,7 +561,7 @@ printprivatenode(HashNode hn, int printflags)
 
 static struct builtin bintab[] = {
     /* Copied from BUILTIN("local"), "P" added */
-    BUILTIN("private", BINF_PLUSOPTS | BINF_MAGICEQUALS | BINF_PSPECIAL | BINF_ASSIGN, (HandlerFunc)bin_private, 0, -1, 0, "AE:%F:%HL:%PR:%TUZ:%ahi:%lprtux", "P")
+    BUILTIN("private", BINF_PLUSOPTS | BINF_MAGICEQUALS | BINF_PSPECIAL | BINF_ASSIGN, (HandlerFunc)bin_private, 0, -1, 0, "AE:%F:%HL:%PR:%TUZ:%ahi:%lmprtux", "P")
 };
 
 static struct features module_features = {
diff --git a/Src/params.c b/Src/params.c
index 704aad588..b703a97ce 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -1013,6 +1013,11 @@ createparam(char *name, int flags)
 		(oldpm->node.flags & PM_SPECIAL) ||
 		/* 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",
+			 name);
+		    return NULL;
+		}
 		oldpm->node.flags &= ~PM_UNSET;
 		if ((oldpm->node.flags & PM_SPECIAL) && oldpm->ename) {
 		    Param altpm =
diff --git a/Test/V10private.ztst b/Test/V10private.ztst
index 03e8259d5..56ffbc5b4 100644
--- a/Test/V10private.ztst
+++ b/Test/V10private.ztst
@@ -116,14 +116,14 @@
  }
  outer () {
   local -PA hash_test=(in function)
-  typeset -p hash_test
+  private + hash_test
   inner
  }
  outer
  print ${(kv)hash_test}
 0:private hides value from surrounding scope in nested scope
 >typeset -a hash_test=( top level )
->typeset -A hash_test=( [in]=function )
+>hash_test=( [in]=function )
 >typeset -g -a hash_test=( top level )
 >array-local top level
 >top level
@@ -246,7 +246,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: attempt to assign private in nested scope
+?(anon):4: array_test: can't change parameter attribute
 F:future revision will create a global with this assignment
 
  typeset -a array_test