Bug in alias expansion

Bug in alias expansion

[Kynn Jones]

The example below illustrates the problem:

#### OK VERSION 5.0.7 ####
% zsh --version
zsh 5.0.7 (x86_64-pc-linux-gnu)
% typeset -A frobozz
% alias -g foo='echo xyz'
% frobozz[$(foo)]=9
% echo ${(kv)frobozz}
xyz 9

#### FAILING VERSION 5.1 ####
% zsh --version
zsh 5.1 (i386-unknown-netbsdelf6.1)
% typeset -A frobozz
% alias -g foo='echo xyz'
% frobozz[$(foo)]=9
zsh: not an identifier: frobozz[$(fooech9


In the examples above, the code is identical in both cases, up to (and
including) the first failing command.

Failure reproduced also under zsh 5.1.1 (x86_64-debian-linux-gnu).
(Ref: http://unix.stackexchange.com/q/242730/10618)

kj

Re: Bug in alias expansion

[Bart Schaefer]

On Nov 13,  1:03pm, Kynn Jones wrote:
}
} % typeset -A frobozz
} % alias -g foo='echo xyz'
} % frobozz[$(foo)]=9
} zsh: not an identifier: frobozz[$(fooech9

Confirmed.  This only occurs for assignments in command position:

torch% typeset frobozz[$(foo)]=9
torch% print ${(kv)frobozz}
xyz 9
torch% 

Re: Bug in alias expansion

[Kynn Jones]

On Fri, Nov 13, 2015 at 8:03 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Nov 13,  1:03pm, Kynn Jones wrote:
> }
> } % typeset -A frobozz
> } % alias -g foo='echo xyz'
> } % frobozz[$(foo)]=9
> } zsh: not an identifier: frobozz[$(fooech9
>
> Confirmed.  This only occurs for assignments in command position:

Thanks for the confirmation.


> ... This only occurs for assignments in command position:

> torch% typeset frobozz[$(foo)]=9
> torch% print ${(kv)frobozz}
> xyz 9
> torch%
>

Unfortunately, the assignment preceded by `typeset` fails under 5.0.7

% alias -g foo='echo xyz'
% typeset frobozz[$(foo)]=9
zsh: no matches found: frobozz[xyz]=9

Re: Bug in alias expansion

[Bart Schaefer]

On Nov 14,  1:57pm, Kynn Jones wrote:
}
} Unfortunately, the assignment preceded by `typeset` fails under 5.0.7
} 
} % alias -g foo='echo xyz'
} % typeset frobozz[$(foo)]=9
} zsh: no matches found: frobozz[xyz]=9

That's just normal file globbing taking place:

% noglob typeset frobozz[$(foo)]=9

or

% typeset "frobozz[($foo)]"=9

Re: Bug in alias expansion

[Kynn Jones]

On Sat, Nov 14, 2015 at 2:03 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Nov 14,  1:57pm, Kynn Jones wrote:
> }
> } Unfortunately, the assignment preceded by `typeset` fails under 5.0.7
> }
> } % alias -g foo='echo xyz'
> } % typeset frobozz[$(foo)]=9
> } zsh: no matches found: frobozz[xyz]=9
>
> That's just normal file globbing taking place:
>
> % noglob typeset frobozz[$(foo)]=9
>
> or
>
> % typeset "frobozz[($foo)]"=9

Thanks for the clarification.

I still find it disconcerting that file globbing would get in the way
under v. 5.0.7 but not under v. 5.1.

Re: Bug in alias expansion

[Bart Schaefer]

On Nov 14,  2:19pm, Kynn Jones wrote:
}
} I still find it disconcerting that file globbing would get in the way
} under v. 5.0.7 but not under v. 5.1.

Between 5.0.7 and 5.1 we introduced a keyword for "typeset" so that the
parser could distinguish assignments-in-argument-position from simple
arguments.  In 5.0.7 and earlier "typeset" is merely a command, so its
arguments are treated like any other shell word.

