PATCH: functions with redirections

[Peter Stephenson <p.w.stephenson@xxxxxxxxxxxx>]

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