POSIX_BUILTINS and readonly parameters, again

POSIX_BUILTINS and readonly parameters, again

[Bart Schaefer]

This concerns the discussion that started in 34991, specifically the patch
34992 and the thread leading to 35004 where things dropped without further
resolution.

Part of the patch in 34992 is this:

@@ -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;
+           }

Short question:

Why does that test POSIXBUILTINS as well as PM_READONLY ?

Long explanation:

The isset(POSIXBUILTINS) there seems to be covering a really strange edge
case.  If POSIXBUILTINS is not set, it's impossible to create a readonly
variable that is unset:

torch% () { readonly foo; print ${foo+foo is set}; unset foo }
foo is set
(anon): read-only variable: foo
torch% () { readonly foo; print ${foo+foo is set}; foo=oopsie }
foo is set
(anon): read-only variable: foo

[That first error is coming from unsetparam_pm() and the second from
assignsparam().  Neither is from the createparam() patch above, the
variable is set so createparam() never called.]

Conversely if POSIXBUILTINS is set, readonly variables are unset unless
assigned in the declaration [good thing we fixed array assignment there]:

torch% () { setopt localoptions posixbuiltins
 readonly foo; print ${foo-foo is NOT set}; unset foo }
foo is NOT set
(anon):1: read-only variable: foo
torch% () { setopt localoptions posixbuiltins
 readonly foo; print ${foo-foo is NOT set}; foo=oopsie }       
foo is NOT set
(anon):1: read-only variable: foo

[Note we still get the error for attempting to unset an already unset
parameter.  Probably wrong.  The first error is from unsetparam_pm()
and the second one is from createparam(), assignment is never tried.]

So the only reason for testing isset(POSIXBUILTINS) in createparam() is
the case where POSIXBUILTINS *changes* between time of declaration and
time of assignment:

torch% () { setopt localoptions posixbuiltins
 readonly foo; print ${foo-foo is NOT set};
 unsetopt posixbuiltins;             
 foo=oopsie; print ${foo-foo is NOT set} }
foo is NOT set
oopsie

This seems a little twitchy.  Having gone to the trouble of creating a
parameter that we know is both readonly and unset, we allow it to be
clobbered anyway?  Could this respect PM_READONLY unconditionally?

-- 
Barton E. Schaefer

Re: POSIX_BUILTINS and readonly parameters, again

[Peter Stephenson]

On Wed, 25 Nov 2015 16:33:51 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Conversely if POSIXBUILTINS is set, readonly variables are unset unless
> assigned in the declaration 

Right, that was the point.

> [good thing we fixed array assignment there]:

Actually, that reminds me...

  % readonly -a foo=(one two)
  % readonly -p
  typeset -a foo
  foo=( one two )
  typeset -ar foo

We don't need to make such heavy weather of this now.  But that can
wait.
 
> So the only reason for testing isset(POSIXBUILTINS) in createparam() is
> the case where POSIXBUILTINS *changes* between time of declaration and
> time of assignment:

I'm sure your analysis is correct, however the rule we were trying to
implement starts "if POSIXBUILTINS is set ..." (and then goes on about
readonly and unset variables).  It seems peverse to have to analyse the
whole sequence and decide that part of the change is actually benign in
other cases when that's not really relevant to what's being implemented.

However, removing the test with a suitable comment about the effect of
the code, now you've worked it out, would certainly be a valid
alternative approach.

pws