LOCAL_VARS option ?

LOCAL_VARS option ?

[Daniel Shahaf]

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

Phil suggested on IRC a LOCAL_VARS option that has the effect of making
all newly-declared variables local; e.g.,

% unset x y
% () { setopt localvars; x=42; typeset -g y=43 }
% echo $+x $+y
0 1
% 

I'm attaching a proof of concept patch (work in progress; see top of the
attachment for known issues), but WDYT of the the general concept?

[-- Attachment #2: local_vars-v1-wip.diff --]
[-- Type: text/x-diff, Size: 9511 bytes --]

[[[
WIP: LOCAL_VARS option

Proof of concept.  Known issues:

1. Interaction with 'emulate -L' (see TODO below).

2. There's a block in assignstrvalue() that I don't know whether needs
changing or not (see TODO below).

3. Currently, LOCAL_VARS overrides 'typeset -g', which is silly.  This
shouldn't be hard to fix by having bin_typeset() propagate a "leave
PM_LOCAL unset" flag to createparam().  (Currently, createparam() is
called with flags=PM_LOCAL for 'typeset' and with flags=0 both from
'typeset -g' and from callers other than bin_typeset().)

Finally, I'm not that familiar with the *param() C functions so I could
easily have overlooked something.
]]]

diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index 3a3130a..ac93810 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -552,6 +552,8 @@ function, if any; normally these options are turned off in all emulation
 modes except tt(ksh). The tt(-L) switch is mutually exclusive with the
 use of tt(-c) in var(flags).
 
+em(TODO): should tt(emulate -L) set tt(LOCAL_VARS)?
+
 If there is a single argument and the tt(-l) switch is given, the
 options that would be set or unset (the latter indicated with the prefix
 `tt(no)') are listed.  tt(-l) can be combined with tt(-L) or tt(-R) and
diff --git a/Doc/Zsh/options.yo b/Doc/Zsh/options.yo
index 434b710..46e5de5 100644
--- a/Doc/Zsh/options.yo
+++ b/Doc/Zsh/options.yo
@@ -1705,8 +1705,9 @@ item(tt(LOCAL_OPTIONS) <K>)(
 If this option is set at the point of return from a shell function,
 most options (including this one) which were in force upon entry to
 the function are restored; options that are not restored are
-tt(PRIVILEGED) and tt(RESTRICTED).  Otherwise, only this option,
-and the tt(LOCAL_LOOPS), tt(XTRACE) and tt(PRINT_EXIT_VALUE) options are
+tt(PRIVILEGED) and tt(RESTRICTED).  Otherwise, only the options
+tt(LOCAL_LOOPS), tt(LOCAL_OPTIONS), tt(LOCAL_VARS), tt(PRINT_EXIT_VALUE),
+and tt(XTRACE) are
 restored.  Hence if this is explicitly unset by a shell function the
 other options in force at the point of return will remain so.
 A shell function can also guarantee itself a known shell configuration
@@ -1745,6 +1746,18 @@ fn+LPAR()RPAR() { setopt localtraps; trap '' INT; sleep 3; })
 
 will restore normal handling of tt(SIGINT) after the function exits.
 )
+pindex(LOCAL_VARS)
+pindex(NO_LOCAL_VARS)
+pindex(LOCALVARS)
+pindex(NOLOCALVARS)
+item(tt(LOCAL_VARS))(
+Whilst this option is set, any shell construct that declares a new shell
+parameter will default to making that parameter local to the current function.
+Declaring a parameter that is visible to outer scopes is still possible with
+tt(typeset -g) and tt(typeset -x).
+
+This option overrides tt(ALL_EXPORT).
+)
 pindex(MULTI_FUNC_DEF)
 pindex(NO_MULTI_FUNC_DEF)
 pindex(MULTIFUNCDEF)
diff --git a/Src/builtin.c b/Src/builtin.c
index 3b5b2c4..d4b290e 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -5779,6 +5779,7 @@ bin_emulate(char *nam, char **argv, Options ops, UNUSED(int func))
 	if (opt_L)
 	    cmdopts[LOCALOPTIONS] = cmdopts[LOCALTRAPS] =
 		cmdopts[LOCALPATTERNS] = 1;
+	    /* XXX LOCALVARS */
 	if (opt_l) {
 	    list_emulate_options(cmdopts, opt_R);
 	    return 0;
@@ -5831,6 +5832,7 @@ bin_emulate(char *nam, char **argv, Options ops, UNUSED(int func))
     } else {
 	if (opt_L)
 	    opts[LOCALOPTIONS] = opts[LOCALTRAPS] = opts[LOCALPATTERNS] = 1;
+	    /* XXX LOCALVARS */
 	return 0;
     }
 
diff --git a/Src/exec.c b/Src/exec.c
index d3538c3..f812205 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2429,7 +2429,6 @@ addvars(Estate state, Wordcode pc, int addflags)
 	if (isstr && (empty(vl) || !nextnode(firstnode(vl)))) {
 	    Param pm;
 	    char *val;
-	    int allexp;
 
 	    if (empty(vl))
 		val = ztrdup("");
@@ -2442,6 +2441,8 @@ addvars(Estate state, Wordcode pc, int addflags)
 		fputc(' ', xtrerr);
 	    }
 	    if ((addflags & ADDVAR_EXPORT) && !strchr(name, '[')) {
+		int old_LOCALVARS;
+		int allexp;
 		if ((addflags & ADDVAR_RESTRICT) && isset(RESTRICTED) &&
 		    (pm = (Param) paramtab->removenode(paramtab, name)) &&
 		    (pm->node.flags & PM_RESTRICTED)) {
@@ -2455,11 +2456,14 @@ addvars(Estate state, Wordcode pc, int addflags)
 		    STTYval = ztrdup(val);
 		}
 		allexp = opts[ALLEXPORT];
+		old_LOCALVARS = opts[LOCALVARS];
 		opts[ALLEXPORT] = 1;
+		opts[LOCALVARS] = 0;
 		if (isset(KSHARRAYS))
 		    unsetparam(name);
 	    	pm = assignsparam(name, val, myflags);
 		opts[ALLEXPORT] = allexp;
+		opts[LOCALVARS] = old_LOCALVARS;
 	    } else
 	    	pm = assignsparam(name, val, myflags);
 	    if (errflag) {
@@ -5532,6 +5536,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	    opts[XTRACE] = saveopts[XTRACE];
 	    opts[PRINTEXITVALUE] = saveopts[PRINTEXITVALUE];
 	    opts[LOCALOPTIONS] = saveopts[LOCALOPTIONS];
+	    opts[LOCALVARS] = saveopts[LOCALVARS];
 	    opts[LOCALLOOPS] = saveopts[LOCALLOOPS];
 	}
 
diff --git a/Src/options.c b/Src/options.c
index 4729ba5..ef7c91c 100644
--- a/Src/options.c
+++ b/Src/options.c
@@ -186,6 +186,7 @@ static struct optname optns[] = {
 {{NULL, "localloops",	      OPT_EMULATE},		 LOCALLOOPS},
 {{NULL, "localpatterns",      OPT_EMULATE},		 LOCALPATTERNS},
 {{NULL, "localtraps",	      OPT_EMULATE|OPT_KSH},	 LOCALTRAPS},
+{{NULL, "localvars",	      0},			 LOCALVARS},
 {{NULL, "login",	      OPT_SPECIAL},		 LOGINSHELL},
 {{NULL, "longlistjobs",	      0},			 LONGLISTJOBS},
 {{NULL, "magicequalsubst",    OPT_EMULATE},		 MAGICEQUALSUBST},
diff --git a/Src/params.c b/Src/params.c
index 946fdd1..30a6027 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -975,13 +975,22 @@ createparam(char *name, int flags)
 	    paramtab->addnode(paramtab, ztrdup(name), pm);
 	}
 
-	if (isset(ALLEXPORT) && !(flags & PM_HASHELEM))
+	if (isset(ALLEXPORT) && !(flags & PM_HASHELEM) &&
+	    unset(LOCALVARS))
 	    flags |= PM_EXPORTED;
     } else {
 	pm = (Param) hcalloc(sizeof *pm);
 	pm->node.nam = nulstring;
     }
-    pm->node.flags = flags & ~PM_LOCAL;
+    pm->node.flags = flags;
+    if ((flags & PM_HASHELEM) || (flags & PM_EXPORTED))
+	pm->node.flags &= ~PM_LOCAL;
+    else if (isset(LOCALVARS)) {
+	pm->node.flags |= PM_LOCAL;
+	pm->level = locallevel;
+    }
+    else
+	pm->node.flags &= ~PM_LOCAL;
 
     if(!(pm->node.flags & PM_SPECIAL))
 	assigngetset(pm);
@@ -2581,10 +2590,14 @@ assignstrvalue(Value v, char *val, int flags)
         }
 	break;
     }
+    /* TODO LOCALVARS: should isset(LOCALVARS) be checked here?
+     * (This block updates $PWD during cd.)
+     */
     if ((!v->pm->env && !(v->pm->node.flags & PM_EXPORTED) &&
-	 !(isset(ALLEXPORT) && !(v->pm->node.flags & PM_HASHELEM))) ||
+	 (unset(ALLEXPORT) || (v->pm->node.flags & PM_HASHELEM))) ||
 	(v->pm->node.flags & PM_ARRAY) || v->pm->ename)
 	return;
+    DPUTS1(0, "exporting %s", v->pm->node.nam);
     export_param(v->pm);
 }
 
@@ -4658,6 +4671,11 @@ pipestatsetfn(UNUSED(Param pm), char **x)
         numpipestats = 0;
 }
 
+/*
+ * If the shell scalar parameter s is exported, then set the corresponding
+ * environment variable to the array 't' joined by the tied array's joinchar.
+ */
+
 /**/
 void
 arrfixenv(char *s, char **t)
diff --git a/Src/zsh.h b/Src/zsh.h
index deefdba..3ab0415 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -2334,6 +2334,7 @@ enum {
     LOCALOPTIONS,
     LOCALPATTERNS,
     LOCALTRAPS,
+    LOCALVARS,
     LOGINSHELL,
     LONGLISTJOBS,
     MAGICEQUALSUBST,
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index 45df9f5..c9dc2bc 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -1213,3 +1213,50 @@
 ?(anon):4: `break' active at end of function scope
 ?(anon):4: `break' active at end of function scope
 ?(anon):4: `break' active at end of function scope
+
+  (unset x; () {                   x=42 }; echo "scalar born, unset: $+x")
+  (unset x; () { setopt localvars; x=42 }; echo "scalar born, set: $+x")
+  (local x; () {                   x=42 }; echo "scalar exists, unset: $+x")
+  (local x; () { setopt localvars; x=42 }; echo "scalar exists, set: $+x")
+  (unset x; () { setopt localvars; x=42 sh -c 'printf %s $x'; }; echo " in child, and now: $+x")
+0:LOCAL_VARS, scalars
+>scalar born, unset: 1
+>scalar born, set: 0
+>scalar exists, unset: 1
+>scalar exists, set: 1
+>42 in child, and now: 0
+
+  (unset x; () {                   x[5]=42 };    echo "array born, unset: $+x")
+  (unset x; () { setopt localvars; x[5]=42 };    echo "array born, set: $+x")
+  (typeset -a x; () {                   x[5]=42 }; echo "array exists, set: $+x ${+x[5]}")
+  (typeset -a x; () { setopt localvars; x[5]=42 }; echo "array exists, unset: $+x ${+x[5]}")
+0:LOCAL_VARS, classic arrays
+>array born, unset: 1
+>array born, set: 0
+>array exists, set: 1 1
+>array exists, unset: 1 1
+
+  (unset x; () {                   : ${(AA)=x::=foo bar}; }; echo "assoc born, unset: $+x")
+  (unset x; () { setopt localvars; : ${(AA)=x::=foo bar}; }; echo "assoc born, set: $+x")
+  (typeset -A x; () {                   x[foo]=42 }; echo "assoc exists, unset: $+x ${+x[foo]}")
+  (typeset -A x; () { setopt localvars; x[foo]=42 }; echo "assoc exists, set: $+x ${+x[foo]}")
+0:LOCAL_VARS, associative arrays
+>assoc born, unset: 1
+>assoc born, set: 0
+>assoc exists, unset: 1 1
+>assoc exists, set: 1 1
+
+# don't use LOCAL_TRAPS when testing it
+  () {
+    local flags
+    for flags in {,a,A}{g,x}; do
+      (unset x; () { typeset -$flags x; setopt localvars; }; echo $+x)
+    done
+  }
+0:LOCAL_VARS with -g/-x
+>1
+>1
+>1
+>1
+>1
+>1

Re: LOCAL_VARS option ?

[Jens Elkner]

On Thu, Jan 19, 2017 at 06:54:08AM +0000, Daniel Shahaf wrote:
> Phil suggested on IRC a LOCAL_VARS option that has the effect of making
> all newly-declared variables local; e.g.,

Hmmm, I like the implicit way ksh93 does it, i.e. if declared within a
"function $fname {...}" declared function using 'typeset', it has local
scope, otherwise (i.e. not typeset or typeset in "$fname() { ... }" aka
old style declared functions) shared. Not sure, whether this can be
accomplished with zsh as well ...

have fun,
jel.
-- 
Otto-von-Guericke University     http://www.cs.uni-magdeburg.de/
Department of Computer Science   Geb. 29 R 027, Universitaetsplatz 2
39106 Magdeburg, Germany         Tel: +49 391 67 52768

Re: LOCAL_VARS option ?

[Peter Stephenson]

On Thu, 19 Jan 2017 06:54:08 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Phil suggested on IRC a LOCAL_VARS option that has the effect of making
> all newly-declared variables local; e.g.,
> 
> % unset x y
> % () { setopt localvars; x=42; typeset -g y=43 }
> % echo $+x $+y
> 0 1
> % 
> 
> I'm attaching a proof of concept patch (work in progress; see top of the
> attachment for known issues), but WDYT of the the general concept?

