Bug in regexp operator when warn_create_global is in effect

Bug in regexp operator when warn_create_global is in effect

[Ronald Fischer]

Consider the following program:

   #!/bin/zsh
   function f {
      [[ x =~ x ]]
   }
   setopt warn_create_global
   f

When run by zsh 5.2 (x86_64-ubuntu-linux-gnu), I get the error messages:

f:1: scalar parameter MATCH created globally in function f
f:1: numeric parameter MBEGIN created globally in function f
f:1: numeric parameter MEND created globally in function f

While it is correct, that regexp matching sets, as side effect, the
variables mentioned here, it doesn't, IMHO, make much sense that a zsh
user, who not even *uses* these variables, receives these error
messages. If the user had to pay attention to those variables which
automatically spring into existence by zsh language constructs, doing a
cd inside a function should equally well raise the message "parameter
OLDPWD created globally in function".

I suggest that the variables MATCH, MBEGIN and MEND are already created
by zsh
as global parameters during shell startup time. This would not trigger
the warning, if someone uses regexp matching inside functions.

Ronald

Re: Bug in regexp operator when warn_create_global is in effect

[Peter Stephenson]

On Thu, 02 Feb 2017 08:37:16 +0100
Ronald Fischer <ynnor@mm.st> wrote:
> Consider the following program:
> 
>    #!/bin/zsh
>    function f {
>       [[ x =~ x ]]
>    }
>    setopt warn_create_global
>    f
> 
> When run by zsh 5.2 (x86_64-ubuntu-linux-gnu), I get the error messages:
> 
> f:1: scalar parameter MATCH created globally in function f
> f:1: numeric parameter MBEGIN created globally in function f
> f:1: numeric parameter MEND created globally in function f
> 
> While it is correct, that regexp matching sets, as side effect, the
> variables mentioned here, it doesn't, IMHO, make much sense that a zsh
> user, who not even *uses* these variables, receives these error
> messages.

Yes, you're right, the user doesn't even necessarily want them, which is
different from the case of the globbing flags in native zsh
expressions.  So probably best to turn the warnings off.

We don't yet have quite the full infrastructure for numerical
variables, annoyingly, but probably useful to add anyway...

pws

diff --git a/Src/Modules/regex.c b/Src/Modules/regex.c
index edb7234..d02769e 100644
--- a/Src/Modules/regex.c
+++ b/Src/Modules/regex.c
@@ -111,7 +111,7 @@ zcond_regex_match(char **a, int id)
 		*x = NULL;
 	    }
 	    if (isset(BASHREMATCH)) {
-		setaparam("BASH_REMATCH", arr);
+		assignaparam("BASH_REMATCH", arr, 0);
 	    } else {
 		zlong offs;
 		char *ptr;
@@ -119,7 +119,7 @@ zcond_regex_match(char **a, int id)
 
 		m = matches;
 		s = metafy(lhstr + m->rm_so, m->rm_eo - m->rm_so, META_DUP);
-		setsparam("MATCH", s);
+		assignsparam("MATCH", s, 0);
 		/*
 		 * Count the characters before the match.
 		 */
@@ -133,7 +133,7 @@ zcond_regex_match(char **a, int id)
 		    ptr += clen;
 		    leftlen -= clen;
 		}
-		setiparam("MBEGIN", offs + !isset(KSHARRAYS));
+		assigniparam("MBEGIN", offs + !isset(KSHARRAYS), 0);
 		/*
 		 * Add on the characters in the match.
 		 */
@@ -144,7 +144,7 @@ zcond_regex_match(char **a, int id)
 		    ptr += clen;
 		    leftlen -= clen;
 		}
