PATCH: WARN_CREATE_GLOBAL option

[Peter Stephenson <pws@xxxxxxx>]

This new option prints a warning when a global variable is created by an
assignment within a function, which is typically the sign of a parameter
that should have been declared local.  As the documentation notes, it's
possible to mark a parameter as global with typeset -g, which
circumvents the error, so it's always possible to fix your code up so
that it only gives warnings for undeclared variables.

Does anybody (particularly those with limited English) think a different
option name would be better?

Note the limitation mentioned at the end of the documentation.  You
don't get an error for this:

fn() {
  setopt warncreateglobal
  local foo=stick
  fn2() {
     foo=bar
  }
  fn2
  # foo is now bar, error or not?
}

which may or may not indicate an error.  This case is quite hard to test
for and fix up, so I've left it alone.

This case is handled properly:

fn() {
  local foo
  foo=pub
  unset foo
  foo=bar
}

with no warning, because in zsh foo=bar recreates the parameter
locally.  (I mention this explicitly because I forgot it to begin with
and had to add an extra test.)

I've explicitly turned it off for the completion code for reasons which
anyone who turns it back on will immediately note.  If someone wants to
fix up the warnings, either by making the offending parameter local or
by using typeset -g, then we could turn the option on and make the
completion system safer.  However, as has been noted recently, it's not
necessarily immediately obvious which parameters should be local.

It's possible I've missed some cases where the warning should or
shouldn't be triggered, but this particular part of the parameter code
isn't as horrendous as other parts, so it may be OK.

Index: Completion/compinit
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/compinit,v
retrieving revision 1.13
diff -u -r1.13 compinit
--- Completion/compinit	22 Oct 2004 15:52:00 -0000	1.13
+++ Completion/compinit	8 Aug 2005 13:53:50 -0000
@@ -146,6 +146,7 @@
     NO_aliases
     NO_errexit
     NO_octalzeroes
+    NO_warncreateglobal
 )
 
 # And this one should be `eval'ed at the beginning of every entry point
Index: Doc/Zsh/options.yo
===================================================================
RCS file: /cvsroot/zsh/zsh/Doc/Zsh/options.yo,v
retrieving revision 1.40
diff -u -r1.40 options.yo
--- Doc/Zsh/options.yo	26 Jul 2005 22:48:43 -0000	1.40
+++ Doc/Zsh/options.yo	8 Aug 2005 13:53:50 -0000
@@ -460,6 +460,16 @@
 Treat unset parameters as if they were empty when substituting.
 Otherwise they are treated as an error.
 )
+pindex(WARN_CREATE_GLOBAL)
+cindex(parameters, warning when created globally)
+item(tt(WARN_CREATE_GLOBAL))(
+Print a warning message when a global parameter is created in a function
+by an assignment.  This often indicates that a parameter has not been
+declared local when it should have been.  Parameters explicitly declared
+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.
+)
 enditem()
 
 subsect(History)
Index: Src/exec.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/exec.c,v
retrieving revision 1.90
diff -u -r1.90 exec.c
--- Src/exec.c	27 Apr 2005 09:58:45 -0000	1.90
+++ Src/exec.c	8 Aug 2005 13:53:51 -0000
@@ -1649,10 +1649,14 @@
     LinkList vl;
     int xtr, isstr, htok = 0;
     char **arr, **ptr, *name;
+    int flags, augment;
+
     Wordcode opc = state->pc;
     wordcode ac;
     local_list1(svl);
 