Re: Bug in alias expansion

[Kynn Jones]

On Sat, Nov 14, 2015 at 4:40 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Nov 14,  2:19pm, Kynn Jones wrote:
> }
> } I still find it disconcerting that file globbing would get in the way
> } under v. 5.0.7 but not under v. 5.1.
>
> Between 5.0.7 and 5.1 we introduced a keyword for "typeset" so that the
> parser could distinguish assignments-in-argument-position from simple
> arguments.  In 5.0.7 and earlier "typeset" is merely a command, so its
> arguments are treated like any other shell word.

Thanks again.  That's very helpful.

Re: Bug in alias expansion

[Peter Stephenson]

On Fri, 13 Nov 2015 13:03:52 -0500
Kynn Jones <kynnjo@gmail.com> wrote:
> % typeset -A frobozz
> % alias -g foo='echo xyz'
> % frobozz[$(foo)]=9
> zsh: not an identifier: frobozz[$(fooech9

This is somewhere in the vicinity of parse_subscript().  That's called
both from isident() and getindex(); I got as far as the case called from
isident(), but it may be the second one gets called, too.  The
suspicious bit in parse_subscript() is this:

    strinbeg(0);
    lexbuf.len = 0;
    lexbuf.ptr = tokstr = s;
    lexbuf.siz = l + 1;
    err = dquote_parse(endchar, sub);
    if (err) {
	err = *lexbuf.ptr;
	*lexbuf.ptr = '\0';
	untokenize(s);
	*lexbuf.ptr = err;
	s = NULL;
    } else {
	s = lexbuf.ptr;
    }
    strinend();

This is assuming the tokstr you get back is the same you sent down,
which it may not because it might need reallocating --- it does indeed
in this case because we are expanding an alias inside.  It confused me a
bit that it's only the end of the string (s, from lexbuf.ptr) that's
being passed back, but that's presumably because it's assuming the start
doesn't move, which is the (or an) erroneous assumption.  It's possible
the fix therefore is simply ensuring that both the start and the end of
the string are pased back and the start passed in forgotten.

Howver, I don't really understand the !ss[1] test (ss is the pointer
passed back) in isident(): it seems to assume the ']' or whatever else
comes at the end of the LHS of the assignment, has been separated off
from anything else, which possibly happens higher up.  When I walked
through this there didn't appear to be any reason why the next character
would be a null since I couldn't see dquote_parse() adding one and I
don't think we've even got that far in reading the input supplied for
this case (as it's only parsing as far as endchar) --- but this may be
part of the same assumption that we've still got the string passed in
down below.  So it's possible it's making even dodgier assumptions that
if you write the output first then something magic is going to happen
about the later parts of it, which it doesn't always.  (Note that the
input is at least a duplicated version of the string being used as the
output area, it's not the "s" in the chunk above itself.)  This might,
for example, be a shortcut instead of testing for end of input after the
']'.  So possibly making parse_subscript() get the next input and look
to see if it's after the end. then passing that back, is also necessary.

I don't know why this works in other contexts.

dquote_parse() has got some comments at the top that look like me trying
to work out what the heck it actually does, which is a bad sign.

Note initial parsing worked OK --- it's got as far as attempting to
evaluate the subscript for the assignment.  Sot it must be broken
assumptions later on rather than failure in the loewr layers.

pws

Re: Bug in alias expansion

[Bart Schaefer]

On Nov 15,  8:03pm, Peter Stephenson wrote:
}
} On Fri, 13 Nov 2015 13:03:52 -0500
} Kynn Jones <kynnjo@gmail.com> wrote:
} > % typeset -A frobozz
} > % alias -g foo='echo xyz'
} > % frobozz[$(foo)]=9
} > zsh: not an identifier: frobozz[$(fooech9
} 
} This is somewhere in the vicinity of parse_subscript().  That's called
} both from isident() and getindex(); I got as far as the case called from
} isident(), but it may be the second one gets called, too.

