Defining function based on alias

Defining function based on alias

[Peter Stephenson]

It's pretty much always an error to define a function when the name of
the function is being expanded as an alias, right?  So we could make it
an error, possible with an option though this would need to be active by
default to make it useful.

% foo() { echo this is function foo; }
% alias foo='echo bar'
% foo() { echo this is not function foo; }
zsh: defining function based on alias `foo'
zsh: parse error near `()'

pws

diff --git a/Src/input.c b/Src/input.c
index eb968ea..f2c22c8 100644
--- a/Src/input.c
+++ b/Src/input.c
@@ -670,3 +670,22 @@ ingetptr(void)
 {
     return inbufptr;
 }
+
+/**/
+char *input_hasalias(void)
+{
+    int flags = inbufflags;
+    struct instacks *instackptr = instacktop;
+
+    for (;;)
+    {
+	if (!(flags & INP_CONT))
+	    break;
+	instackptr--;
+	if (instackptr->alias)
+	    return instackptr->alias->node.nam;
+	flags = instackptr->flags;
+    }
+
+    return NULL;
+}
diff --git a/Src/parse.c b/Src/parse.c
index 50a0d5f..1fed020 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -1738,6 +1738,7 @@ par_simple(int *cmplx, int nr)
 {
     int oecused = ecused, isnull = 1, r, argc = 0, p, isfunc = 0, sr = 0;
     int c = *cmplx, nrediradd, assignments = 0, ppost = 0, is_typeset = 0;
+    char *hasalias = input_hasalias();
     wordcode postassigns = 0;
 
     r = ecused;
@@ -1809,6 +1810,8 @@ par_simple(int *cmplx, int nr)
 	} else
 	    break;
 	zshlex();
+	if (!hasalias)
+	    hasalias = input_hasalias();
     }
     if (tok == AMPER || tok == AMPERBANG)
 	YYERROR(oecused);
@@ -1839,6 +1842,8 @@ par_simple(int *cmplx, int nr)
 			char *idstring = dupstrpfx(tokstr+1, eptr-tokstr-1);
 			redir_var = 1;
 			zshlex();
+			if (!hasalias)
+			    hasalias = input_hasalias();
 
 			if (IS_REDIROP(tok) && tokfd == -1)
 			{
@@ -1874,6 +1879,8 @@ par_simple(int *cmplx, int nr)
 		    argc++;
 		}
 		zshlex();