-		setiparam("MEND", offs + !isset(KSHARRAYS) - 1);
+		assigniparam("MEND", offs + !isset(KSHARRAYS) - 1, 0);
 		if (nelem) {
 		    char **mbegin, **mend, **bptr, **eptr;
 		    bptr = mbegin = (char **)zalloc(sizeof(char *)*(nelem+1));
diff --git a/Src/params.c b/Src/params.c
index 20abe6a..19cbb1c 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -3192,11 +3192,12 @@ sethparam(char *s, char **val)
 
 /*
  * Set a generic shell number, floating point or integer.
+ * Option to warn on setting.
  */
 
 /**/
-Param
-setnparam(char *s, mnumber val)
+mod_export Param
+assignnparam(char *s, mnumber val, int flags)
 {
     struct value vbuf;
     Value v;
@@ -3248,15 +3249,41 @@ setnparam(char *s, mnumber val)
 	    unqueue_signals();
 	    return NULL;
 	}
-	check_warn_pm(v->pm, "numeric", !was_unset, 1);
+	if (flags & ASSPM_WARN)
+	    check_warn_pm(v->pm, "numeric", !was_unset, 1);
     } else {
-	check_warn_pm(v->pm, "numeric", 0, 1);
+	if (flags & ASSPM_WARN)
+	    check_warn_pm(v->pm, "numeric", 0, 1);
     }
     setnumvalue(v, val);
     unqueue_signals();
     return v->pm;
 }
 
+/*
+ * Set a generic shell number, floating point or integer.
+ * Warn on setting based on option.
+ */
+
+/**/
+mod_export Param
+setnparam(char *s, mnumber val)
+{
+    return assignnparam(s, val, ASSPM_WARN);
+}
+
+/* Simplified interface to assignnparam */
+
+/**/
+mod_export Param
+assigniparam(char *s, zlong val, int flags)
+{
+    mnumber mnval;
+    mnval.type = MN_INTEGER;
+    mnval.u.l = val;
+    return assignnparam(s, mnval, flags);
+}
+
 /* Simplified interface to setnparam */
 
 /**/
@@ -3266,7 +3293,7 @@ setiparam(char *s, zlong val)
     mnumber mnval;
     mnval.type = MN_INTEGER;
     mnval.u.l = val;