[...]

} I don't know why this works in other contexts.

When called from "typeset frobozz[$(foo)]=9" the ($foo) has been expanded
before parse_subscript() is called the first time [via isident()]:

Breakpoint 1, parse_subscript (s=0xb7d919e8 "xyz]", sub=1, endchar=93)
    at ../../zsh-5.0/Src/lex.c:1620

When called as a plain assignment without the "typeset" keyword, $(foo)
has not yet been expanded:

Breakpoint 1, parse_subscript (s=0xb7d917f8 "$(foo)]", sub=1, endchar=93)
    at ../../zsh-5.0/Src/lex.c:1620

This call is originating from addvars():

#0  parse_subscript (s=0xb7d91820 "$(foo)]", sub=1, endchar=93)
    at ../../zsh-5.0/Src/lex.c:1620
#1  0x080b2334 in isident (s=0xb7d91818 "frobozz[$(foo)]")
    at ../../zsh-5.0/Src/params.c:1078
#2  0x080b6660 in assignsparam (s=0xb7d91818 "frobozz[$(foo)]", 
    val=0x88bbdf0 "9", flags=0) at ../../zsh-5.0/Src/params.c:2741
#3  0x08078342 in addvars (state=0xbfe30f70, pc=0xb7d917a8, addflags=0)
    at ../../zsh-5.0/Src/exec.c:2348
#4  0x08074dff in execsimple (state=0xbfe30f70)
    at ../../zsh-5.0/Src/exec.c:1118

So the real question may be, how did we get as far as execsimple()
without having expanded all the aliases first?

Re: Bug in alias expansion

[Bart Schaefer]

On Nov 15,  1:52pm, Bart Schaefer wrote:
}
} So the real question may be, how did we get as far as execsimple()
} without having expanded all the aliases first?

Without being able to answer that ... we can work around the issue by
shoving alias expansion down into parse_string() like so:


diff --git a/Src/exec.c b/Src/exec.c
index c0ee527..b1ca551 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -217,6 +217,11 @@ parse_string(char *s, int reset_lineno)
 {
     Eprog p;
     zlong oldlineno;
+    int noalias = noaliases;
+
+    /* See addvars() */
+    if (noaliases < 0)
+	noaliases = 0;
 
     zcontext_save();
     inpush(s, INP_LINENO, NULL);
@@ -231,6 +236,7 @@ parse_string(char *s, int reset_lineno)
     strinend();
     inpop();
     zcontext_restore();
+    noaliases = noalias;
     return p;
 }
 
@@ -2249,10 +2255,15 @@ static void
 addvars(Estate state, Wordcode pc, int addflags)
 {
     LinkList vl;
+    int noalias = noaliases;
     int xtr, isstr, htok = 0;
     char **arr, **ptr, *name;
     int flags;
 
+    /* Signal to parse_string() that it should expand aliases */
+    if (!noaliases)
+	noaliases = -1;
+
     Wordcode opc = state->pc;
     wordcode ac;
     local_list1(svl);
@@ -2291,10 +2302,8 @@ addvars(Estate state, Wordcode pc, int addflags)
 	if (vl && htok) {
 	    prefork(vl, (isstr ? (PREFORK_SINGLE|PREFORK_ASSIGN) :
 			 PREFORK_ASSIGN), NULL);
-	    if (errflag) {
-		state->pc = opc;
-		return;
-	    }
+	    if (errflag)
+		break;
 	    if (!isstr || (isset(GLOBASSIGN) && isstr &&
 			   haswilds((char *)getdata(firstnode(vl))))) {
 		globlist(vl, 0);
@@ -2305,10 +2314,8 @@ addvars(Estate state, Wordcode pc, int addflags)
 		if (isset(GLOBASSIGN) && isstr)
 		    unsetparam(name);
 	    }
-	    if (errflag) {
-		state->pc = opc;
-		return;
-	    }
+	    if (errflag)
+		break;
 	}
 	if (isstr && (empty(vl) || !nextnode(firstnode(vl)))) {
 	    Param pm;
@@ -2331,8 +2338,7 @@ addvars(Estate state, Wordcode pc, int addflags)
 		    (pm->node.flags & PM_RESTRICTED)) {
 		    zerr("%s: restricted", pm->node.nam);
 		    zsfree(val);
-		    state->pc = opc;
-		    return;
+		    break;
 		}
 		if (strcmp(name, "STTY") == 0) {
 		    zsfree(STTYval);
@@ -2346,10 +2352,8 @@ addvars(Estate state, Wordcode pc, int addflags)
 		opts[ALLEXPORT] = allexp;
 	    } else
 	    	pm = assignsparam(name, val, myflags);
-	    if (errflag) {
-		state->pc = opc;
-		return;
-	    }
+	    if (errflag)
+		break;
 	    continue;
 	}
 	if (vl) {
@@ -2377,6 +2381,7 @@ addvars(Estate state, Wordcode pc, int addflags)
 	}
     }
     state->pc = opc;
