PATCH: functions with redirections

PATCH: functions with redirections

[Peter Stephenson]

This seems to add the ability to define functions with redirections that
take effect when the function is run.  Parsing didn't actually need
changing at all, despite what I said a few days ago --- we already parse
the redirections, it's just a question of saving them at the definition
and digging them back out when executing.

I haven't yet looked at dump files.  That's partly becuase I don't
understand them and partly because I don't use them.  Any hints would be
great.

If anyone wants something else to do they can think about whether I've
introduced any subtle memory leaks.

diff --git a/NEWS b/NEWS
index 7dc134e..bf8969b 100644
--- a/NEWS
+++ b/NEWS
@@ -107,6 +107,13 @@ Changes since 5.0.0
   Also, the value of $pipestatus is now updated when a job stops, not just
   when it exits.
 
+- Redirections applied to function definitions take effect when the
+  function is executed, not when it is defined.  Other shells already
+  work this way.  For example,
+    fn() { echo hello } >~/logfile
+  Running fn writes "hello" to logfile.  In older versions of the shell
+  it would create an empty file at the point of definition.
+
 Changes between 4.2 and 5.0.0
 -----------------------------
 
diff --git a/Src/exec.c b/Src/exec.c
index fb2739a..1c2a904 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -198,7 +198,8 @@ static char *blank_env[] = { NULL };
 /* Execution functions. */
 
 static int (*execfuncs[WC_COUNT-WC_CURSH]) _((Estate, int)) = {
-    execcursh, exectime, execfuncdef, execfor, execselect,
+    execcursh, exectime, NULL /* execfuncdef handled specially */,
+    execfor, execselect,
     execwhile, execrepeat, execcase, execif, execcond,
     execarith, execautofn, exectry
 };
@@ -1116,8 +1117,11 @@ execsimple(Estate state)
 	    fflush(xtrerr);
 	}
 	lv = (errflag ? errflag : cmdoutval);
-    } else
+    } else if (code == WC_FUNCDEF) {
+	lv = execfuncdef(state, NULL);
+    } else {
 	lv = (execfuncs[code - WC_CURSH])(state, 0);
+    }
 
     thisjob = otj;
 
@@ -2790,6 +2794,42 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	return;
     }
 