+    flags = (locallevel > 0 && isset(WARNCREATEGLOBAL)) ?
+	ASSPM_WARN_CREATE : 0;
     xtr = isset(XTRACE);
     if (xtr) {
 	printprompt4();
@@ -1660,12 +1664,15 @@
     }
     state->pc = pc;
     while (wc_code(ac = *state->pc++) == WC_ASSIGN) {
+	int myflags = flags;
 	name = ecgetstr(state, EC_DUPTOK, &htok);
 	if (htok)
 	    untokenize(name);
+	if (WC_ASSIGN_TYPE2(ac) == WC_ASSIGN_INC)
+	    myflags |= ASSPM_AUGMENT;
 	if (xtr)
 	    fprintf(xtrerr,
-	    	WC_ASSIGN_TYPE2(ac) == WC_ASSIGN_INC ? "%s+=" : "%s=", name);
+		WC_ASSIGN_TYPE2(ac) == WC_ASSIGN_INC ? "%s+=" : "%s=", name);
 	if ((isstr = (WC_ASSIGN_TYPE(ac) == WC_ASSIGN_SCALAR))) {
 	    init_list1(svl, ecgetstr(state, EC_DUPTOK, &htok));
 	    vl = &svl;
@@ -1716,12 +1723,10 @@
 		}
 		allexp = opts[ALLEXPORT];
 		opts[ALLEXPORT] = 1;
-	    	pm = assignsparam(name, val,
-		    WC_ASSIGN_TYPE2(ac) == WC_ASSIGN_INC);
+	    	pm = assignsparam(name, val, myflags);
 		opts[ALLEXPORT] = allexp;
 	    } else
-	    	pm = assignsparam(name, val,
-		    WC_ASSIGN_TYPE2(ac) == WC_ASSIGN_INC);
+	    	pm = assignsparam(name, val, myflags);
 	    if (errflag) {
 		state->pc = opc;
 		return;
@@ -1746,7 +1751,7 @@
 	    }
 	    fprintf(xtrerr, ") ");
 	}