+    noaliases = noalias;
 }
 
 /**/

Re: Bug in alias expansion

[Bart Schaefer]

On Nov 15,  8:03pm, Peter Stephenson wrote:
}
} I don't know why this works in other contexts.

Addendum:  To some extent it does not.

torch% alias foo='echo x)$(:'
torch% print $(foo)             
zsh: command not found: fooecho

This should of course expand to

    print $( echo x)$(: )

which is perfectly valid and definitely should not produce "fooecho",
so the backtracking is still messed up somewhere and parse_subscript()
is potentially a red herring.

In older versions of zsh it gagged in a different way:

% alias -g foo='echo x)$(:' 
% print $(foo)
zsh: parse error near `)'
zsh: parse error in command substitution

Re: Bug in alias expansion

[Mikael Magnusson]

On Sun, Nov 15, 2015 at 11:48 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Nov 15,  8:03pm, Peter Stephenson wrote:
> }
> } I don't know why this works in other contexts.
>
> Addendum:  To some extent it does not.
>
> torch% alias foo='echo x)$(:'
> torch% print $(foo)
> zsh: command not found: fooecho
>
> This should of course expand to
>
>     print $( echo x)$(: )
>
> which is perfectly valid and definitely should not produce "fooecho",
> so the backtracking is still messed up somewhere and parse_subscript()
> is potentially a red herring.
>
> In older versions of zsh it gagged in a different way:
>
> % alias -g foo='echo x)$(:'
> % print $(foo)
> zsh: parse error near `)'
> zsh: parse error in command substitution

This error is definitely what I would expect from the above command.
It's expanded in the subshell¹, so it shouldn't possibly be able to
affect the syntax of the parent shell.

¹
% alias -g foo=bar
% echo $(foo)
zsh: command not found: bar
% unalias \foo; foo; echo $(foo)
zsh: command not found: bar
zsh: command not found: foo

-- 
Mikael Magnusson

Re: Bug in alias expansion

[Bart Schaefer]