+    if (type == WC_FUNCDEF) {
+	/*
+	 * The first word of a function definition is a list of
+	 * names.  If this is empty, we're doing an anonymous function:
+	 * in that case redirections are handled normally.
+	 * If not, it's a function definition: then we don't do
+	 * redirections here but pass in the list of redirections to
+	 * be stored for recall with the function.
+	 */
+	if (*state->pc != 0) {
+	    /* Nonymous, don't do redirections here */
+	    redir = NULL;
+	}
+    } else if (is_shfunc) {
+	Shfunc shf = (Shfunc)hn;
+	/*
+	 * A function definition may have a list of additional
+	 * redirections to apply, so retrieve it.
+	 */
+	if (shf->redir) {
+	    struct estate s;
+	    LinkList redir2;
+
+	    s.prog = shf->redir;
+	    s.pc = shf->redir->prog;
+	    s.strs = shf->redir->strs;
+	    redir2 = ecgetredirs(&s);
+	    if (!redir)
+		redir = redir2;
+	    else {
+		while (nonempty(redir2))
+		    addlinknode(redir, ugetnode(redir2));
+	    }
+	}
+    }
+
     if (type == WC_SIMPLE && !nullexec) {
 	char *s;
 	char trycd = (isset(AUTOCD) && isset(SHINSTDIN) &&
@@ -3238,7 +3278,33 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		flags |= ESUB_REVERTPGRP;
 	    entersubsh(flags);
 	}
-	if (type >= WC_CURSH) {
+	if (type == WC_FUNCDEF) {
+	    Eprog redir_prog;
+	    if (!redir && wc_code(*beg) == WC_REDIR)  {
+		/*
+		 * We're not using a redirection from the currently
+		 * parsed environment, which is what we'd do for an
+		 * anonymous function, but there are redirections we
+		 * should store with the new function.
+		 */
+		struct estate s;
+
+		s.prog = state->prog;
+		s.pc = beg;
+		s.strs = state->prog->strs;
+
+		/*
+		 * The copy uses the wordcode parsing area, so save and
+		 * restore state.
+		 */
+		lexsave();
+		redir_prog = eccopyredirs(&s);
+		lexrestore();
+	    } else
+		redir_prog = NULL;
+	    
+	    lastval = execfuncdef(state, redir_prog);
+	} else if (type >= WC_CURSH) {
 	    if (last1 == 1)
 		do_exec = 1;
 	    lastval = (execfuncs[type - WC_CURSH])(state, do_exec);
@@ -4235,7 +4301,7 @@ exectime(Estate state, UNUSED(int do_exec))
 
 /**/
 static int
-execfuncdef(Estate state, UNUSED(int do_exec))
+execfuncdef(Estate state, Eprog redir_prog)
 {
     Shfunc shf;
     char *s = NULL;
@@ -4305,6 +4371,7 @@ execfuncdef(Estate state, UNUSED(int do_exec))
 	shf->node.flags = 0;
 	shf->filename = ztrdup(scriptfilename);
 	shf->lineno = lineno;
+	shf->redir = redir_prog;
 	shfunc_set_sticky(shf);
 
 	if (!names) {
@@ -4335,6 +4402,8 @@ execfuncdef(Estate state, UNUSED(int do_exec))
 	    ret = lastval;
 
 	    freeeprog(shf->funcdef);
+	    if (shf->redir) /* shouldn't be */
+		freeeprog(shf->redir);
 	    zsfree(shf->filename);
 	    zfree(shf, sizeof(*shf));
 	    break;
diff --git a/Src/hashtable.c b/Src/hashtable.c
index ef18792..7a43062 100644
--- a/Src/hashtable.c
+++ b/Src/hashtable.c
@@ -887,6 +887,8 @@ freeshfuncnode(HashNode hn)
     zsfree(shf->node.nam);
     if (shf->funcdef)
 	freeeprog(shf->funcdef);
+    if (shf->redir)
+	freeeprog(shf->redir);
     zsfree(shf->filename);
     if (shf->sticky) {
 	if (shf->sticky->n_on_opts)
@@ -954,10 +956,19 @@ printshfuncnode(HashNode hn, int printflags)
 		printf(" \"$@\"");
 	    }
 	}   
-	printf("\n}\n");
+	printf("\n}");
     } else {
-	printf(" () { }\n");
+	printf(" () { }");
     }
+    if (f->redir) {
+	t = getpermtext(f->redir, NULL, 1);
+	if (t) {
+	    zputs(t, stdout);
+	    zsfree(t);
+	}
+    }
+
+    putchar('\n');
 }
 
 /**************************************/
diff --git a/Src/parse.c b/Src/parse.c
index 3633417..6cba050 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -375,6 +375,8 @@ init_parse(void)
 
 /* Build eprog. */
 
+/* careful: copy_ecstr is from arg1 to arg2, unlike memcpy */
+
 static void
 copy_ecstr(Eccstr s, char *p)
 {
@@ -386,24 +388,25 @@ copy_ecstr(Eccstr s, char *p)
 }
 
 static Eprog
-bld_eprog(void)
+bld_eprog(int heap)
 {
     Eprog ret;
     int l;
 
     ecadd(WCB_END());
 
-    ret = (Eprog) zhalloc(sizeof(*ret));
+    ret = heap ? (Eprog) zhalloc(sizeof(*ret)) : (Eprog) zalloc(sizeof(*ret));
     ret->len = ((ecnpats * sizeof(Patprog)) +
 		(ecused * sizeof(wordcode)) +
 		ecsoffs);
     ret->npats = ecnpats;
-    ret->nref = -1;		/* Eprog is on the heap */
-    ret->pats = (Patprog *) zhalloc(ret->len);
+    ret->nref = heap ? -1 : 1;
+    ret->pats = heap ? (Patprog *) zhalloc(ret->len) :
+	(Patprog *) zshcalloc(ret->len);
     ret->prog = (Wordcode) (ret->pats + ecnpats);
     ret->strs = (char *) (ret->prog + ecused);
     ret->shf = NULL;
-    ret->flags = EF_HEAP;
+    ret->flags = heap ? EF_HEAP : EF_REAL;
     ret->dump = NULL;
     for (l = 0; l < ecnpats; l++)
 	ret->pats[l] = dummy_patprog1;
@@ -455,7 +458,7 @@ parse_event(void)
         clear_hdocs();
         return NULL;
     }
-    return bld_eprog();
+    return bld_eprog(1);
 }
 
 /**/
@@ -534,7 +537,7 @@ parse_list(void)
 	yyerror(0);
 	return NULL;
     }
-    return bld_eprog();
+    return bld_eprog(1);
 }
 
 /*
@@ -553,7 +556,7 @@ parse_cond(void)
         clear_hdocs();
 	return NULL;
     }
-    return bld_eprog();
+    return bld_eprog(1);
 }
 
 /* This adds a list wordcode. The important bit about this is that it also
@@ -2554,6 +2557,73 @@ ecgetredirs(Estate s)
     return ret;
 }
 
+/*
+ * Copy the consecutive set of redirections in the state at s.
+ * Return NULL if none, else an Eprog consisting only of the
+ * redirections from permanently allocated memory.
+ *
+ * s is left in the state ready for whatever follows the redirections.
+ */
+
+/**/
+Eprog
+eccopyredirs(Estate s)
+{
+    Wordcode pc = s->pc;
+    wordcode code = *pc;
+    int ncode, ncodes = 0, r, type;
+
+    if (wc_code(code) != WC_REDIR)
+	return NULL;
+
+    init_parse();
+
+    while (wc_code(code) == WC_REDIR) {
+	type = WC_REDIR_TYPE(code);
+
+	DPUTS(type == REDIR_HEREDOC || type == REDIR_HEREDOCDASH,
+	      "unexpanded here document");
+
+	if (WC_REDIR_FROM_HEREDOC(code))
+	    ncode = 5;
+	else
+	    ncode = 3;
+	if (WC_REDIR_VARID(code))
+	    ncode++;
+	pc += ncode;
+	ncodes += ncode;
+	code = *pc;
+    }
+    r = ecused;
+    ecispace(r, ncodes);
+
+    code = *s->pc;
+    while (wc_code(code) == WC_REDIR) {
+	s->pc++;
+
+	ecbuf[r++] = code;
+	/* fd1 */
+	ecbuf[r++] = *s->pc++;
+	/* name or HERE string */
+	/* No DUP needed as we'll copy into Eprog immediately below */
+	ecbuf[r++] = ecstrcode(ecgetstr(s, EC_NODUP, NULL));
+	if (WC_REDIR_FROM_HEREDOC(code))
+	{
+	    /* terminator, raw */
+	    ecbuf[r++] = ecstrcode(ecgetstr(s, EC_NODUP, NULL));
+	    /* terminator, munged */
+	    ecbuf[r++] = ecstrcode(ecgetstr(s, EC_NODUP, NULL));
+	}
+	if (WC_REDIR_VARID(code))
+	    ecbuf[r++] = ecstrcode(ecgetstr(s, EC_NODUP, NULL));
+
+	code = *s->pc;
+    }
+
+    /* bld_eprog() appends a useful WC_END marker */
+    return bld_eprog(0);
+}
+
 /**/
 mod_export struct eprog dummy_eprog;
 