It does seem useful --- it's very easy to forget to make variables
local, while usually only a small fraction ever need to make their way
out of functions (there are special cases of function suites like
completion which behave diffierently).  I'm worried that picking up all
the places where the flag needs setting or unsetting could be a huge
job.

> +em(TODO): should tt(emulate -L) set tt(LOCAL_VARS)?

That's going to be too big a change to the current behaviour, I think.
Not enough people know about WARN_CREATE_GLOBAL / typeset -g which would
have prepared them for this behaviour, and it's going to have very
obscure effects on nested functions.  It clearly needs flagging up under
emulate -L either way.

pws

Re: LOCAL_VARS option ?

[Bart Schaefer]

On Thu, 19 Jan 2017, Daniel Shahaf wrote:

> Phil suggested on IRC a LOCAL_VARS option that has the effect of making
> all newly-declared variables local; e.g.,
>
> % unset x y
> % () { setopt localvars; x=42; typeset -g y=43 }
> % echo $+x $+y
> 0 1
> %
>
> I'm attaching a proof of concept patch (work in progress; see top of the
> attachment for known issues), but WDYT of the the general concept?

I believe we discussed this idea once before, and rejected it on several
grounds.  However, I can't find the thread at the moment.  From memory:

1. Once the option is set, it affects all functions called by (whether
   directly or indirectly) the one that set the option.  If set at the
   top level, this results in a significant change in semantics.

2. Unlike local_options, which applies when the function exits, this has
   to be applied when the parameter is created.  There's already a
   mechanism to accomplish this, namely to declare the parameter.  The
   only reason to need local_vars is to change the semantics of *other*
   functions [see (1)], which is generally a bad idea.

