Re: [BUG] Can't mark unset variables as read-only

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Wed, 29 Apr 2015 08:35:43 +0200
Martijn Dekker <martijn@xxxxxxxx> wrote:
> Unlike other shells, zsh can't mark an unset variable as read-only.

Yes, the standard does indeed require that ability.  As you can imagine,
something like this that completely breaks the normal programming model
of variables (a variable can't have state if it's unset because it
doesn't exist) is a nightmare to implement; however, it can at least be
limited to the POSIXBUILTINS option and maybe the cases using that are
simple enough that it mostly works.  I'm sure there are any number of
strange edge cases, though.

The output of "readonly -p" is still broken (doesn't show the unset
variables in a fashion that can be used for restoring the current state)
but it was anyway:

  typeset -ar '*'
  *=()

Er, no, I don't think that's going to work.

So that can be fixed separately.

diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index dd5a80f..985d19e 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -1933,6 +1933,13 @@ item(tt(-r))(
 The given var(name)s are marked readonly.  Note that if var(name) is a
 special parameter, the readonly attribute can be turned on, but cannot then
 be turned off.
+
+If the tt(POSIX_BUILTINS) option is set, the readonly attribute is
+more restrictive: unset variables can be marked readonly and cannot then
+be set; furthermore, the readonly attribute cannot be removed from any
+variable.  Note that in zsh (unlike other shells) it is still possible
+to create a local variable of the same name as this is considered a
+different variable (though this variable, too, can be marked readonly).
 )
 item(tt(-t))(
 Tags the named parameters.  Tags have no special meaning to the shell.
diff --git a/Src/builtin.c b/Src/builtin.c
index de01014..0a57489 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -1931,8 +1931,12 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
      * locallevel as an unset one we use the pm struct anyway: that's
      * handled in createparam().  Here we just avoid using it for the
      * present tests if it's unset.
+     *
+     * POSIXBUILTINS horror: we need to retain the 'readonly' flag
+     * of an unset parameter.
      */
-    usepm = pm && !(pm->node.flags & PM_UNSET);
+    usepm = pm && (!(pm->node.flags & PM_UNSET) ||
+		   (isset(POSIXBUILTINS) && (pm->node.flags & PM_READONLY)));
 
     /*
      * We need to compare types with an existing pm if special,
@@ -2032,6 +2036,20 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
     else if (newspecial != NS_NONE && strcmp(pname, "SECONDS") == 0)
 	newspecial = NS_SECONDS;
 
+    if (isset(POSIXBUILTINS)) {
+	/*
+	 * Stricter rules about retaining readonly attribute in this case.
+	 */
+	if ((on & PM_READONLY) && (!usepm || (pm->node.flags & PM_UNSET)) &&
+	    !value)
+	    on |= PM_UNSET;
+	else if (usepm && (pm->node.flags & PM_READONLY) &&
+		 !(on & PM_READONLY)) {
+	    zerr("read-only variable: %s", pm->node.nam);
+	    return NULL;
+	}
+    }
+
     /*
      * A parameter will be local if
      * 1. we are re-using an existing local parameter
@@ -2078,9 +2096,15 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
 	}
 	if (usepm == 2)		/* do not change the PM_UNSET flag */
 	    pm->node.flags = (pm->node.flags | (on & ~PM_READONLY)) & ~off;
-	else
+	else {
+	    /*
+	     * Keep unset if using readonly in POSIX mode.
+	     */
+	    if (!(on & PM_READONLY) || !isset(POSIXBUILTINS))
+		off |= PM_UNSET;
 	    pm->node.flags = (pm->node.flags |
-			      (on & ~PM_READONLY)) & ~(off | PM_UNSET);
+			      (on & ~PM_READONLY)) & ~off;
+	}
 	if (on & (PM_LEFT | PM_RIGHT_B | PM_RIGHT_Z)) {
 	    if (typeset_setwidth(cname, pm, ops, on, 0))
 		return NULL;
@@ -2256,7 +2280,12 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
 	 * readonly flag
 	 */
 	pm = createparam(pname, on & ~PM_READONLY);
-	DPUTS(!pm, "BUG: parameter not created");
+	if (!pm) {
+	    if (on & (PM_LEFT | PM_RIGHT_B | PM_RIGHT_Z |
+		      PM_INTEGER | PM_EFLOAT | PM_FFLOAT))
+		zerrnam(cname, "can't change variable attribute: %s", pname);
+	    return NULL;
+	}
 	if (on & (PM_LEFT | PM_RIGHT_B | PM_RIGHT_Z)) {
 	    if (typeset_setwidth(cname, pm, ops, on, 0))
 		return NULL;
diff --git a/Src/params.c b/Src/params.c
index e8a9010..d53b6ca 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -874,10 +874,14 @@ createparam(char *name, int flags)
 	DPUTS(oldpm && oldpm->level > locallevel,
 	      "BUG: old local parameter not deleted");
 	if (oldpm && (oldpm->level == locallevel || !(flags & PM_LOCAL))) {
+	    if (isset(POSIXBUILTINS) && (oldpm->node.flags & PM_READONLY)) {
+		zerr("read-only variable: %s", name);
+		return NULL;
+	    }
 	    if (!(oldpm->node.flags & PM_UNSET) || (oldpm->node.flags & PM_SPECIAL)) {
 		oldpm->node.flags &= ~PM_UNSET;
 		if ((oldpm->node.flags & PM_SPECIAL) && oldpm->ename) {
-		    Param altpm = 
+		    Param altpm =
 			(Param) paramtab->getnode(paramtab, oldpm->ename);
 		    if (altpm)
 			altpm->node.flags &= ~PM_UNSET;
diff --git a/Test/B02typeset.ztst b/Test/B02typeset.ztst
index 51ebc65..f4fb8ec 100644
--- a/Test/B02typeset.ztst
+++ b/Test/B02typeset.ztst
@@ -468,3 +468,20 @@
 0:retying arrays to same array works
 >foo bar
 >goo car
+
+ (
+   setopt POSIXBUILTINS
+   readonly pbro
+   print ${+pbro} >&2
+   (typeset pbro=3)
+   (pbro=4)
+   typeset -r pbro        # idempotent (no error)...
+   print ${+pbro} >&2     # ...so still readonly...
+   typeset +r pbro        # ...can't turn it off
+ )
+1:Readonly with POSIX_BUILTINS
+?0
+?(eval):5: read-only variable: pbro
+?(eval):6: read-only variable: pbro
+?0
+?(eval):9: read-only variable: pbro