On Nov 16,  1:50am, Mikael Magnusson wrote:
} Subject: Re: Bug in alias expansion
}
} > In older versions of zsh it gagged in a different way:
} >
} > % alias -g foo='echo x)$(:'
} > % print $(foo)
} > zsh: parse error near `)'
} > zsh: parse error in command substitution
} 
} This error is definitely what I would expect from the above command.
} It's expanded in the subshell

If this were a normal command alias, I'd agree with you, but this is
a global alias -- the parent shell should expand it wherever it is an
unquoted shell word in the original command line, before the subshell
is ever started.

The "workaround" I posted in 37122 behaves the way you're thinking,
but shouldn't be necessary.

I suppose there's an argument to be made that a word inside $(...) is
"quoted", but if so. the example I showed for "typeset frobozz[$(foo)]"
(where xyz is expanded before parse_subscript() is called) must be
wrong.

Re: Bug in alias expansion

[Mikael Magnusson]

On Mon, Nov 16, 2015 at 4:24 AM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Nov 16,  1:50am, Mikael Magnusson wrote:
> } Subject: Re: Bug in alias expansion
> }
> } > In older versions of zsh it gagged in a different way:
> } >
> } > % alias -g foo='echo x)$(:'
> } > % print $(foo)
> } > zsh: parse error near `)'
> } > zsh: parse error in command substitution
> }
> } This error is definitely what I would expect from the above command.
> } It's expanded in the subshell
>
> If this were a normal command alias, I'd agree with you, but this is
> a global alias -- the parent shell should expand it wherever it is an
> unquoted shell word in the original command line, before the subshell
> is ever started.

As the commands you didn't quote show, this doesn't seem to be the
case, and I don't think it should either.

> The "workaround" I posted in 37122 behaves the way you're thinking,
> but shouldn't be necessary.
>
> I suppose there's an argument to be made that a word inside $(...) is
> "quoted", but if so. the example I showed for "typeset frobozz[$(foo)]"
> (where xyz is expanded before parse_subscript() is called) must be
> wrong.


-- 
Mikael Magnusson

Re: Bug in alias expansion

[Peter Stephenson]

On Sun, 15 Nov 2015 20:03:56 +0000
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> On Fri, 13 Nov 2015 13:03:52 -0500
> Kynn Jones <kynnjo@gmail.com> wrote:
> > % typeset -A frobozz
> > % alias -g foo='echo xyz'
> > % frobozz[$(foo)]=9
> > zsh: not an identifier: frobozz[$(fooech9
>...
> This is assuming the tokstr you get back is the same you sent down,
> which it may not because it might need reallocating --- it does indeed
> in this case because we are expanding an alias inside.  It confused me a
> bit that it's only the end of the string (s, from lexbuf.ptr) that's
> being passed back, but that's presumably because it's assuming the start
> doesn't move, which is the (or an) erroneous assumption.  It's possible
> the fix therefore is simply ensuring that both the start and the end of
> the string are pased back and the start passed in forgotten.

I abandoned this after finding out that a whole tower of functions
(is that a collective noun for functions, or only in completion code?)
sitting on top of parse_subscript() is relying on the in-place
tokenisation that was screwing up the string.

We can get rid of the currently visible problems by allocating a new
string for the token and copying back the result to the original.

It's not very efficient, and it still has the problem that if something
funny happens to the length --- and I don't think there's any guarantee
of length preservation built into the lexer even though it happens to
work in the cases we've looked at so far --- then somebody's going to
get hurt.

However, it does remove the immediate problem (is functionally no
worse than before the problem was introduced) and the inefficiency
should be minor unless the subscripts get really huge.

In the error case, we could possibly just not do the copy but move
the end pointer; however, that case doesn't need to be efficient
and I don't want to change more of the logic than is necessary.

diff --git a/Src/lex.c b/Src/lex.c
index 89af961..81904c1 100644
--- a/Src/lex.c
+++ b/Src/lex.c
@@ -1617,7 +1617,7 @@ parsestrnoerr(char **s)
 mod_export char *
 parse_subscript(char *s, int sub, int endchar)
 {
-    int l = strlen(s), err;
+    int l = strlen(s), err, toklen;
     char *t;
 
     if (!*s || *s == endchar)
@@ -1626,18 +1626,34 @@ parse_subscript(char *s, int sub, int endchar)
     untokenize(t = dupstring(s));
     inpush(t, 0, NULL);
     strinbeg(0);
+    /*
+     * Warning to Future Generations:
+     *
+     * This way of passing the subscript through the lexer is brittle.
+     * Code above this for several layers assumes that when we tokenise
+     * the input it goes into the same place as the original string.
+     * However, the lexer may overwrite later bits of the string or
+     * reallocate it, in particular when expanding aliaes.  To get
+     * around this, we copy the string and then copy it back.  This is a
+     * bit more robust but still relies on the underlying assumption of
+     * length preservation.
+     */
     lexbuf.len = 0;
-    lexbuf.ptr = tokstr = s;
+    lexbuf.ptr = tokstr = dupstring(s);
     lexbuf.siz = l + 1;
     err = dquote_parse(endchar, sub);
+    toklen = (int)(lexbuf.ptr - tokstr);
+    DPUTS(toklen > l, "Bad length for parsed subscript");
+    memcpy(s, tokstr, toklen);
     if (err) {
-	err = *lexbuf.ptr;
-	*lexbuf.ptr = '\0';
+	char *strend = s + toklen;
+	err = *strend;
+	*strend = '\0';
 	untokenize(s);
-	*lexbuf.ptr = err;
+	*strend = err;
 	s = NULL;
     } else {
-	s = lexbuf.ptr;
+	s += toklen;
     }
     strinend();
     inpop();
diff --git a/Test/D06subscript.ztst b/Test/D06subscript.ztst
index cffca74..1449236 100644
--- a/Test/D06subscript.ztst
+++ b/Test/D06subscript.ztst
@@ -249,3 +249,20 @@
   string[0]=!
 1:Can't set only element zero of string
 ?(eval):1: string: assignment to invalid subscript range
+
+  typeset -A assoc=(leader topcat officer dibble sidekick choochoo)
+  alias myind='echo leader' myletter='echo 1' myletter2='echo 4'
+  print ${assoc[$(myind)]}
+  print $assoc[$(myind)]
+  print ${assoc[$(myind)][$(myletter)]}${assoc[$(myind)][$(myletter2)]}
+  assoc[$(myind)]='of the gang'
+  print ${assoc[$(myind)]}
+  print $assoc[$(myind)]
+  print $assoc[leader]
+0: Parsing subscript with non-trivial tokenisation
+>topcat
+>topcat
+>tc
+>of the gang
+>of the gang
+>of the gang

Re: Bug in alias expansion

[Bart Schaefer]

On Nov 17,  5:29pm, Peter Stephenson wrote:
}
} We can get rid of the currently visible problems by allocating a new
} string for the token and copying back the result to the original.