3. If unset by a called function in order to prevent (2) and that called
   function is NOT also using local_options, it can break the calling
   function in unpredictable ways.

4. The semantics of the other LOCAL_* options are already problematic in
   obscure ways, but just because we're stuck with them doesn't mean we
   should add another potentially problematic variation.

5. (New since the last time this was discussed) The type of problem this
   "solves" is generally better addressed by WARN_CREATE_GLOBAL so that
   proper use of parameter declarations can be applied.

Re: LOCAL_VARS option ?

[Peter Stephenson]

On Thu, 19 Jan 2017 07:47:53 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> > Phil suggested on IRC a LOCAL_VARS option that has the effect of making
> > all newly-declared variables local
>
> I believe we discussed this idea once before, and rejected it on several
> grounds.  However, I can't find the thread at the moment.  From memory:
> 
> 1. Once the option is set, it affects all functions called by (whether
>    directly or indirectly) the one that set the option.  If set at the
>    top level, this results in a significant change in semantics.

I suspect we'd even need "emulate -L zsh" to *un*set the option to
restore sanity.  Otherwise you're not emulating native zsh variable
semantics.

There's some prior art for resetting options at each function level, but
it's another icky complexity.

> 2. Unlike local_options, which applies when the function exits, this has
>    to be applied when the parameter is created.  There's already a
>    mechanism to accomplish this, namely to declare the parameter.  The
>    only reason to need local_vars is to change the semantics of *other*
>    functions [see (1)], which is generally a bad idea.

Hmmm... I suspect people would want to set it to protect their own
functions, rather than randomly assume what other functions they have
are or are not doing with variable scoping, which would be a bad thing
to assume.  But other people's functions are likely to get caught in
the crossfire anyway.
 
> 3. If unset by a called function in order to prevent (2) and that called
>    function is NOT also using local_options, it can break the calling
>    function in unpredictable ways.

I think that mostly means the way to turn this off (other than
implicitly at each level) would be

setopt localvars localoptions

but if you have some reason for unsetting localoptions you are stuck.

> 4. The semantics of the other LOCAL_* options are already problematic in
>    obscure ways, but just because we're stuck with them doesn't mean we
>    should add another potentially problematic variation.

Well, the *reason* for adding them is a bit of extra safety, rather than
creating problems as side-effects...  but I agree about the potential
problems.  In fact, my best argument against this probably is the
knock-on effects of everything that has to be made consistent, which
I think is hard to foresee, and the variable code is famously
complicated already.

> 5. (New since the last time this was discussed) The type of problem this
>    "solves" is generally better addressed by WARN_CREATE_GLOBAL so that
>    proper use of parameter declarations can be applied.

Yes, that makes a difference, but it doesn't detect the case where you
re-use a variable in a nested function without redeclaring it.  (That
can be done, too, by comparing variable levels, but it's a significantly
more intrusive option --- consider the completion system.)

pws

Re: LOCAL_VARS option ?

[Bart Schaefer]

On Thu, 19 Jan 2017, Peter Stephenson wrote:

> On Thu, 19 Jan 2017 07:47:53 -0800
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> > 2. Unlike local_options, which applies when the function exits, this has
> >    to be applied when the parameter is created.  There's already a
> >    mechanism to accomplish this, namely to declare the parameter.  The
> >    only reason to need local_vars is to change the semantics of *other*
> >    functions [see (1)], which is generally a bad idea.
>
> Hmmm... I suspect people would want to set it to protect their own
> functions

That's what zsh/param/private is for ...

> > 5. (New since the last time this was discussed) The type of problem this
> >    "solves" is generally better addressed by WARN_CREATE_GLOBAL so that
> >    proper use of parameter declarations can be applied.
>
> Yes, that makes a difference, but it doesn't detect the case where you
> re-use a variable in a nested function without redeclaring it.  (That
> can be done, too, by comparing variable levels, but it's a significantly
> more intrusive option --- consider the completion system.)

Perhaps rather than a setopt, this could be a property of functions --
something like "typeset -fT" so limited to the single function to which
it is applied.  That could be appropriate either for a generic "used
but not declared local" warning or the "implicit local var" feature,
though I would still have reservations about the latter.

Anyway the point is that we should make it easier to debug these issues
rather than make it easier to sweep them under the proverbial rug.

Re: LOCAL_VARS option ?

[Peter Stephenson]

On Thu, 19 Jan 2017 21:01:41 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> Perhaps rather than a setopt, this could be a property of functions --
> something like "typeset -fT" so limited to the single function to which
> it is applied.  That could be appropriate either for a generic "used
> but not declared local" warning or the "implicit local var" feature,
> though I would still have reservations about the latter.

Here's a first go at the warning option, with "functions -W" to turn it on
in the same fashion as "functions -T".  The option is called
WARN_NESTED_VAR for now.  If we want a pair of options like -t / -T
then the option will need to change as autoload -w is for compiled
files (i.e. wordcode).