+		if (!hasalias)
+		    hasalias = input_hasalias();
 	    }
 	} else if (IS_REDIROP(tok)) {
 	    *cmplx = c = 1;
@@ -1902,6 +1909,8 @@ par_simple(int *cmplx, int nr)
 	    ecstr(name);
 	    ecstr(str);
 	    zshlex();
+	    if (!hasalias)
+		hasalias = input_hasalias();
 	} else if (tok == ENVARRAY) {
 	    int n, parr;
 
@@ -1936,6 +1945,10 @@ par_simple(int *cmplx, int nr)
 	    /* Error if preceding assignments */
 	    if (assignments || postassigns)
 		YYERROR(oecused);
+	    if (hasalias && argc) {
+		zwarn("defining function based on alias `%s'", hasalias);
+		YYERROR(oecused);
+	    }
 
 	    *cmplx = c;
 	    lineno = 0;

Re: Defining function based on alias

[Peter Stephenson]

On Sat, 7 Jan 2017 22:16:59 +0000
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> It's pretty much always an error to define a function when the name of
> the function is being expanded as an alias, right?  So we could make it
> an error, possible with an option though this would need to be active by
> default to make it useful.
> 
> % foo() { echo this is function foo; }
> % alias foo='echo bar'
> % foo() { echo this is not function foo; }
> zsh: defining function based on alias `foo'
> zsh: parse error near `()'

OK, let me rephrase this.

I am about to commit the following, which I hope will hope will stop
people coming to grief with this common confusion.  Please say if you
see any problems.

I have changed the code so that it doesn't complain if the alias
in affect at the start of the definition provided the "()" as well, since
there is no confusion in this case.

I have added an option to turn this off which is active in sh
compatibility mode since I would guess we can't get away with this
behaviour there.

diff --git a/Doc/Zsh/options.yo b/Doc/Zsh/options.yo
index f68a945..ba3ec17 100644
--- a/Doc/Zsh/options.yo
+++ b/Doc/Zsh/options.yo
@@ -1539,6 +1539,35 @@ enditem()
 
 subsect(Scripts and Functions)
 startitem()
+pindex(ALIAS_FUNC_DEF)
+pindex(NO_ALIAS_FUNC_DEF)
+pindex(ALIASFUNCDEF)
+pindex(NOALIASFUNCDEF)
+cindex(functions, defining with expanded aliases)
+cindex(aliases, expanding in function definition)
+item(tt(ALIAS_FUNC_DEF) <S>)(
+By default, zsh does not allow the definition of functions using
+the `var(name) tt(LPAR()RPAR())' syntax if var(name) was expanded as an
+alias: this causes an error.  This is usually the desired behaviour, as
+otherwise the combination of an alias and a function based on the same
+definition can easily cause problems.
+
+When this option is set, aliases can be used for defining functions.
+
+For example, consider the following definitions.
+
+example(alias foo=bar
+foo+LPAR()RPAR() {
+  print This probably does not do what you expect.
+})
+
+Here, tt(foo) is expanded as an alias to tt(bar) before the
+tt(LPAR()RPAR()) is encountered, so the function defined would be named
+tt(bar).  By default this is instead an error in native mode.  Note that
+quoting any part of the function name, or using the keyword
+tt(function), avoids the problem, so is recommended when the function
+name can also be an alias.
+)
 pindex(C_BASES)
 pindex(NO_C_BASES)
 pindex(CBASES)
diff --git a/Src/input.c b/Src/input.c
index eb968ea..f2c22c8 100644
--- a/Src/input.c
+++ b/Src/input.c
@@ -670,3 +670,22 @@ ingetptr(void)
 {
     return inbufptr;
 }
+
+/**/
+char *input_hasalias(void)
+{
+    int flags = inbufflags;
+    struct instacks *instackptr = instacktop;
+
+    for (;;)
+    {
+	if (!(flags & INP_CONT))
+	    break;
+	instackptr--;
+	if (instackptr->alias)
+	    return instackptr->alias->node.nam;
+	flags = instackptr->flags;
+    }
+
+    return NULL;
+}
diff --git a/Src/options.c b/Src/options.c
index 18619c8..4729ba5 100644
--- a/Src/options.c
+++ b/Src/options.c
@@ -78,6 +78,7 @@ mod_export HashTable optiontab;
  */
 static struct optname optns[] = {
 {{NULL, "aliases",	      OPT_EMULATE|OPT_ALL},	 ALIASESOPT},
+{{NULL, "aliasfuncdef",       OPT_EMULATE|OPT_BOURNE},	 ALIASFUNCDEF},
 {{NULL, "allexport",	      OPT_EMULATE},		 ALLEXPORT},
 {{NULL, "alwayslastprompt",   OPT_ALL},			 ALWAYSLASTPROMPT},
 {{NULL, "alwaystoend",	      0},			 ALWAYSTOEND},
diff --git a/Src/parse.c b/Src/parse.c
index 50a0d5f..ed6c4a8 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -1738,6 +1738,7 @@ par_simple(int *cmplx, int nr)
 {
     int oecused = ecused, isnull = 1, r, argc = 0, p, isfunc = 0, sr = 0;
     int c = *cmplx, nrediradd, assignments = 0, ppost = 0, is_typeset = 0;
+    char *hasalias = input_hasalias();
     wordcode postassigns = 0;
 
     r = ecused;
@@ -1809,6 +1810,8 @@ par_simple(int *cmplx, int nr)
 	} else
 	    break;
 	zshlex();
+	if (!hasalias)
+	    hasalias = input_hasalias();
     }
     if (tok == AMPER || tok == AMPERBANG)
 	YYERROR(oecused);
@@ -1839,6 +1842,8 @@ par_simple(int *cmplx, int nr)
 			char *idstring = dupstrpfx(tokstr+1, eptr-tokstr-1);
 			redir_var = 1;
 			zshlex();
+			if (!hasalias)
+			    hasalias = input_hasalias();
 
 			if (IS_REDIROP(tok) && tokfd == -1)
 			{
@@ -1874,6 +1879,8 @@ par_simple(int *cmplx, int nr)
 		    argc++;
 		}
 		zshlex();
+		if (!hasalias)
+		    hasalias = input_hasalias();
 	    }
 	} else if (IS_REDIROP(tok)) {
 	    *cmplx = c = 1;
@@ -1902,6 +1909,8 @@ par_simple(int *cmplx, int nr)
 	    ecstr(name);
 	    ecstr(str);
 	    zshlex();
+	    if (!hasalias)
+		hasalias = input_hasalias();
 	} else if (tok == ENVARRAY) {
 	    int n, parr;
 
@@ -1936,6 +1945,11 @@ par_simple(int *cmplx, int nr)
 	    /* Error if preceding assignments */
 	    if (assignments || postassigns)
 		YYERROR(oecused);
+	    if (hasalias && !isset(ALIASFUNCDEF) && argc &&
+		hasalias != input_hasalias()) {
+		zwarn("defining function based on alias `%s'", hasalias);
+		YYERROR(oecused);
+	    }
 
 	    *cmplx = c;
 	    lineno = 0;
diff --git a/Src/zsh.h b/Src/zsh.h
index f22d8b1..2a41638 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -2222,6 +2222,7 @@ struct histent {
 enum {
     OPT_INVALID,
     ALIASESOPT,
+    ALIASFUNCDEF,
     ALLEXPORT,
     ALWAYSLASTPROMPT,
     ALWAYSTOEND,
diff --git a/Test/A02alias.ztst b/Test/A02alias.ztst
index e52578e..e68e93e 100644
--- a/Test/A02alias.ztst
+++ b/Test/A02alias.ztst
@@ -82,6 +82,7 @@
 0:Global aliasing quotes
 > a string S 
 *>*5*echo S a string S "
+# "
 # Note there is a trailing space on the "> a string S " line
 
   (
@@ -115,3 +116,24 @@
 1:error message has the correct sign
 ?(eval):alias:1: bad option: +x
 ?(eval):alias:1: bad option: -z
+
+  # Usual issue that aliases aren't expanded until we
+  # trigger a new parse...
+  (alias badalias=notacommand
+  eval 'badalias() { print does not work; }')
+1:ALIAS_FUNC_DEF off by default.
+?(eval):1: defining function based on alias `badalias'
+?(eval):1: parse error near `()'
+
+  (alias goodalias=isafunc
+  setopt ALIAS_FUNC_DEF
+  eval 'goodalias() { print does now work; }'
+  isafunc)
+0:ALIAS_FUNC_DEF causes the icky behaviour to be avaliable
+>does now work
+
+  (alias thisisokthough='thisworks() { print That worked; }'
+  eval thisisokthough
+  thisworks)
+0:NO_ALIAS_FUNC_DEF works if the alias is a complete definition
+>That worked

Re: Defining function based on alias

[Bart Schaefer]

On Jan 8,  7:19pm, Peter Stephenson wrote:
} Subject: Re: Defining function based on alias
}
} I am about to commit the following, which I hope will hope will stop
} people coming to grief with this common confusion.  Please say if you
} see any problems.