A few remarks about this.

(1) This doesn't resolve the problem I pointed out in workers/37123.

(2) Referring to the discussion I've been having with Mikael (cf.
    workers/37126), with this patch the alias is expanded in the
    parent shell for "alias -g" and in the subshell without -g.
    When the $(...) expansion is NOT in subscript context, on the
    other hand, the alias expands in the subshell for both global
    and command aliases.

(3) I'm fibbing in the last sentence of (2).  The alias is expanded
    BOTH in the parent shell (which is where the "fooecho" bug from
    37123 arises) AND in the subshell.  In the parent shell the alias
    is expanded during the parse but then (in the case where it lexes
    correctly) the expansion is thrown away, only to be re-expanded
    in the subshell.

Consequently ... either Mikael is right that the alias expansion
should be forced into the subshell, your patch is thus "wrong," and
my patch in 37122 is closer to what should happen; or the parent is
responsible for the alias, and when we fix 37123 we should make the
behavior outside of subscripts consistent with the behavior inside.

(Of course 37128 and 37122 aren't mutually exclusive, we can apply
both; there's probably some other case where subscript parsing may
go wrong that's fixed by 37128.)

} It's not very efficient, and it still has the problem that if something
} funny happens to the length --- and I don't think there's any guarantee
} of length preservation built into the lexer even though it happens to
} work in the cases we've looked at so far --- then somebody's going to
} get hurt.

I'm not following this stuff about the length -- it can't mean that the
length of the alias expansion is the same as the length of the aliased
token -- so to what does it refer?