@@ -3546,4 +3616,3 @@ dump_autoload(char *nam, char *file, int on, Options ops, int func)
     }
     return ret;
 }
-
diff --git a/Src/signals.c b/Src/signals.c
index cb2b581..74a39e5 100644
--- a/Src/signals.c
+++ b/Src/signals.c
@@ -752,7 +752,7 @@ dosavetrap(int sig, int level)
 	Shfunc shf, newshf = NULL;
 	if ((shf = (Shfunc)gettrapnode(sig, 1))) {
 	    /* Copy the node for saving */
-	    newshf = (Shfunc) zalloc(sizeof(*newshf));
+	    newshf = (Shfunc) zshcalloc(sizeof(*newshf));
 	    newshf->node.nam = ztrdup(shf->node.nam);
 	    newshf->node.flags = shf->node.flags;
 	    newshf->funcdef = dupeprog(shf->funcdef, 0);
diff --git a/Src/zsh.h b/Src/zsh.h
index 8f56fa2..d284c7a 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1139,6 +1139,7 @@ struct shfunc {
     char *filename;             /* Name of file located in */
     zlong lineno;		/* line number in above file */
     Eprog funcdef;		/* function definition    */
+    Eprog redir;                /* redirections to apply */
     Emulation_options sticky;   /* sticky emulation definitions, if any */
 };
 
diff --git a/Test/A04redirect.ztst b/Test/A04redirect.ztst
index 7ad02db..436ae59 100644
--- a/Test/A04redirect.ztst
+++ b/Test/A04redirect.ztst
@@ -54,7 +54,7 @@
 0:Here-documents stripping tabs
 >barbar
 
-  cat <<-$'$HERE '`$(THERE) `'$((AND)) '"\EVERYWHERE"
+  cat <<-$'$HERE '`$(THERE) `'$((AND)) '"\EVERYWHERE" #
 # tabs again.  sorry about the max miller.
 	Here's a funny thing.  Here is a funny thing.
 	I went home last night.  There's a funny thing.
@@ -455,3 +455,39 @@
 
   [</dev/null ]
 1:check behaviour with square brackets
+
+  print any old rubbish >input1
+  () {
+    local var
+    read var
+    print I just read $var
+  } <input1 >output1
+  print Nothing output yet
+  cat output1
+0:anonymous function redirections are applied immediately
+>Nothing output yet
+>I just read any old rubbish
+
+  redirfn() {
+    local var
+    read var
+    print I want to tell you about $var
+    print Also, this might be an error >&2
+  } <input2 >output2 2>&1
+  print something I heard on the radio >input2
+  redirfn
+  print No output until after this
+  cat output2
+0:redirections with normal function definition
+>No output until after this
+>I want to tell you about something I heard on the radio
+>Also, this might be an error
+
+  which redirfn
+0:text output of function with redirections
+>redirfn () {
+>	local var
+>	read var
+>	print I want to tell you about $var
+>	print Also, this might be an error >&2
+>} < input2 > output2 2>&1

pws

Re: PATCH: functions with redirections

[Peter Stephenson]

One thing I missed is multiply named functions.

diff --git a/Src/exec.c b/Src/exec.c
index 1c2a904..cedadc8 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -4306,6 +4306,7 @@ execfuncdef(Estate state, Eprog redir_prog)
     Shfunc shf;
     char *s = NULL;
     int signum, nprg, sbeg, nstrs, npats, len, plen, i, htok = 0, ret = 0;
+    int nfunc = 0;
     Wordcode beg = state->pc, end;
     Eprog prog;
     Patprog *pp;
@@ -4330,6 +4331,8 @@ execfuncdef(Estate state, Eprog redir_prog)
 	}
     }
 
+    DPUTS(!names && redir_prog,
+	  "Passing redirection to anon function definition.");
     while (!names || (s = (char *) ugetnode(names))) {
 	if (!names) {
 	    prog = (Eprog) zhalloc(sizeof(*prog));
@@ -4371,7 +4374,15 @@ execfuncdef(Estate state, Eprog redir_prog)
 	shf->node.flags = 0;
 	shf->filename = ztrdup(scriptfilename);
 	shf->lineno = lineno;
-	shf->redir = redir_prog;
+	/*
+	 * redir_prog is permanently allocated --- but if
+	 * this function has multiple names we need an additional
+	 * one.
+	 */
+	if (nfunc++ && redir_prog)
+	    shf->redir = dupeprog(redir_prog, 0);
+	else
+	    shf->redir = redir_prog;
 	shfunc_set_sticky(shf);
 
 	if (!names) {
@@ -4427,6 +4438,10 @@ execfuncdef(Estate state, Eprog redir_prog)
 	    shfunctab->addnode(shfunctab, ztrdup(s), shf);
 	}
     }
+    if (!nfunc && redir_prog) {
+	/* For completeness, shouldn't happen */
+	freeeprog(redir_prog);
+    }
     state->pc = end;
     return ret;
 }