No problems, but -- if it's possible to detect this situation, why not
simply suppress the alias expansion and define the function as named,
rather than trigger an error?

Etc/FAQ should be updated too.

I never remember how to rebuild FAQ from FAQ.yo ... can someone check
my yodl syntax?


diff --git a/Etc/FAQ.yo b/Etc/FAQ.yo
index 6b635ea..9eb23db 100644
--- a/Etc/FAQ.yo
+++ b/Etc/FAQ.yo
@@ -574,7 +574,8 @@ tt(EXTENDED_GLOB).
   it() Aliases and functions:
   itemize(
     it()  The order in which aliases and functions are defined is significant:
-        function definitions with () expand aliases -- see question \
+        in zsh versions 5.3.1 and earlier, function definitions with ()
+        expand aliases -- see question \
 link(2.3)(23).
     it()  Aliases and functions cannot be exported.
     it()  There are no tracked aliases: command hashing replaces these.
@@ -704,6 +705,12 @@ label(23)
   the manual). Note also that the mytt(;) at the end of the function is
   optional in zsh, but not in ksh or sh (for sh's where it exists).
 
+  Recent versions of zsh support anonymous functions with argument lists,
+  so you can create an alias that acts like a function.
+  verb(
+    alias cd='() { builtin cd "$@"; print -D $PWD; }'
+  )
+
   Here is Bart Schaefer's guide to converting csh aliases for zsh.
 
   enumerate(
@@ -711,6 +718,9 @@ label(23)
      then in zsh you need a function (referencing tt($1), tt($*) etc.).
      Otherwise, you can use a zsh alias.
 
+     In zsh version 5.0 and later you can use an anonymous zsh function in
+     combination with a zsh alias.
+
   myeit() If you use a zsh function, you need to refer _at_least_ to
      tt($*) in the body (inside the tt({ })).  Parameters don't magically
      appear inside the tt({ }) the way they get appended to an alias.
@@ -772,13 +782,17 @@ label(23)
     alias l='/bin/ls -F'
     l() { /bin/ls -la "$@" | more }
   )
-  mytt(l) in the function definition is in command position and is expanded
+  mytt(l) in the function definition is in command position and may expand
   as an alias, defining mytt(/bin/ls) and mytt(-F) as functions which call
   mytt(/bin/ls), which gets a bit recursive.  This can be avoided if you use
-  mytt(function) to define a function, which doesn't expand aliases.  It is
-  possible to argue for extra warnings somewhere in this mess.
+  mytt(function) to define a function, which doesn't expand aliases.
+
+  For zsh versions after 5.3.1, alias expansion of function names is an
+  error by default and the option tt(ALIAS_FUNC_DEF) must be active to
+  expand aliases in function names.  However, this option em(is) active
+  in compatibility modes where aliases are supported.
 
-  One workaround for this is to use the "function" keyword instead:
+  Here is an example using the "function" keyword instead:
   verb(
     alias l='/bin/ls -F'
     function l { /bin/ls -la "$@" | more }

Re: Defining function based on alias

[Peter Stephenson]

On Sun, 8 Jan 2017 12:14:54 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 8,  7:19pm, Peter Stephenson wrote:
> } Subject: Re: Defining function based on alias
> }
> } I am about to commit the following, which I hope will hope will stop
> } people coming to grief with this common confusion.  Please say if you
> } see any problems.
> 
> No problems, but -- if it's possible to detect this situation, why not
> simply suppress the alias expansion and define the function as named,
> rather than trigger an error?