Re: Bug in alias expansion

[Peter Stephenson]

On Tue, 17 Nov 2015 20:55:08 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> (Of course 37128 and 37122 aren't mutually exclusive, we can apply
> both; there's probably some other case where subscript parsing may
> go wrong that's fixed by 37128.)

37128 is fixing a particular problem to do with assumptions about
parsing that are no longer valid; for this, it is entirely irrelevant
where the alias actually gets expanded.  I haven't followed whether
there is a visible bug as a consequence of the latter.

> } It's not very efficient, and it still has the problem that if something
> } funny happens to the length --- and I don't think there's any guarantee
> } of length preservation built into the lexer even though it happens to
> } work in the cases we've looked at so far --- then somebody's going to
> } get hurt.
> 
> I'm not following this stuff about the length -- it can't mean that the
> length of the alias expansion is the same as the length of the aliased
> token -- so to what does it refer?

The code assumes that if you pass a string through parse_subscript(), or
more accurately through dquote_parse(), then the sequence of characters
that comes out maps one-to-one onto the sequence of characters that went
in. For example, a ( turns into an Inpar, a " turns into a Dnull, and
so on --- similarly to if you simply tokenised it character by
character but with more context sensitivity.  The change makes the
alias expansion irrelevant to this --- we backtrack over that -- but
the code still relies on this assumption.  Given how much can now
happen inside dquote_parse() (as we see here, if only as a side issue
to the bug being fixed), this doesn't seem ideal.  However, I don't know
of a case where it fails.

pws

Re: Bug in alias expansion

[Peter Stephenson]

On Sun, 15 Nov 2015 14:48:05 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Nov 15,  8:03pm, Peter Stephenson wrote:
> }
> } I don't know why this works in other contexts.
> 
> Addendum:  To some extent it does not.
> 
> torch% alias foo='echo x)$(:'
> torch% print $(foo)
> zsh: command not found: fooecho

This is a completely different problem again.  There appear to be three
different but interacting issues:

- this one, which could be hairy as we emerge from parsing a $(...)
expansion with an alias unpopped --- I'm not sure yet if that's
tractable without making the code yet more unreadable

- the fact that parse_subscript() made invalid assumptions about what
was going underneath it, now fixed, at least to the point where it's
no worse than it ever was

- where alias expansion takes place, which would presumably be sensible
to define properly but which I haven't yet worked up an interest in.

pws

Re: Bug in alias expansion

[Peter Stephenson]

On Wed, 18 Nov 2015 10:42:54 +0000
Peter Stephenson <p.stephenson@samsung.com> wrote:
> > Addendum:  To some extent it does not.
> > 
> > torch% alias foo='echo x)$(:'
> > torch% print $(foo)
> > zsh: command not found: fooecho
> 
> This is a completely different problem again.  There appear to be three
> different but interacting issues:
> 
> - this one, which could be hairy as we emerge from parsing a $(...)
> expansion with an alias unpopped --- I'm not sure yet if that's
> tractable without making the code yet more unreadable

Actually, now I start looking, I think I'm inclining to Mikael's view on
the interaction between this and the position of alias expansion, even
if it's global.  At the top level we have $(, foo, ).  foo clearly
starts within the $(.  So it's hard to see how the $( can then end
within the foo; it seems to violate the hierarchy.  Yes, I know aliases
can violate hierarchies, but only once they're expanded, obviously.  I
can't see a sensible reason for expanding foo until we're already within
the context of the $(...) --- surely the intention of $(...) is to
protect the outside world from whatever's inside, and if foo isn't
inside I'm no longer sure which way up I am.

Currently we expand both when parsing the entrails of the $(...)
and executing it, but should possibly only do so in the latter case.
I haven't look to see if this affects any tests.

pws

Re: Bug in alias expansion

[Bart Schaefer]

On Nov 18,  2:13pm, Peter Stephenson wrote:
}
} Actually, now I start looking, I think I'm inclining to Mikael's view on
} the interaction between this and the position of alias expansion, even
} if it's global.  At the top level we have $(, foo, ).  foo clearly
} starts within the $(.  So it's hard to see how the $( can then end
} within the foo; it seems to violate the hierarchy.