The main drawback over WARN_CREATE_GLOBAL is that "typeset -g" doesn't
mark the variable for future assignments; it's still in a global scope
so still gets a warning next time.  I think that's correct behaviour
(it's certainly incredibly fiddly and bug-prone to do anything about).
But this makes it impossible to suppress the warning for a (( ... )),
though "typeset -g var=$(( ... ))" works.

pws

diff --git a/Completion/compinit b/Completion/compinit
index cc663cb..eb88a9f 100644
--- a/Completion/compinit
+++ b/Completion/compinit
@@ -157,6 +157,7 @@ _comp_options=(
     NO_posixidentifiers
     NO_shwordsplit
     NO_shglob
+    NO_warnnestedvar
     NO_warncreateglobal
 )
 
diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index 3a3130a..0a9021c 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -147,7 +147,7 @@ ifnzman(noderef(Aliasing)).
 findex(autoload)
 cindex(functions, autoloading)
 cindex(autoloading functions)
-item(tt(autoload) [ {tt(PLUS())|tt(-)}tt(RTUXdkmrtz) ] [ tt(-w) ] [ var(name) ... ])(
+item(tt(autoload) [ {tt(PLUS())|tt(-)}tt(RTUXdkmrtWz) ] [ tt(-w) ] [ var(name) ... ])(
 vindex(fpath, searching)
 See the section `Autoloading Functions' in ifzman(zmanref(zshmisc))\
 ifnzman(noderef(Functions)) for full details.  The tt(fpath) parameter
@@ -836,19 +836,24 @@ Equivalent to tt(typeset -E), except that options irrelevant to floating
 point numbers are not permitted.
 )
 findex(functions)
-xitem(tt(functions) [ {tt(PLUS())|tt(-)}tt(UkmtTuz) ] [ tt(-x) var(num) ] [ var(name) ... ])
+xitem(tt(functions) [ {tt(PLUS())|tt(-)}tt(UkmtTuWz) ] [ tt(-x) var(num) ] [ var(name) ... ])
 xitem(tt(functions -M) var(mathfn) [ var(min) [ var(max) [ var(shellfn) ] ] ])
 xitem(tt(functions -M) [ tt(-m) var(pattern) ... ])
 item(tt(functions +M) [ tt(-m) ] var(mathfn) ... )(
-Equivalent to tt(typeset -f), with the exception of the tt(-x) and
-tt(-M) options.  For tt(functions -u) and tt(functions -U), see
-tt(autoload), which provides additional options.
+Equivalent to tt(typeset -f), with the exception of the tt(-x),
+tt(-M) and tt(-W) options.  For tt(functions -u) and tt(functions -U),
+see tt(autoload), which provides additional options.
 
 The tt(-x) option indicates that any functions output will have
 each leading tab for indentation, added by the shell to show syntactic
 structure, expanded to the given number var(num) of spaces.  var(num)
 can also be 0 to suppress all indentation.
 
+The tt(-W) option turns on the option tt(WARN_NESTED_VAR) for the named
+function or functions only.  The option is turned off at the start of
+nested functions (apart from anonoymous functions) unless the called
+function also has the tt(-W) attribute.
+
 Use of the tt(-M) option may not be combined with any of the options
 handled by tt(typeset -f).
 
diff --git a/Doc/Zsh/options.yo b/Doc/Zsh/options.yo
index 434b710..aa62748 100644
--- a/Doc/Zsh/options.yo
+++ b/Doc/Zsh/options.yo
@@ -768,6 +768,37 @@ global from within a function using tt(typeset -g) do not cause a warning.
 Note that there is no warning when a local parameter is assigned to in
 a nested function, which may also indicate an error.
 )
+pindex(WARN_NESTED_VAR)
+pindex(NO_WARN_NESTED_VAR)
+pindex(WARNNESTEDVAR)
+pindex(NO_WARNNESTEDVAR)
+cindex(parameters, warning when setting in enclosing scope)
+item(tt(WARN_NESTED_VAR))(
+Print a warning message when an existing parameter from an
+enclosing function scope, or global, is set in a function
+by an assignment or in math context.  Assignment to shell
+special parameters does not cause a warning.  This is the companion
+to tt(WARN_CREATE_GLOBAL) as in this case the warning is only
+printed when a parameter is em(not) created.  Where possible,
+use of tt(typeset -g) to set the parameter suppresses the error,
+but note that this needs to be used every time the parameter is set.
+To restrict the effect of this option to a single function scope,
+use `tt(functions -W)'.
+
+For example, the following code produces a warning for the assignment
+inside the function tt(nested) as that overrides the value within
+tt(toplevel)
+
+example(toplevel+LPAR()RPAR() {
+  local foo="in fn"
+  nested
+}
+nested+LPAR()RPAR() {
+     foo="in nested"
+}
+setopt warn_nested_var
+toplevel)
+)
 enditem()
 
 subsect(History)
diff --git a/Src/builtin.c b/Src/builtin.c
index 7a04a79..2fb1a70 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -46,7 +46,7 @@ static struct builtin builtins[] =
     BUILTIN(".", BINF_PSPECIAL, bin_dot, 1, -1, 0, NULL, NULL),
     BUILTIN(":", BINF_PSPECIAL, bin_true, 0, -1, 0, NULL, NULL),
     BUILTIN("alias", BINF_MAGICEQUALS | BINF_PLUSOPTS, bin_alias, 0, -1, 0, "Lgmrs", NULL),
-    BUILTIN("autoload", BINF_PLUSOPTS, bin_functions, 0, -1, 0, "dmktrRTUwXz", "u"),
+    BUILTIN("autoload", BINF_PLUSOPTS, bin_functions, 0, -1, 0, "dmktrRTUwWXz", "u"),
     BUILTIN("bg", 0, bin_fg, 0, -1, BIN_BG, NULL, NULL),
     BUILTIN("break", BINF_PSPECIAL, bin_break, 0, 1, BIN_BREAK, NULL, NULL),
     BUILTIN("bye", 0, bin_break, 0, 1, BIN_EXIT, NULL, NULL),
@@ -72,7 +72,7 @@ static struct builtin builtins[] =
     BUILTIN("fc", 0, bin_fc, 0, -1, BIN_FC, "aAdDe:EfiIlLmnpPrRt:W", NULL),
     BUILTIN("fg", 0, bin_fg, 0, -1, BIN_FG, NULL, NULL),
     BUILTIN("float", BINF_PLUSOPTS | BINF_MAGICEQUALS | BINF_PSPECIAL | BINF_ASSIGN, (HandlerFunc)bin_typeset, 0, -1, 0, "E:%F:%HL:%R:%Z:%ghlprtux", "E"),
-    BUILTIN("functions", BINF_PLUSOPTS, bin_functions, 0, -1, 0, "kmMtTuUx:z", NULL),
+    BUILTIN("functions", BINF_PLUSOPTS, bin_functions, 0, -1, 0, "kmMtTuUWx:z", NULL),
     BUILTIN("getln", 0, bin_read, 0, -1, 0, "ecnAlE", "zr"),
     BUILTIN("getopts", 0, bin_getopts, 2, -1, 0, NULL, NULL),
     BUILTIN("hash", BINF_MAGICEQUALS, bin_hash, 0, -1, 0, "Ldfmrv", NULL),
@@ -796,8 +796,8 @@ set_pwd_env(void)
 	unsetparam_pm(pm, 0, 1);
     }
 
-    setsparam("PWD", ztrdup(pwd));
-    setsparam("OLDPWD", ztrdup(oldpwd));
+    assignsparam("PWD", ztrdup(pwd), 0);
+    assignsparam("OLDPWD", ztrdup(oldpwd), 0);
 
     pm = (Param) paramtab->getnode(paramtab, "PWD");
     if (!(pm->node.flags & PM_EXPORTED))
@@ -3068,6 +3068,10 @@ bin_functions(char *name, char **argv, Options ops, int func)
 	on |= PM_TAGGED_LOCAL;
     else if (OPT_PLUS(ops,'T'))
 	off |= PM_TAGGED_LOCAL;
+    if (OPT_MINUS(ops,'W'))
+	on |= PM_WARNNESTED;
+    else if (OPT_PLUS(ops,'W'))
+	off |= PM_WARNNESTED;
     roff = off;
     if (OPT_MINUS(ops,'z')) {
 	on |= PM_ZSHSTORED;
diff --git a/Src/exec.c b/Src/exec.c
index 6c82643..8f4969f 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2379,9 +2379,7 @@ addvars(Estate state, Wordcode pc, int addflags)
      * to be restored after the command, since then the assignment
      * is implicitly scoped.
      */
-    flags = (!(addflags & ADDVAR_RESTORE) &&
-	     locallevel > forklevel && isset(WARNCREATEGLOBAL)) ?
-	ASSPM_WARN_CREATE : 0;
+    flags = !(addflags & ADDVAR_RESTORE) ? ASSPM_WARN : 0;
     xtr = isset(XTRACE);
     if (xtr) {
 	printprompt4();
@@ -5431,6 +5429,14 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	    else
 		opts[XTRACE] = 0;
 	}
+	if (flags & PM_WARNNESTED)
+	    opts[WARNNESTEDVAR] = 1;
+	else if (oflags & PM_WARNNESTED) {
+	    if (shfunc->node.nam == ANONYMOUS_FUNCTION_NAME)
+		flags |= PM_WARNNESTED;
+	    else
+		opts[WARNNESTEDVAR] = 0;
+	}
 	ooflags = oflags;
 	/*
 	 * oflags is static, because we compare it on the next recursive
@@ -5549,6 +5555,7 @@ doshfunc(Shfunc shfunc, LinkList doshargs, int noreturnval)
 	    opts[PRINTEXITVALUE] = saveopts[PRINTEXITVALUE];
 	    opts[LOCALOPTIONS] = saveopts[LOCALOPTIONS];
 	    opts[LOCALLOOPS] = saveopts[LOCALLOOPS];
+	    opts[WARNNESTEDVAR] = saveopts[WARNNESTEDVAR];
 	}
 
 	if (opts[LOCALLOOPS]) {
diff --git a/Src/options.c b/Src/options.c
index 4729ba5..e0b67d2 100644
--- a/Src/options.c
+++ b/Src/options.c
@@ -258,6 +258,7 @@ static struct optname optns[] = {
 {{NULL, "verbose",	      0},			 VERBOSE},
 {{NULL, "vi",		      0},			 VIMODE},
 {{NULL, "warncreateglobal",   OPT_EMULATE},		 WARNCREATEGLOBAL},
+{{NULL, "warnnestedvar",      OPT_EMULATE},		 WARNNESTEDVAR},
 {{NULL, "xtrace",	      0},			 XTRACE},
 {{NULL, "zle",		      OPT_SPECIAL},		 USEZLE},
 {{NULL, "braceexpand",	      OPT_ALIAS}, /* ksh/bash */ -IGNOREBRACES},
diff --git a/Src/params.c b/Src/params.c
index d490466..ebdd252 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2849,20 +2849,47 @@ gethkparam(char *s)
     return NULL;
 }
 