diff --git a/Test/A04redirect.ztst b/Test/A04redirect.ztst
index 436ae59..6c38a31 100644
--- a/Test/A04redirect.ztst
+++ b/Test/A04redirect.ztst
@@ -491,3 +491,19 @@
 >	print I want to tell you about $var
 >	print Also, this might be an error >&2
 >} < input2 > output2 2>&1
+
+  1func 2func 3func() { print Ich heisse $0 } >output3
+  for i in 1 2 3; do
+    f=${i}func
+    print Running $f
+    $f
+    cat output3
+    unfunction $f
+  done
+0:multiply named functions with redirection
+>Running 1func
+>Ich heisse 1func
+>Running 2func
+>Ich heisse 2func
+>Running 3func
+>Ich heisse 3func

pws

Re: PATCH: functions with redirections

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]

On Sep 29, 2014 12:52 PM, "Peter Stephenson" <p.w.stephenson@ntlworld.com>
wrote:
>
> This seems to add the ability to define functions with redirections that
> take effect when the function is run.  Parsing didn't actually need
> changing at all, despite what I said a few days ago --- we already parse
> the redirections, it's just a question of saving them at the definition
> and digging them back out when executing.

Pardon me for asking when I haven't had a chance to try it yet, but does
this work right if redirection of the same FD appears both at the time of
def'n and at the time of call?

E.g. in bash:

foo() { echo foo; } >&3
foo 3>&2

first dups 3 from 2 and then dups 1 from 3, so "foo" is echo'd to stderr.

> I haven't yet looked at dump files.  That's partly becuase I don't
> understand them and partly because I don't use them.  Any hints would be
> great.

By dump files do you mean zcompile?
I fear I'm not of much help there.

> If anyone wants something else to do they can think about whether I've
> introduced any subtle memory leaks.

I wouldn't expect any that would fool valgrind on a few simple examples.

Re: PATCH: functions with redirections

[Bart Schaefer]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

On Sep 29, 2014 1:53 PM, "Bart Schaefer" <schaefer@brasslantern.com> wrote:
>
>
> On Sep 29, 2014 12:52 PM, "Peter Stephenson" <p.w.stephenson@ntlworld.com>
wrote:
> >
> > I haven't yet looked at dump files.  That's partly becuase I don't
> > understand them and partly because I don't use them.  Any hints would be
> > great.
>
> By dump files do you mean zcompile?
> I fear I'm not of much help there.

A thought:

My first idea when this came up was to alter the wordcode for functions
with redirections to explicitly add the implicit set of braces, e.g.,

foo () { echo foo } >&3

becomes

foo () { { echo foo } >&3 }

Then it would not be necessary to mess with printing/dumping etc. of the
definition, or invent a new place to store the redirections.  Yeah, output
of "functions" et al. becomes a little less faithful but so what?

Sadly I never managed to make that work, but maybe the concept is helpful
for dump file creation.

Re: PATCH: functions with redirections

[Bart Schaefer]

On Sep 29,  1:53pm, Bart Schaefer wrote:
}
} foo() { echo foo; } >&3
} foo 3>&2

Tried this now; happily it works the same as bash.

} > If anyone wants something else to do they can think about whether I've
} > introduced any subtle memory leaks.
} 
} I wouldn't expect any that would fool valgrind on a few simple examples.

I tried a function with an output descriptor redirection and an input
here-document and valgrind found no leaks.

Tangential remark:  function-definition  redirections to files may
produce unexpected errors when noclobber is set.

Re: PATCH: functions with redirections

[Mikael Magnusson]

On 29 September 2014 23:37, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Sep 29, 2014 1:53 PM, "Bart Schaefer" <schaefer@brasslantern.com> wrote:
>>
>>
>> On Sep 29, 2014 12:52 PM, "Peter Stephenson" <p.w.stephenson@ntlworld.com>
> wrote:
>> >
>> > I haven't yet looked at dump files.  That's partly becuase I don't
>> > understand them and partly because I don't use them.  Any hints would be
>> > great.
>>
>> By dump files do you mean zcompile?
>> I fear I'm not of much help there.
>
> A thought:
>
> My first idea when this came up was to alter the wordcode for functions
> with redirections to explicitly add the implicit set of braces, e.g.,
>
> foo () { echo foo } >&3
>
> becomes
>
> foo () { { echo foo } >&3 }
>
> Then it would not be necessary to mess with printing/dumping etc. of the
> definition, or invent a new place to store the redirections.  Yeah, output
> of "functions" et al. becomes a little less faithful but so what?
>
> Sadly I never managed to make that work, but maybe the concept is helpful
> for dump file creation.

I noticed that the contents of $functions[foo] in the former case
completely omits the redirection as it is currently. An argument for
turning the syntax into the latter is that you could claim it wasn't a
function definition of the form foo() { ... } in the first place, but
a foo() ... definition, where ... happens to be { xyz } > &3 which is
also a valid single statement command, printing the function in a
normalized form would then tack on the outer braces as usual. I don't
know if this would hold up in court though.

-- 
Mikael Magnusson

Re: PATCH: functions with redirections

[Peter Stephenson]

On Mon, 29 Sep 2014 18:02:57 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Sep 29,  1:53pm, Bart Schaefer wrote:
> }
> } foo() { echo foo; } >&3
> } foo 3>&2
> 
> Tried this now; happily it works the same as bash.

It works by appending the list from the function definition to the list
from the command line, so the effect is "3>&2 >&3" as you expect.
Should probably add a test for it --- it's slightly different code from
the normal case.