-	assignaparam(name, arr, WC_ASSIGN_TYPE2(ac) == WC_ASSIGN_INC);
+	assignaparam(name, arr, myflags);
 	if (errflag) {
 	    state->pc = opc;
 	    return;
Index: Src/options.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/options.c,v
retrieving revision 1.23
diff -u -r1.23 options.c
--- Src/options.c	15 Jul 2005 17:41:50 -0000	1.23
+++ Src/options.c	8 Aug 2005 13:53:51 -0000
@@ -211,6 +211,7 @@
 {NULL, "unset",		      OPT_EMULATE|OPT_BSHELL,	 UNSET},
 {NULL, "verbose",	      0,			 VERBOSE},
 {NULL, "vi",		      0,			 VIMODE},
+{NULL, "warncreateglobal",    0,			 WARNCREATEGLOBAL},
 {NULL, "xtrace",	      0,			 XTRACE},
 {NULL, "zle",		      OPT_SPECIAL,		 USEZLE},
 {NULL, "braceexpand",	      OPT_ALIAS, /* ksh/bash */	 -IGNOREBRACES},
Index: Src/params.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/params.c,v
retrieving revision 1.99
diff -u -r1.99 params.c
--- Src/params.c	24 Jul 2005 05:37:23 -0000	1.99
+++ Src/params.c	8 Aug 2005 13:53:51 -0000
@@ -1990,7 +1990,7 @@
 
 /**/
 mod_export Param
-assignsparam(char *s, char *val, int augment)
+assignsparam(char *s, char *val, int flags)
 {
     struct value vbuf;
     Value v;
@@ -2011,12 +2011,14 @@
 	*ss = '\0';
 	if (!(v = getvalue(&vbuf, &s, 1)))
 	    createparam(t, PM_ARRAY);
+	else
+	    flags &= ~ASSPM_WARN_CREATE;
 	*ss = '[';
 	v = NULL;
     } else {
 	if (!(v = getvalue(&vbuf, &s, 1)))
 	    createparam(t, PM_SCALAR);
-	else if ((((v->pm->flags & PM_ARRAY) && !augment) ||
+	else if ((((v->pm->flags & PM_ARRAY) && !(flags & ASSPM_AUGMENT)) ||
 	    	 (v->pm->flags & PM_HASHED)) &&
 		 !(v->pm->flags & (PM_SPECIAL|PM_TIED)) && 
 		 unset(KSHARRAYS)) {
@@ -2024,13 +2026,18 @@
 	    createparam(t, PM_SCALAR);
 	    v = NULL;
 	}
+	else
+	    flags &= ~ASSPM_WARN_CREATE;
     }
     if (!v && !(v = getvalue(&vbuf, &t, 1))) {
 	unqueue_signals();
 	zsfree(val);
 	return NULL;
     }
-    if (augment) {
+    if ((flags & ASSPM_WARN_CREATE) && v->pm->level == 0)
+	zwarn("scalar parameter %s created globally in function",
+	      v->pm->nam, 0);
+    if (flags & ASSPM_AUGMENT) {
 	if (v->start == 0 && v->end == -1) {
 	    switch (PM_TYPE(v->pm->flags)) {
 	    case PM_SCALAR:
@@ -2109,7 +2116,7 @@
 
 /**/
 mod_export Param
-assignaparam(char *s, char **val, int augment)
+assignaparam(char *s, char **val, int flags)
 {
     struct value vbuf;
     Value v;
@@ -2127,6 +2134,8 @@
 	*ss = '\0';
 	if (!(v = getvalue(&vbuf, &s, 1)))
 	    createparam(t, PM_ARRAY);
+	else
+	    flags &= ~ASSPM_WARN_CREATE;
 	*ss = '[';
 	if (v && PM_TYPE(v->pm->flags) == PM_HASHED) {
 	    unqueue_signals();
@@ -2143,7 +2152,7 @@
 	else if (!(PM_TYPE(v->pm->flags) & (PM_ARRAY|PM_HASHED)) &&
 		 !(v->pm->flags & (PM_SPECIAL|PM_TIED))) {
 	    int uniq = v->pm->flags & PM_UNIQUE;
-	    if (augment) {
+	    if (flags & ASSPM_AUGMENT) {
 	    	/* insert old value at the beginning of the val array */
 		char **new;
 		int lv = arrlen(val);
@@ -2153,12 +2162,13 @@
 		memcpy(new+1, val, sizeof(char *) * (lv + 1));
 		free(val);
 		val = new;
-		
 	    }
 	    unsetparam(t);
 	    createparam(t, PM_ARRAY | uniq);
 	    v = NULL;
 	}
+	else
+	    flags &= ~ASSPM_WARN_CREATE;
     }
     if (!v)
 	if (!(v = fetchvalue(&vbuf, &t, 1, SCANPM_ASSIGNING))) {
@@ -2167,7 +2177,10 @@
 	    return NULL;
 	}
 
-    if (augment) {
+    if ((flags & ASSPM_WARN_CREATE) && v->pm->level == 0)
+	zwarn("array parameter %s created globally in function",
+	      v->pm->nam, 0);
+    if (flags & ASSPM_AUGMENT) {
     	if (v->start == 0 && v->end == -1) {
 	    if (PM_TYPE(v->pm->flags) & PM_ARRAY) {
 	    	v->start = arrlen(v->pm->gsu.a->getfn(v->pm));
Index: Src/zsh.h
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/zsh.h,v
retrieving revision 1.75
diff -u -r1.75 zsh.h
--- Src/zsh.h	15 Jul 2005 17:41:33 -0000	1.75
+++ Src/zsh.h	8 Aug 2005 13:53:51 -0000
@@ -1361,6 +1361,14 @@
 #define setsparam(S,V) assignsparam(S,V,0)
 #define setaparam(S,V) assignaparam(S,V,0)
 
+/*
+ * Flags for assignsparam and assignaparam.
+ */
+enum {
+    ASSPM_AUGMENT = 1 << 0,
+    ASSPM_WARN_CREATE = 1 << 1
+};
+
 /* node for named directory hash table (nameddirtab) */
 
 struct nameddir {
@@ -1624,6 +1632,7 @@
     UNSET,
     VERBOSE,
     VIMODE,
+    WARNCREATEGLOBAL,
     XTRACE,
     USEZLE,
     DVORAK,


-- 
Peter Stephenson <pws@xxxxxxx>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


**********************************************************************
This email and any files transmitted with it are confidential and
intended solely for the use of the individual or entity to whom they
are addressed. If you have received this email in error please notify
the system manager.

**********************************************************************