buggy WARN_CREATE_GLOBAL warning

buggy WARN_CREATE_GLOBAL warning

[Vincent Lefevre]

Consider the following script:

----------------------------------------
#!/usr/bin/env zsh

setopt WARN_CREATE_GLOBAL

foo()
{
  local blah=$(TZ=UTC date)
}

foo
----------------------------------------

I get:

foo:2: scalar parameter TZ created globally in function foo

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)

Re: buggy WARN_CREATE_GLOBAL warning

[Peter Stephenson]

On Fri, 4 Dec 2015 03:51:55 +0100
Vincent Lefevre <vincent@vinc17.net> wrote:
> setopt WARN_CREATE_GLOBAL
> 
> foo()
> {
>   local blah=$(TZ=UTC date)
> }
> 
> foo
> ----------------------------------------
> 
> I get:
> 
> foo:2: scalar parameter TZ created globally in function foo

I think the following is logically correct --- variables can't propagate
back from forks so you don't care if they're notionally global.

However, I don't actually understand why this produces a warning and the
normal case

foo()
{
   TX=UTC data
}
foo

doesn't.  That is, I can see the test that means that the normal case
doesn't, but I can't see why the warning becomes active when in the
command substitution.

Don't read this after a heavy lunch, but I suspect it's some subtlety to
do with the fact that we know we're about to exit the subshell after the
substitution so aren't set up to restore the variable ("set up to" ---
but if we forked we wouldn't actually need to restore, though if it's
e.g. a shell function we would), so addvars() doesn't know we're in the
export-for-a-command case.  If that's right, then this fix ought to be
robust because the reason we're not worried about restoring variables is
the same reason we don't care about the locallevel.

It would almost certainly make more sense to move the locallevel test
down to where the ASSPM_WARN_CREATE flag is tested; the trouble is it's
quite hard to get your mind round all the possible side effects.

pws

diff --git a/Src/exec.c b/Src/exec.c
index fc31c6b..acc867c 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2264,7 +2264,7 @@ addvars(Estate state, Wordcode pc, int addflags)
      * is implicitly scoped.
      */
     flags = (!(addflags & ADDVAR_RESTORE) &&
-	     locallevel > 0 && isset(WARNCREATEGLOBAL)) ?
+	     locallevel > forklevel && isset(WARNCREATEGLOBAL)) ?
 	ASSPM_WARN_CREATE : 0;
     xtr = isset(XTRACE);
     if (xtr) {
diff --git a/Src/params.c b/Src/params.c
index 142697f..aed72d4 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -2868,7 +2868,7 @@ mod_export Param
 setsparam(char *s, char *val)
 {
     return assignsparam(
-	s, val, isset(WARNCREATEGLOBAL) && locallevel > 0 ?
+	s, val, isset(WARNCREATEGLOBAL) && locallevel > forklevel ?
 	ASSPM_WARN_CREATE : 0);
 }
 
@@ -2966,7 +2966,7 @@ mod_export Param
 setaparam(char *s, char **aval)
 {
     return assignaparam(
-	s, aval, isset(WARNCREATEGLOBAL) && locallevel >0 ?
+	s, aval, isset(WARNCREATEGLOBAL) && locallevel > forklevel ?
 	ASSPM_WARN_CREATE : 0);
 }
 
@@ -2997,7 +2997,7 @@ sethparam(char *s, char **val)
     if (!(v = fetchvalue(&vbuf, &s, 1, SCANPM_ASSIGNING))) {
 	DPUTS(!v, "BUG: assigning to undeclared associative array");
 	createparam(t, PM_HASHED);
-	checkcreate = isset(WARNCREATEGLOBAL) && locallevel > 0;
+	checkcreate = isset(WARNCREATEGLOBAL) && locallevel > forklevel;
     } else if (!(PM_TYPE(v->pm->node.flags) & PM_HASHED) &&
 	     !(v->pm->node.flags & PM_SPECIAL)) {
 	unsetparam(t);
@@ -3075,7 +3075,7 @@ setnparam(char *s, mnumber val)
 	    /* errflag |= ERRFLAG_ERROR; */
 	    return NULL;
 	}
-	if (!was_unset && isset(WARNCREATEGLOBAL) && locallevel > 0)
+	if (!was_unset && isset(WARNCREATEGLOBAL) && locallevel > forklevel)
 	    check_warn_create(v->pm, "numeric");
     }
     setnumvalue(v, val);
@@ -3113,7 +3113,8 @@ setiparam_no_convert(char *s, zlong val)
     convbase(buf, val, 10);
     return assignsparam(
 	s, ztrdup(buf),
-	isset(WARNCREATEGLOBAL) && locallevel > 0 ? ASSPM_WARN_CREATE : 0);
+	isset(WARNCREATEGLOBAL) && locallevel > forklevel ?
+	ASSPM_WARN_CREATE : 0);
 }
 
 /* Unset a parameter */

Re: buggy WARN_CREATE_GLOBAL warning

[Peter Stephenson]

These tests could probably do with some more cases.

pws

diff --git a/Test/E01options.ztst b/Test/E01options.ztst
index c9427c7..f270767 100644
--- a/Test/E01options.ztst
+++ b/Test/E01options.ztst
@@ -1120,6 +1120,15 @@
 ?fn:5: scalar parameter foo1 created globally in function fn
 ?fn:15: numeric parameter foo5 created globally in function fn
 
+  fn() {
+    emulate -L zsh
+    setopt warncreateglobal
+    TZ=UTC date >&/dev/null
+    local um=$(TZ=UTC date 2>/dev/null)
+  }
+  fn
+0:WARN_CREATE_GLOBAL negative cases
+
 # This really just tests if XTRACE is egregiously broken.
 # To test it properly would need a full set of its own.
   fn() { print message; }