$functions[foo] output will need a special case.  I'll look at it.  I
don't think there's anything much to be gained by extending the special
case elsewhere since it's already broadly working like other shells, and
$functions is an odd one out there.

pws

Re: PATCH: functions with redirections

[Peter Stephenson]

On Tue, 30 Sep 2014 09:48:13 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> On Mon, 29 Sep 2014 18:02:57 -0700
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> > On Sep 29,  1:53pm, Bart Schaefer wrote:
> > }
> > } foo() { echo foo; } >&3
> > } foo 3>&2
> > 
> > Tried this now; happily it works the same as bash.
> 
> It works by appending the list from the function definition to the list
> from the command line, so the effect is "3>&2 >&3" as you expect.
> Should probably add a test for it --- it's slightly different code from
> the normal case.

There's one here.

> $functions[foo] output will need a special case.  I'll look at it.

This is straightforward, except the code is messily duplicated in two
places and both have an unmetafy() I'm extremely dubious about ---
surely as the string is internal to a parameter value it should still be
metafied?  If something's not unmetafying it that's a bug elsewhere.

I'm also a bit dubious about the way output for functions with the EF_RUN
flag is handled --- the original code rather than the additions.  I
think this is to do with ksh-style autoloads.

diff --git a/Src/Modules/parameter.c b/Src/Modules/parameter.c
index 0385a70..55157a9 100644
--- a/Src/Modules/parameter.c
+++ b/Src/Modules/parameter.c
@@ -392,20 +392,36 @@ getfunction(UNUSED(HashTable ht), const char *name, int dis)
 				((shf->node.flags & PM_TAGGED) ? "t" : "")));
 	} else {
 	    char *t = getpermtext(shf->funcdef, NULL, 1), *n, *h;
+	    char *start;
+
+	    if (shf->redir)
+		start = "{\n\t";
+	    else
+		start = "\t";
 
 	    if (shf->funcdef->flags & EF_RUN) {
 		n = nicedupstring(name);
-		h = (char *) zhalloc(strlen(t) + strlen(n) + 9);
-		h[0] = '\t';
-		strcpy(h + 1, t);
+		h = (char *) zhalloc(strlen(start) + strlen(t) + strlen(n) + 8);
+		strcpy(h, start);
+		strcat(h, t);
 		strcat(h, "\n\t");
 		strcat(h, n);
 		strcat(h, " \"$@\"");
 	    } else
-		h = dyncat("\t", t);
+		h = dyncat(start, t);
 	    zsfree(t);
+	    /*
+	     * TBD: Is this unmetafy correct?  Surely as this
+	     * is a parameter value it stays metafied?
+	     */
 	    unmetafy(h, NULL);
 
+	    if (shf->redir) {
+		t = getpermtext(shf->redir, NULL, 1);
+		h = zhtricat(h, "\n}", t);
+		zsfree(t);
+	    }
+
 	    pm->u.str = h;
 	}
     } else {
@@ -456,21 +472,38 @@ scanfunctions(UNUSED(HashTable ht), ScanFunc func, int flags, int dis)
 				    ((shf->node.flags & PM_TAGGED) ? "Ut" : "U") :
 				    ((shf->node.flags & PM_TAGGED) ? "t" : "")));
 		    } else {
-			char *t = getpermtext(((Shfunc) hn)->funcdef, NULL, 1);
-			char *n;
+			Shfunc shf = (Shfunc)hn;
+			char *t = getpermtext(shf->funcdef, NULL, 1);
+			char *n, *start;
 
-			if (((Shfunc) hn)->funcdef->flags & EF_RUN) {
+			if (shf->redir)
+			    start = "{\n\t";
+			else
+			    start = "\t";
+
+			if (shf->funcdef->flags & EF_RUN) {
 			    n = nicedupstring(hn->nam);
-			    pm.u.str = (char *) zhalloc(strlen(t) + strlen(n) + 9);
-			    pm.u.str[0] = '\t';
-			    strcpy(pm.u.str + 1, t);
+			    pm.u.str = (char *) zhalloc(
+				strlen(start) + strlen(t) + strlen(n) + 8);
+			    strcpy(pm.u.str, start);
+			    strcat(pm.u.str, t);
 			    strcat(pm.u.str, "\n\t");
 			    strcat(pm.u.str, n);
 			    strcat(pm.u.str, " \"$@\"");
 			} else
-			    pm.u.str = dyncat("\t", t);
+			    pm.u.str = dyncat(start, t);
+			/*
+			 * TBD: Is this unmetafy correct?  Surely as this
+			 * is a parameter value it stays metafied?
+			 */
 			unmetafy(pm.u.str, NULL);
 			zsfree(t);
+
+			if (shf->redir) {
+			    t = getpermtext(shf->redir, NULL, 1);
+			    pm.u.str = zhtricat(pm.u.str, "\n}", t);
+			    zsfree(t);
+			}
 		    }
 		}
 		func(&pm.node, flags);
diff --git a/Test/A04redirect.ztst b/Test/A04redirect.ztst
index 6c38a31..a39ce46 100644
--- a/Test/A04redirect.ztst
+++ b/Test/A04redirect.ztst
@@ -507,3 +507,27 @@
 >Ich heisse 2func
 >Running 3func
 >Ich heisse 3func