It's too late; we'd have to wind back to where the alias was read and
read eveything in again, which is quite contorted, and not actually
guaranteed to fix things if it's a tortuous alias.  Possibly an even better
argument is that this silently changes the syntax, which is usually a
bad idea.

> I never remember how to rebuild FAQ from FAQ.yo ... can someone check
> my yodl syntax?

It seems OK but you just run "make" in Etc.

pws

Re: Defining function based on alias

[Bart Schaefer]

On Jan 8,  7:19pm, Peter Stephenson wrote:
}
} I am about to commit the following, which I hope will hope will stop
} people coming to grief with this common confusion.  Please say if you
} see any problems.

OK, here's an unusual case ...

    alias safely:foo='[[ -n $functions[foo] ]] || foo'

    safely:foo() { echo this is foo only if no other foo }

I hesitate to suggest further convolutions, but maybe an option to the
alias command to "locally" switch on ALIAS_FUNC_DEF if it's actually
*meant* to be used that way?

Probably not very important.

Re: Defining function based on alias

[Daniel Shahaf]

Peter Stephenson wrote on Sun, Jan 08, 2017 at 19:19:22 +0000:
> +++ b/Src/input.c
> @@ -670,3 +670,22 @@ ingetptr(void)
>  {
>      return inbufptr;
>  }
> +
> +/**/
> +char *input_hasalias(void)

May we please have docstrings for new code?  E.g.,

/*
 * If the input at the top of the stack(?) is an alias, return the name of
 * that alias, otherwise return NULL.
 */

Gave this a quick spin with various aliases and global aliases; behaves
as I'd expect.

My main concern is that .zshrc files that contain such a bug will not be
sourced all the way through.  Add a README entry?

Thanks,

Daniel

> +{
> +    int flags = inbufflags;
> +    struct instacks *instackptr = instacktop;
> +
> +    for (;;)
> +    {
> +	if (!(flags & INP_CONT))
> +	    break;
> +	instackptr--;
> +	if (instackptr->alias)
> +	    return instackptr->alias->node.nam;
> +	flags = instackptr->flags;
> +    }
> +
> +    return NULL;
> +}

Re: Defining function based on alias

[Daniel Shahaf]

Bart Schaefer wrote on Sun, Jan 08, 2017 at 16:09:21 -0800:
> On Jan 8,  7:19pm, Peter Stephenson wrote:
> }
> } I am about to commit the following, which I hope will hope will stop
> } people coming to grief with this common confusion.  Please say if you
> } see any problems.
> 
> OK, here's an unusual case ...
> 
>     alias safely:foo='[[ -n $functions[foo] ]] || foo'
> 
>     safely:foo() { echo this is foo only if no other foo }
> 
> I hesitate to suggest further convolutions, but maybe an option to the
> alias command to "locally" switch on ALIAS_FUNC_DEF if it's actually
> *meant* to be used that way?

This case doesn't require a new option:

    alias safely:foo='[[ -n $functions[foo] ]] || function foo'

> Probably not very important.
> 

Another case:

% alias foo=''
% foo() echo hey  
hey

Or less minimally:

% setopt NO_multifuncdef 
% alias foo='' 
% foo bar () :        
% which -a foo bar  
foo: aliased to 
bar () {
        :
}

Should these two behave differently when the new option is set?  The
first case appeared to be a non-anonymous function definition and the
second a multifuncdef, but the alias changed the semantics.

As Bart said: probably not very important.

Re: Defining function based on alias

[Peter Stephenson]

On Mon, 9 Jan 2017 04:02:58 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Should these two behave differently when the new option is set?  The
> first case appeared to be a non-anonymous function definition and the
> second a multifuncdef, but the alias changed the semantics.

I wasn't proposing to try to protect everyone from every possible error,
just the case we know is quite common even in normal use of functions
and aliases.

Conversely, if you need to do clever tricks I'm assuming you will simply
sign on the dotted line to say you will take responsibility and set the
option.

I do need to add something to the README.

pws