PATCH: Error parsing $(...)

PATCH: Error parsing $(...)

[Tomáš Smetana]

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

Hello,
  I have stumbled upon a problem with comments in $(...) construct.  Consider
the following script:

# Comment containing '
VAR=$(
echo a
# Comment containing '
)
echo $VAR

This is syntactically correct but zsh would throw an error parsing it:
parse error near `VAR=$('

Note that the same script works OK in ksh or bash. It seems that the '\''
character doesn't lose its special meaning even though it appears in the
comment.  I have written a patch (attached) which seems to fix the problem.
Could you please review the patch and consider applying it?

Please cc: me in the answers I'm not subscribed.

Thanks and regards,
Tomas Smetana

[-- Attachment #2: zsh-subcomm.patch --]
[-- Type: text/x-patch, Size: 443 bytes --]

Index: Src/lex.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/lex.c,v
retrieving revision 1.49
diff -u -r1.49 lex.c
--- Src/lex.c	25 Feb 2009 10:24:01 -0000	1.49
+++ Src/lex.c	26 Feb 2009 08:28:08 -0000
@@ -1854,6 +1854,11 @@
 		else
 		    add(c);
 	    break;
+	case '#':
+	    add(c);
+	    while ((c = hgetc()) != '\n' && !lexstop)
+		add(c);
+	    break;
 	}
     }
     while (pct);

Re: PATCH: Error parsing $(...)

[Peter Stephenson]

On Thu, 26 Feb 2009 09:45:07 +0100
Tomáš Smetana <t.smetana@gmail.com> wrote:
> # Comment containing '
> VAR=$(
> echo a
> # Comment containing '
> )
> echo $VAR
> 
> This is syntactically correct but zsh would throw an error parsing it:
> parse error near `VAR=$('
>
> I have written a patch (attached) which seems to fix the problem.
> Could you please review the patch and consider applying it?

Thanks for reporting this.

It's not quite that simple, since "#" is only a comment character
at the start of a word.  The following is better, I hope I haven't
missed any cases...

(We could do with some tests for this... and plenty of other things.
Anyone can write tests, they're just ordinary shell code comparing
against output and status.)

Index: Src/lex.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/lex.c,v
retrieving revision 1.49
diff -u -r1.49 lex.c
--- Src/lex.c	25 Feb 2009 10:24:01 -0000	1.49
+++ Src/lex.c	26 Feb 2009 22:49:04 -0000
@@ -1802,16 +1802,18 @@
 static int
 skipcomm(void)
 {
-    int pct = 1, c;
+    int pct = 1, c, start = 1;
 
     cmdpush(CS_CMDSUBST);
     SETPARBEGIN
     c = Inpar;
     do {
+	int iswhite;
 	add(c);
 	c = hgetc();
 	if (itok(c) || lexstop)
 	    break;
+	iswhite = isep(c);
 	switch (c) {
 	case '(':
 	    pct++;
@@ -1854,7 +1856,15 @@
 		else
 		    add(c);
 	    break;
+	case '#':
+	    if (start) {
+		add(c);
+		while ((c = hgetc()) != '\n' && !lexstop)
+		    add(c);
+	    }
+	    break;
 	}
+	start = iswhite;
     }
     while (pct);
     if (!lexstop)

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

Re: PATCH: Error parsing $(...)

[Peter Stephenson]

oncurrene of 43
On Thu, 26 Feb 2009 22:57:43 +0000
Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> It's not quite that simple, since "#" is only a comment character
> at the start of a word.  The following is better, I hope I haven't
> missed any cases...

I should have tested for whitespace including a possible newline, not
separators, before the "#" for this level of parsing, and I should have
marked that a previous comment line ended in whitespace.

Index: Src/lex.c
===================================================================
RCS file: /cvsroot/zsh/zsh/Src/lex.c,v
retrieving revision 1.50
diff -u -r1.50 lex.c
--- Src/lex.c	26 Feb 2009 23:01:49 -0000	1.50
+++ Src/lex.c	27 Feb 2009 10:17:51 -0000
@@ -1813,7 +1813,7 @@
 	c = hgetc();
 	if (itok(c) || lexstop)
 	    break;
-	iswhite = isep(c);
+	iswhite = inblank(c);
 	switch (c) {
 	case '(':
 	    pct++;
@@ -1861,6 +1861,7 @@
 		add(c);
 		while ((c = hgetc()) != '\n' && !lexstop)
 		    add(c);
+		iswhite = 1;
 	    }
 	    break;
 	}
Index: Test/A01grammar.ztst
===================================================================
RCS file: /cvsroot/zsh/zsh/Test/A01grammar.ztst,v
retrieving revision 1.17
diff -u -r1.17 A01grammar.ztst
--- Test/A01grammar.ztst	11 May 2008 19:03:58 -0000	1.17
+++ Test/A01grammar.ztst	27 Feb 2009 10:17:51 -0000
@@ -503,3 +503,15 @@
   $ZTST_testdir/../Src/zsh -f NonExistentScript
 127q:Non-existent script causes exit status 127
 ?$ZTST_testdir/../Src/zsh: can't open input file: NonExistentScript
+
+  (setopt nonomatch
+  # use this to get stuff at start of line
+  contents=$'# comment \'\necho value #with " stuff\n# comment\n#comment
+  echo not#comment\n'
+  eval 'VAR=$('"$contents"')'
+  print -l $VAR)
+0:comments within $(...)
+>value
+>not#comment
+
+  

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

Re: PATCH: Error parsing $(...)

[Bart Schaefer]

This doesn't work properly with respect to NO_INTERACTIVECOMMENTS:

schaefer<501> print $(echo XX
echo this # is not a comment
echo YY)
XX this YY
schaefer<502> allopt interactive
interactive           on
interactivecomments   off

Re: PATCH: Error parsing $(...)

[Peter Stephenson]

On Fri, 27 Feb 2009 08:33:34 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> This doesn't work properly with respect to NO_INTERACTIVECOMMENTS:
> 
> schaefer<501> print $(echo XX
> echo this # is not a comment
> echo YY)
> XX this YY
> schaefer<502> allopt interactive
> interactive           on
> interactivecomments   off

That behaviour hasn't changed, it's always worked that way.  The bug was
only that stuff after the '#' wasn't properly ignored.

It's never been well defined whether the text of a "$(...)" was
"interactive" in the sense meant.  The bit that happens interactively is
reading from "$(" until ")" is hit, during which the lexical analsyer isn't
looking for comments because it know the "end of the current interactive
line" (again, in some not very well defined sense) is after the ")".  After
that the string is stored internally and evaluated later.  Anyway, it's
probably too late to do it the other way now, but if someone wants to write
a patch to clarify it they're welcome.

-- 
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