Bug: Wrong completion of $(( $(!cmd

Bug: Wrong completion of $(( $(!cmd

[Jörg Sommer]

Hi,

% echo 12
12
% echo $(( $(!echo<TAB>
% echo $($(( $(echo 12
% echo $ZSH_VERSION 
4.3.2-dev-1
% dpkg -l zsh-beta
||/ Name           Version                Beschreibung
+++-==============-======================-============================================
ii  zsh-beta       4.3.2-dev-1+20070102-1 A shell with lots of features (dev tree)

Bye, Jörg.
-- 
Die NASA brauchte 12 Jahre um einen Kugelschreiber zu entwickeln, der
kopfüber, in der Schwerelosigkeit und unter Wasser schreiben kann.
Die Russen benutzten einfach einen Bleistift …

Re: Bug: Wrong completion of $(( $(!cmd

[Geoff Wing]

On Saturday 2007-01-06 23:48 +1100, Jörg Sommer output:
:% echo 12
:12
:% echo $(( $(!echo<TAB>
:% echo $($(( $(echo 12

and inungetc() just after gives me 11 lines of
	Warning: backing up wrong character.

Regards,
Geoff

Re: Bug: Wrong completion of $(( $(!cmd

[Peter Stephenson]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]

Geoff Wing wrote:
> On Saturday 2007-01-06 23:48 +1100, Jörg Sommer output:
> :% echo 12
> :12
> :% echo $(( $(!echo<TAB>
> :% echo $($(( $(echo 12
> 
> and inungetc() just after gives me 11 lines of
> 	Warning: backing up wrong character.

The problem is somewhere here (in lex.c).  If dquote_parse() returns
non-zero it's an error indication, not a character, so the hungetc(c) is
definitely wrong in that case.  However, I don't understand the
intention behind error handling at this point.  I think we probably need
to return 1 earlier.


static int
cmd_or_math(int cs_type)
{
    int oldlen = len;
    int c;

    cmdpush(cs_type);
    c = dquote_parse(')', 0);
    cmdpop();
    *bptr = '\0';
    if (!c) {
	c = hgetc();
	if (c == ')')
	    return 1;
	hungetc(c);
	lexstop = 0;
	c = ')';
    }
    hungetc(c);
    lexstop = 0;
    while (len > oldlen) {
	len--;
	hungetc(itok(*--bptr) ? ztokens[*bptr - Pound] : *bptr);
    }
    hungetc('(');
    return 0;
}

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070

Re: Bug: Wrong completion of $(( $(!cmd

[Peter Stephenson]

Peter Stephenson <pws@csr.com> wrote:
> Geoff Wing wrote:
> > On Saturday 2007-01-06 23:48 +1100, J_rg Sommer output:
> > :% echo 12
> > :12
> > :% echo $(( $(!echo<TAB>
> > :% echo $($(( $(echo 12
> > 
> > and inungetc() just after gives me 11 lines of
> > 	Warning: backing up wrong character.
> 
> The problem is somewhere here (in lex.c).  If dquote_parse() returns
> non-zero it's an error indication, not a character, so the hungetc(c) is
> definitely wrong in that case.  However, I don't understand the
> intention behind error handling at this point.  I think we probably need
> to return 1 earlier.

Is it this simple?  Seems to do the trick...

Index: Src/lex.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/lex.c,v
retrieving revision 1.37
diff -u -r1.37 lex.c
--- Src/lex.c	19 Jan 2007 21:36:03 -0000	1.37
+++ Src/lex.c	23 Jan 2007 13:08:47 -0000
@@ -542,14 +542,14 @@
     c = dquote_parse(')', 0);
     cmdpop();
     *bptr = '\0';
-    if (!c) {
-	c = hgetc();
-	if (c == ')')
-	    return 1;
-	hungetc(c);
-	lexstop = 0;
-	c = ')';
-    }
+    if (c)
+	return 1;
+    c = hgetc();
+    if (c == ')')
+	return 1;
+    hungetc(c);
+    lexstop = 0;
+    c = ')';
     hungetc(c);
     lexstop = 0;
     while (len > oldlen) {

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


To access the latest news from CSR copy this link into a web browser:  http://www.csr.com/email_sig.php

To get further information regarding CSR, please visit our Investor Relations page at http://ir.csr.com/csr/about/overview

Re: Bug: Wrong completion of $(( $(!cmd

[Bart Schaefer]

On Jan 23,  1:04pm, Peter Stephenson wrote:
}
} The problem is somewhere here (in lex.c). If dquote_parse() returns
} non-zero it's an error indication, not a character, so the hungetc(c)
} is definitely wrong in that case.

I don't think that's true.  If you look at the tail end of dquote_parse()
you'll see that if lexstop is false the return value of dquote_parse() is
overloaded -- it's both an error indication AND the most recently read
character from the input stream.  That's what hungetc() is trying to put
back in that spot in cmd_or_math().

That doesn't make it correct, but that's what's going on.  There must be
(or have been at some past time) some other reason higher up the parse
tree why that final character needed to be retained.

Re: Bug: Wrong completion of $(( $(!cmd

[Peter Stephenson]

Bart Schaefer wrote:
> On Jan 23,  1:04pm, Peter Stephenson wrote:
> }
> } The problem is somewhere here (in lex.c). If dquote_parse() returns
> } non-zero it's an error indication, not a character, so the hungetc(c)
> } is definitely wrong in that case.
> 
> I don't think that's true.

I presume you mean the second statement.  If you've got any evidence
against the first statement I'd like to see it.

> If you look at the tail end of dquote_parse()
> you'll see that if lexstop is false the return value of dquote_parse() is
> overloaded -- it's both an error indication AND the most recently read
> character from the input stream.  That's what hungetc() is trying to put
> back in that spot in cmd_or_math().
> 
> That doesn't make it correct, but that's what's going on.  There must be
> (or have been at some past time) some other reason higher up the parse
> tree why that final character needed to be retained.

I looked a bit further and it seems it's used in gettokstr() to
hungetc() a character when we have an error.  I don't understand what
that buys when we can't parse the input anyway, particularly since we
don't even know if it's a valid character because of the other way of
setting the value; could it have been a hack to get history lines to be
stored properly after an error?  I recall we improved that at some
point, so this hack may not be necessary.

I traced it (the "err = c" thing in dquote_parse()) back in the code to
3.0.0, which is before the use in cmd_or_math() that is causing the
problem, so it wasn't introduced specifically for that, and
cmd_or_math() was roughly the way it now is right back in 3.0.8.
Consequently (although what you say is true) I don't see anything wrong
with the proposed change.

-- 
Peter Stephenson <pws@csr.com>                  Software Engineer
CSR PLC, Churchill House, Cambridge Business Park, Cowley Road
Cambridge, CB4 0WZ, UK                          Tel: +44 (0)1223 692070


To access the latest news from CSR copy this link into a web browser:  http://www.csr.com/email_sig.php

To get further information regarding CSR, please visit our Investor Relations page at http://ir.csr.com/csr/about/overview

Re: Bug: Wrong completion of $(( $(!cmd

[Bart Schaefer]

On Jan 23,  3:35pm, Peter Stephenson wrote:
} Subject: Re: Bug: Wrong completion of $(( $(!cmd
}
} Bart Schaefer wrote:
} > On Jan 23,  1:04pm, Peter Stephenson wrote:
} > }
} > } The problem is somewhere here (in lex.c). If dquote_parse() returns
} > } non-zero it's an error indication, not a character, so the hungetc(c)
} > } is definitely wrong in that case.
} > 
} > I don't think that's true.
} 
} I presume you mean the second statement.

Yes.

} Consequently (although what you say is true) I don't see anything wrong
} with the proposed change.

OK.  Thanks for looking further.