+/*
+ * Function behind WARNCREATEGLOBAL and WARNNESTEDVAR option.
+ *
+ * For WARNNESTEDVAR:
+ * Called when the variable is created.
+ * Apply heuristics to see if this variable was just created
+ * globally but in a local context.
+ *
+ * For WARNNESTEDVAR:
+ * Called when the variable already exists and is set.
+ * Apply heuristics to see if this variable is setting
+ * a variable that was created in a less nested function
+ * or globally.
+ */
+
 /**/
 static void
-check_warn_create(Param pm, const char *pmtype)
+check_warn_pm(Param pm, const char *pmtype, int created)
 {
     Funcstack i;
 
-    if (pm->level != 0 || (pm->node.flags & PM_SPECIAL))
+    if (created && isset(WARNCREATEGLOBAL)) {
+	if (locallevel <= forklevel || pm->level != 0)
+	    return;
+    } else if (!created && isset(WARNNESTEDVAR)) {
+	if (pm->level >= locallevel)
+	    return;
+    } else
+	return;
+
+    if (pm->node.flags & PM_SPECIAL)
 	return;
 
     for (i = funcstack; i; i = i->prev) {
 	if (i->tp == FS_FUNC) {
+	    char *msg;
 	    DPUTS(!i->name, "funcstack entry with no name");
-	    zwarn("%s parameter %s created globally in function %s",
-		  pmtype, pm->node.nam, i->name);
+	    msg = created ?
+		"%s parameter %s created globally in function %s" :
+		"%s parameter %s set in enclosing scope in function %s";
+	    zwarn(msg, pmtype, pm->node.nam, i->name);
 	    break;
 	}
     }
@@ -2923,8 +2950,8 @@ assignsparam(char *s, char *val, int flags)
 	/* errflag |= ERRFLAG_ERROR; */
 	return NULL;
     }