+
+  redirfn2() { print The latest output; } >&3
+  redirfn2 3>output4
+  print No output yet
+  cat output4
+0:Redirections in both function definition and command line
+>No output yet
+>The latest output
+
+# This relies on the fact that the test harness always loads
+# the zsh/parameter module.
+  print $functions[redirfn]
+0:Output from $functions[] for definition with redirection
+>{
+>	local var
+>	read var
+>	print I want to tell you about $var
+>	print Also, this might be an error >&2
+>} < input2 > output2 2>&1
+
+  noredirfn() { print This rather boring function has no redirection.; }
+  print $functions[noredirfn]
+0:Output from $functions[] for definition with no redirection
+>	print This rather boring function has no redirection.


pws

Re: PATCH: functions with redirections

[Peter Stephenson]

On Mon, 29 Sep 2014 14:37:35 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> My first idea when this came up was to alter the wordcode for functions
> with redirections to explicitly add the implicit set of braces, e.g.,
> 
> foo () { echo foo } >&3
> 
> becomes
> 
> foo () { { echo foo } >&3 }
> 
> Then it would not be necessary to mess with printing/dumping etc. of the
> definition, or invent a new place to store the redirections.  Yeah, output
> of "functions" et al. becomes a little less faithful but so what?
> 
> Sadly I never managed to make that work, but maybe the concept is helpful
> for dump file creation.

This appears to be about the only way I could even conceivably get this
to work --- otherwise it means reengineering multiple different ways of
constructing and reading a dump file --- but even this appears too
difficult.  The main sticking point is where you need to add the wrapper
you have an Eprog, and it's hard to convert that back into the form you
need for adding additional components or glueing bits together ---
Eprogs are already glued together seamlessly.  I fixed it for the redir
on its own by writing a function to extract the simple redir wordcode
directly, but that's not feasible here.  It's not fatal that it doesn't
work, you just lose the redirections.

So I'll simply document around this and be satisfied with the fact that
it is possible to compile an autoload file like

foo() { echo foo } >&3

with or without the zsh kludge 'foo "$@"' stuck at the end.  I suspect
compiling existing autoload files is much more common than compiling
functions typed into the shell.

I've also documented the basic feature and verified for the first time
that

foo() echo foo >&3

also works as expected.  (As expected if you understand weird squiggles,
that is.)

diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index 9862c63..41c189f 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -2174,6 +2174,20 @@ match one of these patterns will be written. If no var(name) is given,
 the definitions of all functions currently defined or marked as
 autoloaded will be written.
 
+Note the second form cannot be used for compiling functions that
+include redirections as part of the definition rather than within
+the body of the function; for example
+
+example(fn1() { { ... } >~/logfile })
+
+can be compiled but
+
+example(fn1() { ... } >~/logfile)
+
+cannot.  It is possible to use the first form of tt(zcompile) to compile
+autoloadable functions that include the full function definition instead
+of just the body of the function.
+
 The third form, with the tt(-t) option, examines an existing
 compiled file.  Without further arguments, the names of the original
 files compiled into it are listed.  The first line of output shows
diff --git a/Doc/Zsh/grammar.yo b/Doc/Zsh/grammar.yo
index 77f0098..eb1edf7 100644
--- a/Doc/Zsh/grammar.yo
+++ b/Doc/Zsh/grammar.yo
@@ -352,6 +352,15 @@ If the option tt(SH_GLOB) is set for compatibility with other shells, then
 whitespace may appear between between the left and right parentheses when
 there is a single var(word);  otherwise, the parentheses will be treated as
 forming a globbing pattern in that case.
+
+In any of the forms above, a redirection may appear outside the
+function body, for example
+
+example(func() { ... } 2>&1)
+
+The redirection is stored with the function and applied whenever the
+function is executed.  Any variables in the redirection are expanded
+at the point the function is executed, but outside the function scope.
 )
 cindex(timing)
 findex(time)

-- 
Peter Stephenson <p.w.stephenson@ntlworld.com>
Web page now at http://homepage.ntlworld.com/p.w.stephenson/

Re: PATCH: functions with redirections

[Bart Schaefer]

On Oct 1,  8:17pm, Peter Stephenson wrote:
} Subject: Re: PATCH: functions with redirections
}
} On Mon, 29 Sep 2014 14:37:35 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > 
} > foo () { echo foo } >&3
} > 
} > becomes
} > 
} > foo () { { echo foo } >&3 }
} 
} This appears to be about the only way I could even conceivably get this
} to work --- otherwise it means reengineering multiple different ways of
} constructing and reading a dump file --- but even this appears too
} difficult.

It's also apparently wrong, if this ...

} +function is executed.  Any variables in the redirection are expanded
} +at the point the function is executed, but outside the function scope.

... is actually the way it's supposed to work.

} The main sticking point is where you need to add the wrapper
} you have an Eprog, and it's hard to convert that back into the form you
} need for adding additional components or glueing bits together ---

Yes.  This was what I ran into as well.

} So I'll simply document around this and be satisfied with the fact that
} it is possible to compile an autoload file like
} 
} foo() { echo foo } >&3
} 
} with or without the zsh kludge 'foo "$@"' stuck at the end.  I suspect
} compiling existing autoload files is much more common than compiling
} functions typed into the shell.

Hmm.

torch% cat /tmp/foo
foo() { echo foo } >&3
torch% zcompile -k /tmp/foo
torch% autoload foo
torch% FPATH=/tmp foo      
foo

Oops, the redirection didn't get applied.  But it does once the function
is loaded and then called:

torch% functions foo
foo () {
        echo foo
} >&3
torch% foo
zsh: 3: bad file descriptor
torch% 

Re: PATCH: functions with redirections

[Peter Stephenson]

