40391: Add WARN_NESTED_VAR option and functions -W.

These are companions to WARN_CREATED_GLOBAL, warning when a variable
from an enclosing scope is altered.

10 files changed, 177 insertions, 37 deletions

diff --git a/ChangeLog b/ChangeLog
index f4667ab2c..738d2048d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2017-01-23  Peter Stephenson  <p.stephenson@samsung.com>
+
+	* 40391: Completion/compinit, Doc/Zsh/builtins.yo,
+	Doc/Zsh/options.yo, Src/builtin.c, Src/exec.c, Src/options.c,
+	Src/params.c, Src/zsh.h, Test/E01options.ztst: Add
+	WARN_NESTED_VAR option and functions -W to turn it on similarly
+	to functions -T.
+
 2017-01-23  Daniel Shahaf  <d.s@daniel.shahaf.name>
 
 	* unposted: Etc/BUGS: Record users/20807 vcs_info quilt issue.
diff --git a/Completion/compinit b/Completion/compinit
index cc663cb71..eb88a9fb9 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 3a3130a95..0a9021cb0 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 434b71094..aa6274814 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 7a04a79a7..2fb1a70a4 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 6c8264394..8f4969f52 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 4729ba54a..e0b67d205 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 d4904666f..ebdd25225 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 7d18333be..d0222605b 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 45df9f572..bcd89f787 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; }