OK, so by that logic a subscript is also in the hierarchy (and for that
matter is supposed to behave mostly like double-quotes, within which
aliases do not expand) so I should commit 37122.

} Currently we expand both when parsing the entrails of the $(...)
} and executing it, but should possibly only do so in the latter case.
} I haven't look to see if this affects any tests.

Would not-expanding affect cmd_or_math() adversely?  Is that why we
expand and then backtrack?

Re: Bug in alias expansion

[Peter Stephenson]

On Wed, 18 Nov 2015 07:52:46 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Nov 18,  2:13pm, Peter Stephenson wrote:
> } Currently we expand both when parsing the entrails of the $(...)
> } and executing it, but should possibly only do so in the latter case.
> } I haven't look to see if this affects any tests.
> 
> Would not-expanding affect cmd_or_math() adversely?  Is that why we
> expand and then backtrack?

I can't offhand think of a case.  The crucial thing about cmd_or_math()
is to get the parentheses right.  Being affected by this change implies
it's currently propagating the effect to some outer layer of expansion,
which we're in the process of deciding is probably a bad thing.  If this
is correct it applies recursively, so nested expansions aren't an issue.
Did you have some vague pointers in mind?

I don't believe there's ever a case where we won't do the alias
expansion when we finally execute the inside of a $(...), because it's
always reparsed for execution (considered a bug up to now but getting us
out of the present hole), and $((...)) is equivalent to math eval in
double quotes, so no global aliases, if that's what you mean.

Come to think of it, the name of the function dquote_parse() kind of
implies we're not expanding aliases, but then it was named before we
started handling $(...) better, so that's not a great pointer.

Anyway, there are some pretty hairy tests, so if we change it we should
find out quickly.

pws

Re: Bug in alias expansion

[Bart Schaefer]

On Nov 18,  4:14pm, Peter Stephenson wrote:
} Subject: Re: Bug in alias expansion
}
} On Wed, 18 Nov 2015 07:52:46 -0800
} Bart Schaefer <schaefer@brasslantern.com> wrote:
} > On Nov 18,  2:13pm, Peter Stephenson wrote:
} > } Currently we expand both when parsing the entrails of the $(...)
} > } and executing it, but should possibly only do so in the latter case.
} > } I haven't look to see if this affects any tests.
} > 
} > Would not-expanding affect cmd_or_math() adversely?  Is that why we
} > expand and then backtrack?
} 
} I can't offhand think of a case.  The crucial thing about cmd_or_math()
} is to get the parentheses right.

So it turns out that dquote_parse() has almost nothing to do with the
bug (though probably still a lot to do with whether the aliases ought
to expand at all); it's skipcomm() that causes the error.  This small
change fixes the problem seen in 37128:

diff --git a/Src/lex.c b/Src/lex.c
index 81904c1..0f260d0 100644
--- a/Src/lex.c
+++ b/Src/lex.c
@@ -2022,7 +2022,9 @@ skipcomm(void)
     int new_lexstop, new_lex_add_raw;
     int save_infor = infor;
     struct lexbufstate new_lexbuf;
+    int noalias = noaliases;
 
+    noaliases = 1;
     infor = 0;
     cmdpush(CS_CMDSUBST);
     SETPARBEGIN
@@ -2140,6 +2142,7 @@ skipcomm(void)
 	SETPAREND
     cmdpop();
     infor = save_infor;
+    noaliases = noalias;
 
     return lexstop;
 #endif

All "make check" tests still pass.  And now:

torch% alias -g foo='echo xyz)$(echo zyx'
torch% print $(foo)
zsh: parse error near `)'
zsh: parse error in command substitution
torch%