On Wed, 01 Oct 2014 21:07:41 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Oct 1,  8:17pm, Peter Stephenson wrote:
> } Subject: Re: PATCH: functions with redirections
> }
> } On Mon, 29 Sep 2014 14:37:35 -0700
> } Bart Schaefer <schaefer@brasslantern.com> wrote:
> } > 
> } > foo () { echo foo } >&3
> } > 
> } > becomes
> } > 
> } > foo () { { echo foo } >&3 }
> } 
> } This appears to be about the only way I could even conceivably get this
> } to work --- otherwise it means reengineering multiple different ways of
> } constructing and reading a dump file --- but even this appears too
> } difficult.
> 
> It's also apparently wrong, if this ...
> 
> } +function is executed.  Any variables in the redirection are expanded
> } +at the point the function is executed, but outside the function scope.
> 
> ... is actually the way it's supposed to work.

It doesn't make any obvious practical difference simply to expanding
variables given that no variables appear in the function before the
redirection is applied.

> torch% cat /tmp/foo
> foo() { echo foo } >&3
> torch% zcompile -k /tmp/foo
> torch% autoload foo
> torch% FPATH=/tmp foo      
> foo
> 
> Oops, the redirection didn't get applied.  But it does once the function
> is loaded and then called:
> 
> torch% functions foo
> foo () {
>         echo foo
> } >&3
> torch% foo
> zsh: 3: bad file descriptor
> torch% 

Not sure how that could happen --- there must be some kludge to run the
function the first time.  I guess this is specific to ksh style ---
there may be something funny with the EF_RUN flag.

pws

Re: PATCH: functions with redirections

[Peter Stephenson]

On Thu, 02 Oct 2014 09:35:09 +0100
Peter Stephenson <p.stephenson@samsung.com> wrote:
> > torch% cat /tmp/foo
> > foo() { echo foo } >&3
> > torch% zcompile -k /tmp/foo
> > torch% autoload foo
> > torch% FPATH=/tmp foo      
> > foo
> > 
> > Oops, the redirection didn't get applied.
> 
> Not sure how that could happen --- there must be some kludge to run the
> function the first time.

That's correct:  we use a very simple execution environment to execute a
function that's just been autoloaded.  This isn't actually related to
dumps --- we have the same problem with a normal ksh autoload.

The fix is to promote the point where we actually load (but don't
execute) the function.  We do this for all autoloads to find out if
there are redirections.  Having done that we are in a position to
extract the redirections as usual.

The normal zsh case wouldn't have redirections at that point because the
syntax doesn't allow it.  The kludged zsh case with an explicit call to
the function (the one I originally tested) works because in that case
all the normal function definition code is executed; it's not a special
case as far as redirection code is concerned.

We don't currently set errflag when failing to load a function (unless
it happens deeper than I've looked), so we still don't.  There'a at
least some sign this is deliberate (warnings rather than errors).

Moving the load up might have some very minor side effect on some error
case where previously we wouldn't have defined the function but now do.

pws

diff --git a/Src/exec.c b/Src/exec.c
index 10f71da..a5452e5 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -2789,13 +2789,6 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	    errflag = 1;
     }
 
-    if (errflag) {
-	lastval = 1;
-	if (oautocont >= 0)
-	    opts[AUTOCONTINUE] = oautocont;
-	return;
-    }
-
     if (type == WC_FUNCDEF) {
 	/*
 	 * The first word of a function definition is a list of
@@ -2809,8 +2802,24 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	    /* Nonymous, don't do redirections here */
 	    redir = NULL;
 	}
-    } else if (is_shfunc) {
-	Shfunc shf = (Shfunc)hn;
+    } else if (is_shfunc || type == WC_AUTOFN) {
+	Shfunc shf;
+	if (is_shfunc)
+	    shf = (Shfunc)hn;
+	else {
+	    shf = loadautofn(state->prog->shf, 1, 0);
+	    if (shf)
+		state->prog->shf = shf;
+	    else {
+		/*
+		 * This doesn't set errflag, so just return now.
+		 */
+		lastval = 1;
+		if (oautocont >= 0)
+		    opts[AUTOCONTINUE] = oautocont;
+		return;
+	    }
+	}
 	/*
 	 * A function definition may have a list of additional
 	 * redirections to apply, so retrieve it.
@@ -2832,6 +2841,13 @@ execcmd(Estate state, int input, int output, int how, int last1)
 	}
     }
 
+    if (errflag) {
+	lastval = 1;
+	if (oautocont >= 0)
+	    opts[AUTOCONTINUE] = oautocont;
+	return;
+    }
+
     if (type == WC_SIMPLE && !nullexec) {
 	char *s;
 	char trycd = (isset(AUTOCD) && isset(SHINSTDIN) &&
@@ -3306,10 +3322,18 @@ execcmd(Estate state, int input, int output, int how, int last1)
 		redir_prog = NULL;
 	    
 	    lastval = execfuncdef(state, redir_prog);
-	} else if (type >= WC_CURSH) {
+	} 
+	else if (type >= WC_CURSH) {
 	    if (last1 == 1)
 		do_exec = 1;
-	    lastval = (execfuncs[type - WC_CURSH])(state, do_exec);
+	    if (type == WC_AUTOFN) {
+		/*
+		 * We pre-loaded this to get any redirs.
+		 * So we execuate a simplified function here.
+		 */
+		lastval =  execautofn_basic(state, do_exec);
+	    } else
+		lastval = (execfuncs[type - WC_CURSH])(state, do_exec);
 	} else if (is_builtin || is_shfunc) {
 	    LinkList restorelist = 0, removelist = 0;
 	    /* builtin or shell function */
@@ -4540,21 +4564,28 @@ execshfunc(Shfunc shf, LinkList args)
 	deletefilelist(last_file_list, 0);
 }
 