-    return setnparam(s, mnval);
+    return assignnparam(s, mnval, ASSPM_WARN);
 }
 
 /*

Re: Bug in regexp operator when warn_create_global is in effect

[Bart Schaefer]

On Feb 2,  9:43am, Peter Stephenson wrote:
}
} > While it is correct, that regexp matching sets, as side effect, the
} > variables mentioned here, it doesn't, IMHO, make much sense that a zsh
} > user, who not even *uses* these variables, receives these error
} > messages.
} 
} Yes, you're right, the user doesn't even necessarily want them
} [...].  So probably best to turn the warnings off.

Didn't we discuss this once beforeand decide exactly the opposite?

Yes, here:
    http://www.zsh.org/mla/workers//2015/msg03106.html

That seems to directly contradict what you just said/patched.

Re: Bug in regexp operator when warn_create_global is in effect

[Peter Stephenson]

On Thu, 2 Feb 2017 14:55:27 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:

> On Feb 2,  9:43am, Peter Stephenson wrote:
> }
> } > While it is correct, that regexp matching sets, as side effect, the
> } > variables mentioned here, it doesn't, IMHO, make much sense that a zsh
> } > user, who not even *uses* these variables, receives these error
> } > messages.
> } 
> } Yes, you're right, the user doesn't even necessarily want them
> } [...].  So probably best to turn the warnings off.
> 
> Didn't we discuss this once beforeand decide exactly the opposite?
> 
> Yes, here:
>     http://www.zsh.org/mla/workers//2015/msg03106.html
> 
> That seems to directly contradict what you just said/patched.

That's a fair point to bring up, and I remember the discussion now you
point it out, but that appears to have been more general --- in
particular, I believe I was mostly thinking about cases where the user
would have done something explicitly causing the variable to be set as
a key part of what they were doing rather than as a side effect.  For
example, anything setting REPLY has been called to get a reply, and in
native zsh, match / MATCH etc. are triggered by globbing flags.

I refer the honourable gentleman to the closing remark I made to the
list on that occasion:

> If we need to make exceptions, I think they're more likely to be connected
> to specific instances of setting variables internally.

I think this can be considered one such case --- the user is doing a
basic regular expression where the result they want is the result of the
test, and they may not even be aware that variables are created.  As a
second possibly significant point, this is not zsh-specific syntax, it's
allowed in other recent shells where you wouldn't expect to have to
declare the variables.

I'm sure you can argue this the other way, though.

pws

Re: Bug in regexp operator when warn_create_global is in effect

[Ronald Fischer]

> Yes, you're right, the user doesn't even necessarily want them, which is
> different from the case of the globbing flags in native zsh
> expressions.  So probably best to turn the warnings off.

I wouldn't consider this an optimal solution. This type of warning
proved to be extremely useful, and was catching already several spelling
errors in our own program.

Even worse, this new behaviour was not present in 5.1.1, and since we
are migrating a large zsh codebase from 5.1.1 to 5.2, we suddenly get
these warnings in several places.
Ronald

Re: Bug in regexp operator when warn_create_global is in effect

[Peter Stephenson]

On Mon, 06 Feb 2017 11:44:09 +0100
Ronald Fischer <ynnor@mm.st> wrote:
> > Yes, you're right, the user doesn't even necessarily want them, which is
> > different from the case of the globbing flags in native zsh
> > expressions.  So probably best to turn the warnings off.
> 
> I wouldn't consider this an optimal solution. This type of warning
> proved to be extremely useful, and was catching already several spelling
> errors in our own program.

I'm not sure what you're referring to, but the change only turns off the
warnings for the case in question, i.e. regular expression matches,
where the use of the variables is hidden, the one you were complaining
about.  You were talking about creating the relevant variables even if
they were never used, but simply not warning when they are created by
the syntax in question should be equivalent and less invasive, I think.
(Those are "the warnings" I was talking about:  I wasn't explicit, which
may have caused confusion.)

Furthermore, creating MATCH etc. would suppress warnings in other cases
where zsh-specific syntax is used to request they be created, in
particular the globbing flag (#m), and from the above you appear to be
suggesting other cases should (ideally) not be suppressed.

If you're talking about other cases, if you could show exactly what's
triggering a warning, or failing to trigger it, I'll have a look.

pws

Re: Bug in regexp operator when warn_create_global is in effect

[Ronald Fischer]

On Mon, Feb 6, 2017, at 12:11, Peter Stephenson wrote:
> On Mon, 06 Feb 2017 11:44:09 +0100
> Ronald Fischer <ynnor@mm.st> wrote:
> > > Yes, you're right, the user doesn't even necessarily want them, which is
> > > different from the case of the globbing flags in native zsh
> > > expressions.  So probably best to turn the warnings off.
> > 
> > I wouldn't consider this an optimal solution. This type of warning
> > proved to be extremely useful, and was catching already several spelling
> > errors in our own program.
> 
> I'm not sure what you're referring to, but the change only turns off the
> warnings for the case in question, i.e. regular expression matches,
> where the use of the variables is hidden, the one you were complaining
> about.  

I think I misunderstood your comment here. I thought that you did not
want to change anything in zsh, but recommend that I simply turn off
warning about creating globals inside functions. This indeed would not
have been a good idea.

Basically, I want to be warned about globals which I explicitly create
by myself, but I don't want to be warned about globals which are created
implicitly by zsh (since I don't have any control about them.

> If you're talking about other cases, if you could show exactly what's
> triggering a warning, or failing to trigger it, I'll have a look.

The case is exactly the example program I've supplied: We get a warning
about a global variable being created, while this variable is not even
mentioned anywhere in the code.

About the suggestion in my original posting: What speaks against the
idea of creating all "implicit variables" as globals implicitly, when
the shell starts execution? That is, a program

    #!/bin/zsh
    source something
    foo bar

would be equivalent (that is, internally replaced) by

    #!/bin/zsh
    MATCH=
    source something
    foo bar

If you worry that MATCH could be an exported variable in the parent
shell (and hence must not be modified), it is trivial to include this
case as well.

Ronald

Re: Bug in regexp operator when warn_create_global is in effect

[Peter Stephenson]

On Mon, 06 Feb 2017 12:26:37 +0100
Ronald Fischer <ynnor@mm.st> wrote:
> Basically, I want to be warned about globals which I explicitly create
> by myself, but I don't want to be warned about globals which are created
> implicitly by zsh (since I don't have any control about them.

You might want to look at the discussion Bart referred to in his
previous post,

http://www.zsh.org/mla/workers//2015/msg03106.html

but here's the summary.

The main point is to handle cases something like the following, where
although the variable creation is hidden it's explicitly requested by the
user:

fn() {
   [[ $1 = (#m)${~2} ]]
}

That (#m) in the pattern and the fact the pattern is in a function are
the only relevant facts here.  The (#m) is a request to cause MATCH,
MBEGIN and MEND to be set by the shell.  Because that's the purpose of
(#m), you want the warning so as to be able to decide whether those
variables are to be global or not and resolve between the two with a
local or typeset -g.

Your case is different because the use of the variables isn't requested
by special syntax, it's buried under standard regex syntax, so I've
fixed it for that one case.  I'm certainly interested in any more like
this you come up with.

> > If you're talking about other cases, if you could show exactly what's
> > triggering a warning, or failing to trigger it, I'll have a look.
> 
> The case is exactly the example program I've supplied: We get a warning
> about a global variable being created, while this variable is not even
> mentioned anywhere in the code.

That's fixed by my patch, as far as I can tell with no collateral
damage.  Please do report any further problems of this type so we can
discuss explicitly what's going on, which always makes things much
easier.

> About the suggestion in my original posting: What speaks against the
> idea of creating all "implicit variables" as globals implicitly, when
> the shell starts execution? That is, a program
> 
>     #!/bin/zsh
>     source something
>     foo bar
> 
> would be equivalent (that is, internally replaced) by
> 
>     #!/bin/zsh
>     MATCH=
>     source something
>     foo bar

Unless you're implying something further I haven't grapsed, that MATCH
variable is always and only cruft that's never actually providing a
useful value, and in some (as above) cases you might even want to guard
against it being global if it is created.  Also, it's not useful for any
purpose for anyone who isn't using WANRCREATEGLOBAL.  Tests to see if
the variable MATCH is set would become useless, as it always is set,
potentially breaking scripts.

(You won't know this, because it's new syntax, but we've just introduced
a new option WARN_NESTED_VAR that warns if you set a variable in an
enclosing scope, which you can turn on for specific functions if
needed.  Your trick doesn't fix that case, fixing the individual warning
does.  That option may not interest you anyway, as it's a bit more
invasive than WARN_CREATE_GLOBAL, but the problem is still there.)

I would hope there are better ways of fixing the problems related to the
warning itself, as I've done in this particular case, which is why I'd
like to see any further explicit examples in case there are any that
don't obviously fit one way or the other.

pws

Re: Bug in regexp operator when warn_create_global is in effect

[Ronald Fischer]

> The main point is to handle cases something like the following, where
> although the variable creation is hidden it's explicitly requested by the
> user:
> 
> fn() {
>    [[ $1 = (#m)${~2} ]]
> }
> 
> That (#m) in the pattern and the fact the pattern is in a function are
> the only relevant facts here.  The (#m) is a request to cause MATCH,
> MBEGIN and MEND to be set by the shell.  Because that's the purpose of
> (#m), you want the warning so as to be able to decide whether those
> variables are to be global or not and resolve between the two with a
> local or typeset -g.
> 
> Your case is different because the use of the variables isn't requested
> by special syntax, it's buried under standard regex syntax, so I've
> fixed it for that one case.  I'm certainly interested in any more like
> this you come up with.

Now I get it! Thanks for the explanation!

This (#m) example makes me think to the following problem (looks like an
opened can of worms): 

Clearly, a user who is aware to the global-vs-local topic will
conciously decide, whether he wants the M* variables local or global,
but no matter how he decides, it would be an insane decision if he
doesn't decide for the same scope on all three variables. For example,
it doesn't make sense to define MBEGIN as global and MATCH and MEND as
local. From the programmer's viewpoint, it would be more natural to
either decide on these three variables "as a group". Either all global
or all local. Currently, there is nothing which would enforce this
decision.

This now looks like a special case for (#m), but I wonder whether the
same wouldn't be true too for zsh modules, which also might create sets
of variables which belong together.

>From a programmer's viewpoint, a good concept (IMHO) would be that these
automatically created variables are always local, so if a function wants
to make a match globally visible, it would have to create its own global
variable for this. In the same way, it would perhaps make sense that
loop variables (in for-loop) are by default local, because this is the
usual way to use them. However, in both cases this might break existing
code, so it is perhaps not a real alternative. Seems that we have to
live with a compromise.

> That's fixed by my patch, as far as I can tell with no collateral
> damage. 

Thanks a lot, really. From which zsh version will this be available?

Ronald

Re: Bug in regexp operator when warn_create_global is in effect

[Peter Stephenson]

On Mon, 06 Feb 2017 16:48:10 +0100
Ronald Fischer <ynnor@mm.st> wrote:
> Now I get it! Thanks for the explanation!
> 
> This (#m) example makes me think to the following problem (looks like an
> opened can of worms): 
> 
> Clearly, a user who is aware to the global-vs-local topic will
> conciously decide, whether he wants the M* variables local or global,
> but no matter how he decides, it would be an insane decision if he
> doesn't decide for the same scope on all three variables. For example,
> it doesn't make sense to define MBEGIN as global and MATCH and MEND as
> local. From the programmer's viewpoint, it would be more natural to
> either decide on these three variables "as a group". Either all global
> or all local. Currently, there is nothing which would enforce this
> decision.

Yes, indeed.  We're right at the limit of what you can get from a single
option.  You could argue even the various cases of MATCH, REPLY,
etc. could have been a different option from the original
WARN_CREATE_GLOBAL --- it isn't basically because it still fits in with
what I originally thought the option was for, but I think your use is a
little different. Too late now.

The case of leaving one of the variables as a global and then having it
overridden in a function is what the new WARN_NESTED_VAR / functions -W
is about.  But that's arguably something of a sledgehammer:  that's why
we made it settable for only one function at a time as well as
glboally.  (That will also appear in 5.4 if you ever want to
investigate.)

> From a programmer's viewpoint, a good concept (IMHO) would be that these
> automatically created variables are always local, so if a function wants
> to make a match globally visible, it would have to create its own global
> variable for this. In the same way, it would perhaps make sense that
> loop variables (in for-loop) are by default local, because this is the
> usual way to use them. However, in both cases this might break existing
> code, so it is perhaps not a real alternative. Seems that we have to
> live with a compromise.

Yes, it's all rather a compromise.  Shell language is a bit of an
outlier in terms of modern scripting languages in many ways.

We actually recently rejected the notion of automatically making things
local as making scoping that bit too complicated, introducing too many
incompatibilities or possible hard to find glitches, etc., and I think
it's quite unlikely we'd go down that route as things stand.  (We
invented WARN_NESTED_VAR as a proxy.)  But that certainly doesn't mean
what we've got is perfect.

> > That's fixed by my patch, as far as I can tell with no collateral
> > damage. 
> 
> Thanks a lot, really. From which zsh version will this be available?

Will be in 5.4 --- but that's not currently imminent.  I'd like to
have quicker releases than we did last year, if possible, but it seems
the shell doesn't currently compile on at least one not-very-obscure
system.

pws