Re: buggy WARN_CREATE_GLOBAL warning

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Fri, 4 Dec 2015 03:51:55 +0100
Vincent Lefevre <vincent@xxxxxxxxxx> 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 */