-    if (flags & ASSPM_WARN_CREATE)
-	check_warn_create(v->pm, "scalar");
+    if (flags & ASSPM_WARN)
+	check_warn_pm(v->pm, "scalar", flags & ASSPM_WARN_CREATE);
     if (flags & ASSPM_AUGMENT) {
 	if (v->start == 0 && v->end == -1) {
 	    switch (PM_TYPE(v->pm->node.flags)) {
@@ -3005,9 +3032,7 @@ assignsparam(char *s, char *val, int flags)
 mod_export Param
 setsparam(char *s, char *val)
 {
-    return assignsparam(
-	s, val, isset(WARNCREATEGLOBAL) && locallevel > forklevel ?
-	ASSPM_WARN_CREATE : 0);
+    return assignsparam(s, val, ASSPM_WARN);
 }
 
 /**/
@@ -3074,8 +3099,8 @@ assignaparam(char *s, char **val, int flags)
 	    return NULL;
 	}
 
-    if (flags & ASSPM_WARN_CREATE)
-	check_warn_create(v->pm, "array");
+    if (flags & ASSPM_WARN)
+	check_warn_pm(v->pm, "array", flags & ASSPM_WARN_CREATE);
     if (flags & ASSPM_AUGMENT) {
     	if (v->start == 0 && v->end == -1) {
 	    if (PM_TYPE(v->pm->node.flags) & PM_ARRAY) {
@@ -3103,9 +3128,7 @@ assignaparam(char *s, char **val, int flags)
 mod_export Param
 setaparam(char *s, char **aval)
 {
-    return assignaparam(
-	s, aval, isset(WARNCREATEGLOBAL) && locallevel > forklevel ?
-	ASSPM_WARN_CREATE : 0);
+    return assignaparam(s, aval, ASSPM_WARN);
 }
 
 /**/
@@ -3134,7 +3157,7 @@ sethparam(char *s, char **val)
     queue_signals();
     if (!(v = fetchvalue(&vbuf, &s, 1, SCANPM_ASSIGNING))) {
 	createparam(t, PM_HASHED);
-	checkcreate = isset(WARNCREATEGLOBAL) && locallevel > forklevel;
+	checkcreate = 1;
     } else if (!(PM_TYPE(v->pm->node.flags) & PM_HASHED) &&
 	     !(v->pm->node.flags & PM_SPECIAL)) {
 	unsetparam(t);
@@ -3148,8 +3171,7 @@ sethparam(char *s, char **val)
 	    /* errflag |= ERRFLAG_ERROR; */
 	    return NULL;
 	}
-    if (checkcreate)
-	check_warn_create(v->pm, "associative array");
+    check_warn_pm(v->pm, "associative array", checkcreate);
     setarrvalue(v, val);
     unqueue_signals();
     return v->pm;
@@ -3214,8 +3236,9 @@ setnparam(char *s, mnumber val)
 	    unqueue_signals();
 	    return NULL;
 	}
-	if (!was_unset && isset(WARNCREATEGLOBAL) && locallevel > forklevel)
-	    check_warn_create(v->pm, "numeric");
+	check_warn_pm(v->pm, "numeric", !was_unset);
+    } else {
+	check_warn_pm(v->pm, "numeric", 0);
     }
     setnumvalue(v, val);
     unqueue_signals();
@@ -3250,10 +3273,7 @@ setiparam_no_convert(char *s, zlong val)
      */
     char buf[BDIGBUFSIZE];
     convbase(buf, val, 10);
-    return assignsparam(
-	s, ztrdup(buf),
-	isset(WARNCREATEGLOBAL) && locallevel > forklevel ?
-	ASSPM_WARN_CREATE : 0);
+    return assignsparam(s, ztrdup(buf), ASSPM_WARN);
 }
 
 /* Unset a parameter */
diff --git a/Src/zsh.h b/Src/zsh.h
index 7d18333..d022260 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1815,6 +1815,7 @@ struct tieddata {
 #define PM_HIDE		(1<<14)	/* Special behaviour hidden by local        */
 #define PM_CUR_FPATH    (1<<14) /* (function): can use $fpath with filename */
 #define PM_HIDEVAL	(1<<15)	/* Value not shown in `typeset' commands    */
+#define PM_WARNNESTED   (1<<15) /* (function): non-recursive WARNNESTEDVAR  */
 #define PM_TIED 	(1<<16)	/* array tied to colon-path or v.v.         */
 #define PM_TAGGED_LOCAL (1<<16) /* (function): non-recursive PM_TAGGED      */
 
@@ -2016,9 +2017,15 @@ struct paramdef {
  * Flags for assignsparam and assignaparam.
  */
 enum {
+    /* Add to rather than override value */
     ASSPM_AUGMENT = 1 << 0,
+    /* Test for warning if creating global variable in function */
     ASSPM_WARN_CREATE = 1 << 1,
-    ASSPM_ENV_IMPORT = 1 << 2
+    /* Test for warning if using nested variable in function */
+    ASSPM_WARN_NESTED = 1 << 2,
+    ASSPM_WARN = (ASSPM_WARN_CREATE|ASSPM_WARN_NESTED),
+    /* Import from environment, so exercise care evaluating value */
+    ASSPM_ENV_IMPORT = 1 << 3,
 };
 
 /* node for named directory hash table (nameddirtab) */
@@ -2400,6 +2407,7 @@ enum {
     VERBOSE,
     VIMODE,
     WARNCREATEGLOBAL,
+    WARNNESTEDVAR,
     XTRACE,
     USEZLE,
     DVORAK,
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index 45df9f5..bcd89f7 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -1118,7 +1118,8 @@
     integer foo6=9
     (( foo6=10 ))
   }
-  fn
+  # don't pollute the test environment with the variables...
+  (fn)
 0:WARN_CREATE_GLOBAL option
 ?fn:3: scalar parameter foo1 created globally in function fn
 ?fn:5: scalar parameter foo1 created globally in function fn
@@ -1133,6 +1134,60 @@
   fn
 0:WARN_CREATE_GLOBAL negative cases
 
+  (
+    foo1=global1 foo2=global2 foo3=global3 foo4=global4
+    integer foo5=5
+    fn_wnv() {
+       # warns
+       foo1=bar1
+       # doesn't warn
+       local foo2=bar3
+       unset foo2
+       # still doesn't warn
+       foo2=bar4
+       # doesn't warn
+       typeset -g foo3=bar5
+       # warns
+       foo3=bar6
+       fn2() {
+          # warns if global option, not attribute
+          foo3=bar6
+       }
+       fn2
+       # doesn't warn
+       foo4=bar7 =true
+       # warns
+       (( foo5=8 ))
+       integer foo6=9
+       # doesn't warn
+       (( foo6=10 ))
+    }
+    print option off >&2
+    fn_wnv
+    print option on >&2
+    setopt warnnestedvar
+    fn_wnv
+    unsetopt warnnestedvar
+    print function attribute on >&2
+    functions -W fn_wnv
+    fn_wnv
+    print all off again >&2
+    functions +W fn_wnv
+    fn_wnv
+  )
+0:WARN_NESTED_VAR option
+?option off
+?option on
+?fn_wnv:2: scalar parameter foo1 set in enclosing scope in function fn_wnv
+?fn_wnv:11: scalar parameter foo3 set in enclosing scope in function fn_wnv
+?fn2:2: scalar parameter foo3 set in enclosing scope in function fn2
+?fn_wnv:20: numeric parameter foo5 set in enclosing scope in function fn_wnv
+?function attribute on
+?fn_wnv:2: scalar parameter foo1 set in enclosing scope in function fn_wnv
+?fn_wnv:11: scalar parameter foo3 set in enclosing scope in function fn_wnv
+?fn_wnv:20: numeric parameter foo5 set in enclosing scope in function fn_wnv
+?all off again
+
 # This really just tests if XTRACE is egregiously broken.
 # To test it properly would need a full set of its own.
   fn() { print message; }

Re: LOCAL_VARS option ?

[Bart Schaefer]

On Fri, 20 Jan 2017, Peter Stephenson wrote:

> Here's a first go at the warning option, with "functions -W" to turn it on
> in the same fashion as "functions -T".  The option is called
> WARN_NESTED_VAR for now.

This is almost exactly what I was thinking ... except I wasn't thinking
of it having a name that could be accessed with setopt.  I was thinking
more of something that could ONLY be activated by "functions -W".

Still, I can see it either way, so thanks for the prototype.

> The main drawback over WARN_CREATE_GLOBAL is that "typeset -g" doesn't
> mark the variable for future assignments; it's still in a global scope
> so still gets a warning next time.  I think that's correct behaviour

It certainly seems reasonable.

Possible logical extensions would be to warn only if the variable is
truly in global scope, or to warn only if the variable is NOT in global
scope (i.e., is local to some caller's scope).  If -W were implemented
in some other way than as a setopt, it could accept arguments (along
the lines of gcc -W...) to indicate different kinds of warnings.

Re: LOCAL_VARS option ?

[Peter Stephenson]

On Sun, 22 Jan 2017 10:45:21 -0800 (PST)
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Fri, 20 Jan 2017, Peter Stephenson wrote:
> 
> > Here's a first go at the warning option, with "functions -W" to turn it on
> > in the same fashion as "functions -T".  The option is called
> > WARN_NESTED_VAR for now.
> 
> This is almost exactly what I was thinking ... except I wasn't thinking
> of it having a name that could be accessed with setopt.  I was thinking
> more of something that could ONLY be activated by "functions -W".

I did think about this.  The other way just gives a hidden variable
which is still there but you can't access, so the shell sort of had the
capability to apply this to a whole hierarchy of functions at once but
you couldn't actually use it.  That didn't seem particularly useful.

> Possible logical extensions would be to warn only if the variable is
> truly in global scope, or to warn only if the variable is NOT in global
> scope (i.e., is local to some caller's scope).  If -W were implemented
> in some other way than as a setopt, it could accept arguments (along
> the lines of gcc -W...) to indicate different kinds of warnings.

I'm not sure I'd want it to be complicated enough not to be at least
encodable as an option, though as it's for debugging I suppose it's not
that big a deal.

We've got a byte's worth of data with options we could use more
expressively anyway.  I think we've vaguely discussed this before.

pws

Re: LOCAL_VARS option ?

[Peter Stephenson]

I've committed this for further examination.

> We've got a byte's worth of data with options we could use more
> expressively anyway.  I think we've vaguely discussed this before.

This could be done in a fairly non-disruptive way if we ever need it.
Extend the current syntax to setopt option=(off|on) and allow individual
options to extend the mapping (off|on|other1|other2).  isset(X) is simply
(opts[X]) so the extra detail is already there if you want to get it out.

This won't stop the current init trickery with a value of 2 for certain
existing options which are never going to get public additional values.

pws

Re: LOCAL_VARS option ?

[Daniel Shahaf]

Peter Stephenson wrote on Mon, Jan 23, 2017 at 10:09:14 +0000:
> I've committed this for further examination.

Thanks!  (not only for the patch, but also for the shiny new docstring
it adds)

I'm tempted to add it to zsh-syntax-highlighting as:
.
    setopt localoptions warncreateglobal ${(k)options[(I)warnnestedvar]}
.
which should also work when ${options} is unset and in released shells
that lack WARN_NESTED_VAR.

> > We've got a byte's worth of data with options we could use more
> > expressively anyway.  I think we've vaguely discussed this before.
> 
> This could be done in a fairly non-disruptive way if we ever need it.
> Extend the current syntax to setopt option=(off|on) and allow individual
> options to extend the mapping (off|on|other1|other2).  isset(X) is simply
> (opts[X]) so the extra detail is already there if you want to get it out.

For extensibility, should we make the -W option take an argument?  I.e.,
«functions -W '' foo» instead of «functions -W foo».  We can permit just
one value for the argument for now; the point is to leave room for
future extensions.

> This won't stop the current init trickery with a value of 2 for certain
> existing options which are never going to get public additional values.

Re: LOCAL_VARS option ?

[Peter Stephenson]

On Mon, 23 Jan 2017 11:20:08 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> For extensibility, should we make the -W option take an argument?  I.e.,
> «functions -W '' foo» instead of «functions -W foo».  We can permit just
> one value for the argument for now; the point is to leave room for
> future extensions.

I suppose that depends how likely we are to need the extra values.  As I
said, I'm not really that motivated to do anything more complicated
until someone points out something they actually need to (rather than
merely could in principle) test for. We need to decide this before the
next release, but not necessarily instantly.

pws

Re: LOCAL_VARS option ?

[Daniel Shahaf]

Daniel Shahaf wrote on Mon, Jan 23, 2017 at 11:20:08 +0000:
> Peter Stephenson wrote on Mon, Jan 23, 2017 at 10:09:14 +0000:
> > I've committed this for further examination.
> 
> Thanks!  (not only for the patch, but also for the shiny new docstring
> it adds)
> 
> I'm tempted to add it to zsh-syntax-highlighting as:
> .
>     setopt localoptions warncreateglobal ${(k)options[(I)warnnestedvar]}
> .
> which should also work when ${options} is unset and in released shells
> that lack WARN_NESTED_VAR.

This case seems to be a false positive:

% () { typeset -A a; : ${a[hello world]::=foo} } 
(anon): scalar parameter hello world set in enclosing scope in function (anon)

Re: LOCAL_VARS option ?

[Peter Stephenson]

On Wed, 25 Jan 2017 05:50:09 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> This case seems to be a false positive:
> 
> % () { typeset -A a; : ${a[hello world]::=foo} } 
> (anon): scalar parameter hello world set in enclosing scope in function (anon)

There's a bogus parameter created for assistance in this case.  I didn't
see what was going on so I didn't turn off the new warning.

By the way, you won't get a warning in a case like this:

() {
  local var=(one two)
  () { var[3]=three; }
  print $var
}

which is probably OK because setting an element of something already
presupposes it exists.  The WARN_CREATE_GLOBAL equivalent does operate
here, so you're protected if it doesn't exist.  You also get a warning
if you trash the whole array:

() {
  local var=(one two)
  () { var=(three); }
  print $var
}

However, you don't get a warning if you change the array to something
else:

() {
  local var=(one two)
  () { var=three; }
  print $var
}

That's a crucial case for protecting against problems and needs looking
at in the tortuous type conversion logic.

pws


diff --git a/Src/params.c b/Src/params.c
index ebdd252..a629cf4 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2926,7 +2926,11 @@ assignsparam(char *s, char *val, int flags)
 		unqueue_signals();
 		return NULL;
 	    }
-	    flags &= ~ASSPM_WARN_CREATE;
+	    /*
+	     * Parameter defined here is a temporary bogus one.
+	     * Don't warn about anything.
+	     */
+	    flags &= ~ASSPM_WARN;
 	}
 	*ss = '[';
 	v = NULL;
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index bcd89f7..fd3263a 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -1188,6 +1188,21 @@
 ?fn_wnv:20: numeric parameter foo5 set in enclosing scope in function fn_wnv
 ?all off again
 
+
+  (
+    setopt warnnestedvar
+    () {
+      typeset -A a
+      : ${a[hello world]::=foo}
+      print ${(t)a}
+      key="hello world"
+      print $a[$key]
+    }
+  )
+0:No false positive on parameter used with subscripted assignment
+>association-local
+>foo
+
 # This really just tests if XTRACE is egregiously broken.
 # To test it properly would need a full set of its own.
   fn() { print message; }

Re: LOCAL_VARS option ?

[Daniel Shahaf]

Peter Stephenson wrote on Wed, Jan 25, 2017 at 09:24:38 +0000:
> On Wed, 25 Jan 2017 05:50:09 +0000
> Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > This case seems to be a false positive:
> > 
> > % () { typeset -A a; : ${a[hello world]::=foo} } 
> > (anon): scalar parameter hello world set in enclosing scope in function (anon)
> 
> There's a bogus parameter created for assistance in this case.  I didn't
> see what was going on so I didn't turn off the new warning.

Thanks for the fix.

> By the way, you won't get a warning in a case like this:
> 
> () {
>   local var=(one two)
>   () { var[3]=three; }
>   print $var
> }
> 
> which is probably OK because setting an element of something already
> presupposes it exists.
>

It does warn if the inner function assigns «var[3]=(three)».  I suppose
it shouldn't, for the same reason as the above case?

That would also enable «var[1,-1]=(...)» as an idiom to intentionally
overwrite an array declared in a parent function, without using the
reserved word 'typeset -ga' syntax.  (which is useful for scripts that
need to be compatible with older zsh's)

> The WARN_CREATE_GLOBAL equivalent does operate here, so you're
> protected if it doesn't exist.  You also get a warning if you trash
> the whole array:
> 
> () {
>   local var=(one two)
>   () { var=(three); }
>   print $var
> }

Cheers,

Daniel

> However, you don't get a warning if you change the array to something
> else:
> 
> () {
>   local var=(one two)
>   () { var=three; }
>   print $var
> }
> 
> That's a crucial case for protecting against problems and needs looking
> at in the tortuous type conversion logic.

Re: LOCAL_VARS option ?

[Bart Schaefer]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 385 bytes --]

On Wed, 25 Jan 2017, Daniel Shahaf wrote:

> It does warn if the inner function assigns «var[3]=(three)».  I suppose
> it shouldn't, for the same reason as the above case?

It warns in that instance because inserting a slice into an array goes
through the same code branch as assigning to the array as a whole (IIRC).
It is probably not possible to selectively suppress this warning.

Re: LOCAL_VARS option ?

[Daniel Shahaf]

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

Bart Schaefer wrote on Wed, Jan 25, 2017 at 13:50:06 -0800:
> On Wed, 25 Jan 2017, Daniel Shahaf wrote:
> 
> > It does warn if the inner function assigns «var[3]=(three)».  I suppose
> > it shouldn't, for the same reason as the above case?
> 
> It warns in that instance because inserting a slice into an array goes
> through the same code branch as assigning to the array as a whole (IIRC).
> It is probably not possible to selectively suppress this warning.

Patch attached.

Cheers,

Daniel

[-- Attachment #2: 0001-WARN_NESTED_VAR-Don-t-warn-when-assigning-to-a-slice.patch --]
[-- Type: text/x-diff, Size: 3264 bytes --]

>From a97270dbee2e2169ddf99aeb7fc8ac902263a944 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Sun, 29 Jan 2017 21:17:48 +0000
Subject: [PATCH] WARN_NESTED_VAR: Don't warn when assigning to a slice of an
 existing array

---
 Src/params.c         | 19 +++++++++++++------
 Test/E01options.ztst |  4 ++++
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/Src/params.c b/Src/params.c
index a8d4f50..20abe6a 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2866,10 +2866,14 @@ gethkparam(char *s)
 
 /**/
 static void
-check_warn_pm(Param pm, const char *pmtype, int created)
+check_warn_pm(Param pm, const char *pmtype, int created,
+	      int may_warn_about_nested_vars)
 {
     Funcstack i;
 
+    if (!may_warn_about_nested_vars && !created)
+	return;
+
     if (created && isset(WARNCREATEGLOBAL)) {
 	if (locallevel <= forklevel || pm->level != 0)
 	    return;
@@ -2956,7 +2960,7 @@ assignsparam(char *s, char *val, int flags)
 	return NULL;
     }
     if (flags & ASSPM_WARN)
-	check_warn_pm(v->pm, "scalar", created);
+	check_warn_pm(v->pm, "scalar", created, 1);
     if (flags & ASSPM_AUGMENT) {
 	if (v->start == 0 && v->end == -1) {
 	    switch (PM_TYPE(v->pm->node.flags)) {
@@ -3049,6 +3053,7 @@ assignaparam(char *s, char **val, int flags)
     char *t = s;
     char *ss;
     int created = 0;
+    int may_warn_about_nested_vars = 1;
 
     if (!isident(s)) {
 	zerr("not an identifier: %s", s);
@@ -3062,6 +3067,8 @@ assignaparam(char *s, char **val, int flags)
 	if (!(v = getvalue(&vbuf, &s, 1))) {
 	    createparam(t, PM_ARRAY);
 	    created = 1;
+	} else {
+	    may_warn_about_nested_vars = 0;
 	}
 	*ss = '[';
 	if (v && PM_TYPE(v->pm->node.flags) == PM_HASHED) {
@@ -3105,7 +3112,7 @@ assignaparam(char *s, char **val, int flags)
 	}
 
     if (flags & ASSPM_WARN)
-	check_warn_pm(v->pm, "array", created);
+	check_warn_pm(v->pm, "array", created, may_warn_about_nested_vars);
     if (flags & ASSPM_AUGMENT) {
     	if (v->start == 0 && v->end == -1) {
 	    if (PM_TYPE(v->pm->node.flags) & PM_ARRAY) {
@@ -3176,7 +3183,7 @@ sethparam(char *s, char **val)
 	    /* errflag |= ERRFLAG_ERROR; */
 	    return NULL;
 	}
-    check_warn_pm(v->pm, "associative array", checkcreate);
+    check_warn_pm(v->pm, "associative array", checkcreate, 1);
     setarrvalue(v, val);
     unqueue_signals();
     return v->pm;
@@ -3241,9 +3248,9 @@ setnparam(char *s, mnumber val)
 	    unqueue_signals();
 	    return NULL;
 	}
-	check_warn_pm(v->pm, "numeric", !was_unset);
+	check_warn_pm(v->pm, "numeric", !was_unset, 1);
     } else {
-	check_warn_pm(v->pm, "numeric", 0);
+	check_warn_pm(v->pm, "numeric", 0, 1);
     }
     setnumvalue(v, val);
     unqueue_signals();
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index 343d5a9..2bd4fdb 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -1137,6 +1137,8 @@
   (
     foo1=global1 foo2=global2 foo3=global3 foo4=global4
     integer foo5=5
+    # skip foo6, defined in fn_wnv
+    foo7=(one two)
     fn_wnv() {
        # warns
        foo1=bar1
@@ -1161,6 +1163,8 @@
        integer foo6=9
        # doesn't warn
        (( foo6=10 ))
+       foo7[3]=three
+       foo7[4]=(four)
     }
     print option off >&2
     fn_wnv

Re: LOCAL_VARS option ?

[Peter Stephenson]

On Wed, 25 Jan 2017 09:24:38 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> However, you don't get a warning if you change the array to something
> else:
> 
> () {
>   local var=(one two)
>   () { var=three; }
>   print $var
> }
> 
> That's a crucial case for protecting against problems and needs looking
> at in the tortuous type conversion logic.

This wasn't particularly tortuous, simply that when I extended the
create warning logic I handled this one rather badly.

I'll commit this anyway if the message doesn't get out quickly.

pws

diff --git a/Src/params.c b/Src/params.c
index c38f2e0..45b37cb 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2905,7 +2905,7 @@ assignsparam(char *s, char *val, int flags)
     char *ss, *copy, *var;
     size_t lvar;
     mnumber lhs, rhs;
-    int sstart;
+    int sstart, created = 0;
 
     if (!isident(s)) {
 	zerr("not an identifier: %s", s);
@@ -2916,9 +2916,10 @@ assignsparam(char *s, char *val, int flags)
     queue_signals();
     if ((ss = strchr(s, '['))) {
 	*ss = '\0';
-	if (!(v = getvalue(&vbuf, &s, 1)))
+	if (!(v = getvalue(&vbuf, &s, 1))) {
 	    createparam(t, PM_ARRAY);
-	else {
+	    created = 1;
+	} else {
 	    if (v->pm->node.flags & PM_READONLY) {
 		zerr("read-only variable: %s", v->pm->node.nam);
 		*ss = '[';
@@ -2935,18 +2936,18 @@ assignsparam(char *s, char *val, int flags)
 	*ss = '[';
 	v = NULL;
     } else {
-	if (!(v = getvalue(&vbuf, &s, 1)))
+	if (!(v = getvalue(&vbuf, &s, 1))) {
 	    createparam(t, PM_SCALAR);
-	else if ((((v->pm->node.flags & PM_ARRAY) && !(flags & ASSPM_AUGMENT)) ||
+	    created = 1;
+	} else if ((((v->pm->node.flags & PM_ARRAY) && !(flags & ASSPM_AUGMENT)) ||
 	    	 (v->pm->node.flags & PM_HASHED)) &&
 		 !(v->pm->node.flags & (PM_SPECIAL|PM_TIED)) && 
 		 unset(KSHARRAYS)) {
 	    unsetparam(t);
 	    createparam(t, PM_SCALAR);
+	    /* not regarded as a new creation */
 	    v = NULL;
 	}
-	else
-	    flags &= ~ASSPM_WARN_CREATE;
     }
     if (!v && !(v = getvalue(&vbuf, &t, 1))) {
 	unqueue_signals();
@@ -2955,7 +2956,7 @@ assignsparam(char *s, char *val, int flags)
 	return NULL;
     }
     if (flags & ASSPM_WARN)
-	check_warn_pm(v->pm, "scalar", flags & ASSPM_WARN_CREATE);
+	check_warn_pm(v->pm, "scalar", created);
     if (flags & ASSPM_AUGMENT) {
 	if (v->start == 0 && v->end == -1) {
 	    switch (PM_TYPE(v->pm->node.flags)) {
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index fd3263a..c265d78 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -1203,6 +1203,18 @@
 >association-local
 >foo
 
+  (
+    setopt warnnestedvar
+    () {
+      local var=(one two)
+      () { var=three; }
+      print $var
+    }
+  )
+0:Warn when changing type of nested variable.
+?(anon): scalar parameter var set in enclosing scope in function (anon)
+>three
+
 # This really just tests if XTRACE is egregiously broken.
 # To test it properly would need a full set of its own.
   fn() { print message; }

Re: LOCAL_VARS option ?

[Peter Stephenson]

On Thu, 26 Jan 2017 19:43:28 +0000
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> This wasn't particularly tortuous, simply that when I extended the
> create warning logic I handled this one rather badly.

There's more!

diff --git a/Src/params.c b/Src/params.c
index 45b37cb..a8d4f50 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -3048,6 +3048,7 @@ assignaparam(char *s, char **val, int flags)
     Value v;
     char *t = s;
     char *ss;
+    int created = 0;
 
     if (!isident(s)) {
 	zerr("not an identifier: %s", s);
@@ -3058,10 +3059,10 @@ assignaparam(char *s, char **val, int flags)
     queue_signals();
     if ((ss = strchr(s, '['))) {
 	*ss = '\0';
-	if (!(v = getvalue(&vbuf, &s, 1)))
+	if (!(v = getvalue(&vbuf, &s, 1))) {
 	    createparam(t, PM_ARRAY);
-	else
-	    flags &= ~ASSPM_WARN_CREATE;
+	    created = 1;
+	}
 	*ss = '[';
 	if (v && PM_TYPE(v->pm->node.flags) == PM_HASHED) {
 	    unqueue_signals();
@@ -3073,9 +3074,10 @@ assignaparam(char *s, char **val, int flags)
 	}
 	v = NULL;
     } else {
-	if (!(v = fetchvalue(&vbuf, &s, 1, SCANPM_ASSIGNING)))
+	if (!(v = fetchvalue(&vbuf, &s, 1, SCANPM_ASSIGNING))) {
 	    createparam(t, PM_ARRAY);
-	else if (!(PM_TYPE(v->pm->node.flags) & (PM_ARRAY|PM_HASHED)) &&
+	    created = 1;
+	} else if (!(PM_TYPE(v->pm->node.flags) & (PM_ARRAY|PM_HASHED)) &&
 		 !(v->pm->node.flags & (PM_SPECIAL|PM_TIED))) {
 	    int uniq = v->pm->node.flags & PM_UNIQUE;
 	    if (flags & ASSPM_AUGMENT) {
@@ -3093,8 +3095,6 @@ assignaparam(char *s, char **val, int flags)
 	    createparam(t, PM_ARRAY | uniq);
 	    v = NULL;
 	}
-	else
-	    flags &= ~ASSPM_WARN_CREATE;
     }
     if (!v)
 	if (!(v = fetchvalue(&vbuf, &t, 1, SCANPM_ASSIGNING))) {
@@ -3105,7 +3105,7 @@ assignaparam(char *s, char **val, int flags)
 	}
 
     if (flags & ASSPM_WARN)
-	check_warn_pm(v->pm, "array", flags & ASSPM_WARN_CREATE);
+	check_warn_pm(v->pm, "array", created);
     if (flags & ASSPM_AUGMENT) {
     	if (v->start == 0 && v->end == -1) {
 	    if (PM_TYPE(v->pm->node.flags) & PM_ARRAY) {
diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index c265d78..343d5a9 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -1211,10 +1211,22 @@
       print $var
     }
   )
-0:Warn when changing type of nested variable.
+0:Warn when changing type of nested variable: array to scalar.
 ?(anon): scalar parameter var set in enclosing scope in function (anon)
 >three
 
+  (
+    setopt warnnestedvar
+    () {
+      local var=three
+      () { var=(one two); }
+      print $var
+    }
+  )
+0:Warn when changing type of nested variable: scalar to array.
+?(anon): array parameter var set in enclosing scope in function (anon)
+>one two
+
 # This really just tests if XTRACE is egregiously broken.
 # To test it properly would need a full set of its own.
   fn() { print message; }