-/* Function to execute the special type of command that represents an *
- * autoloaded shell function.  The command structure tells us which   *
- * function it is.  This function is actually called as part of the   *
- * execution of the autoloaded function itself, so when the function  *
- * has been autoloaded, its list is just run with no frills.          */
+/*
+ * Function to execute the special type of command that represents an
+ * autoloaded shell function.  The command structure tells us which
+ * function it is.  This function is actually called as part of the
+ * execution of the autoloaded function itself, so when the function
+ * has been autoloaded, its list is just run with no frills.
+ *
+ * There are two cases because if we are doing all-singing, all-dancing
+ * non-simple code we load the shell function early in execcmd() (the
+ * action also present in the non-basic version) to check if
+ * there are redirections that need to be handled at that point.
+ * Then we call execautofn_basic() to do the rest.
+ */
 
 /**/
 static int
-execautofn(Estate state, UNUSED(int do_exec))
+execautofn_basic(Estate state, UNUSED(int do_exec))
 {
     Shfunc shf;
     char *oldscriptname, *oldscriptfilename;
 
-    if (!(shf = loadautofn(state->prog->shf, 1, 0)))
-	return 1;
+    shf = state->prog->shf;
 
     /*
      * Probably we didn't know the filename where this function was
@@ -4574,6 +4605,19 @@ execautofn(Estate state, UNUSED(int do_exec))
 }
 
 /**/
+static int
+execautofn(Estate state, UNUSED(int do_exec))
+{
+    Shfunc shf;
+
+    if (!(shf = loadautofn(state->prog->shf, 1, 0)))
+	return 1;
+
+    state->prog->shf = shf;
+    return execautofn_basic(state, 0);
+}
+
+/**/
 Shfunc
 loadautofn(Shfunc shf, int fksh, int autol)
 {
diff --git a/Test/A05execution.ztst b/Test/A05execution.ztst
index df6d7bc..8d256ff 100644
--- a/Test/A05execution.ztst
+++ b/Test/A05execution.ztst
@@ -216,3 +216,16 @@ F:This similar test was triggering a reproducible failure with pipestatus.
 >done
 F:This test checks for a file descriptor leak that could cause the left
 F:side of a pipe to block on write after the right side has exited
+
+  print "autoload_redir() { print Autoloaded ksh style; } >autoload.log" >autoload_redir
+  autoload -Uk autoload_redir
+  autoload_redir
+  print No output yet
+  cat autoload.log
+  functions autoload_redir
+0:
+>No output yet
+>Autoloaded ksh style
+>autoload_redir () {
+>	print Autoloaded ksh style
+>} > autoload.log

Re: PATCH: functions with redirections

[Bart Schaefer]

(Sorry for the extensive context for short responses.)

On Oct 2,  9:35am, Peter Stephenson wrote:
}
} On Wed, 01 Oct 2014 21:07:41 -0700
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > On Oct 1,  8:17pm, Peter Stephenson wrote:
} > }
} > } On Mon, 29 Sep 2014 14:37:35 -0700
} > } Bart Schaefer <schaefer@brasslantern.com> wrote:
} > } > 
} > } > foo () { { echo foo } >&3 }
} > 
} > } +function is executed.  Any variables in the redirection are expanded
} > } +at the point the function is executed, but outside the function scope.
} 
} It doesn't make any obvious practical difference simply to expanding
} variables given that no variables appear in the function before the
} redirection is applied.

It makes a practical difference if $argv or its aliases are mentioned, or
for special parameters like funcstack and zsh_eval_context.


} > torch% cat /tmp/foo
} > foo() { echo foo } >&3
} > torch% zcompile -k /tmp/foo
} > torch% autoload foo
} > torch% FPATH=/tmp foo      
} > foo
} > 
} > Oops, the redirection didn't get applied.
} 
} Not sure how that could happen --- there must be some kludge to run the
} function the first time.  I guess this is specific to ksh style ---
} there may be something funny with the EF_RUN flag.

loadautofn() does a direct call to execode() in some cases but not in
others.  That or something like it is probably bypassing the redirect
handling.

Re: PATCH: functions with redirections

[Bart Schaefer]

On Oct 2,  9:35am, Peter Stephenson wrote:
}
} > } +function is executed.  Any variables in the redirection are expanded
} > } +at the point the function is executed, but outside the function scope.
} > 
} > ... is actually the way it's supposed to work.

Bash appears to evaluate the redirects IN the function scope:

$ foo() { echo bar; } >$1
[schaefer@torch zsh-5.0-build]$ foo
$1: ambiguous redirect
$ foo /dev/null
$ 

Re: PATCH: functions with redirections

[Peter Stephenson]

On Thu, 02 Oct 2014 08:56:14 -0700
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Oct 2,  9:35am, Peter Stephenson wrote:
> }
> } > } +function is executed.  Any variables in the redirection are expanded
> } > } +at the point the function is executed, but outside the function scope.
> } > 
> } > ... is actually the way it's supposed to work.
> 
> Bash appears to evaluate the redirects IN the function scope:

We don't really have that option without a great deal of code
rearrangement and, while I presume in that case it's the right thing to
do, it's not obvious from the syntax.  So it can stay the way it is for
now.

pws