${(z)} parsing of multiple array assignments

${(z)} parsing of multiple array assignments

[Daniel Shahaf]

In the following two cases, why are the assignments to $b
parsed differently to the assignments to $a?

    % pz() { print -rl -- ${(qqqq)${(z)1}} }

    % pz 'a=() b=()' 
    $'a=('
    $')'
    $'b='
    $'()'

    % pz 'a=(foo) b=(bar)' 
    $'a=('
    $'foo'
    $')'
    $'b=(bar)'

Thanks,

Daniel

Re: ${(z)} parsing of multiple array assignments

[Phil Pennock]

On 2019-12-23 at 17:31 +0000, Daniel Shahaf wrote:
> In the following two cases, why are the assignments to $b
> parsed differently to the assignments to $a?
> 
>     % pz() { print -rl -- ${(qqqq)${(z)1}} }

Looks to be a bug, a lack of state reset.  In two places, perhaps: you
can see that one example resets when you alternate back away from having
empty assignment lists, where the b/c pairing here is reset by `d`, so
that the f/g pairing matches.  But I can't trigger a reset to the
initial parse state used for `a`; unless and until I introduce a
newline, where in the second example you can see the same parse used for
`e`:

    % echo $ZSH_VERSION
    5.7.1

    % x='a=(foo) b=() c=() d=(bar) e=(baz) f=() g=()'
    % print -rl -- "${(z)x}"
    a=(
    foo
    )
    b=
    ()
    c=(
    )
    d=(bar)
    e=(baz)
    f=
    ()
    g=(
    )

    ## differs in $'...' not '...' and \n between d and e:

    % x=$'a=(foo) b=() c=() d=(bar)\ne=(baz) f=() g=()'
    % print -rl -- "${(z)x}"
    a=(
    foo
    )
    b=
    ()
    c=(
    )
    d=(bar)
    ;
    e=(
    baz
    )
    f=
    ()
    g=(
    )

Re: ${(z)} parsing of multiple array assignments

[Sebastian Gniazdowski]

Looks like an empty assignment does reset the state for a single next one:

% x='a=(foo) b=() d=(bar)'
% print -rl -- "${(z)x}"
a=(
foo
)
b=
()
d=(
bar
)

On Tue, 24 Dec 2019 at 21:35, Phil Pennock
<zsh-workers+phil.pennock@spodhuis.org> wrote:
>
> On 2019-12-23 at 17:31 +0000, Daniel Shahaf wrote:
> > In the following two cases, why are the assignments to $b
> > parsed differently to the assignments to $a?
> >
> >     % pz() { print -rl -- ${(qqqq)${(z)1}} }
>
> Looks to be a bug, a lack of state reset.  In two places, perhaps: you
> can see that one example resets when you alternate back away from having
> empty assignment lists, where the b/c pairing here is reset by `d`, so
> that the f/g pairing matches.  But I can't trigger a reset to the
> initial parse state used for `a`; unless and until I introduce a
> newline, where in the second example you can see the same parse used for
> `e`:
>
>     % echo $ZSH_VERSION
>     5.7.1
>
>     % x='a=(foo) b=() c=() d=(bar) e=(baz) f=() g=()'
>     % print -rl -- "${(z)x}"
>     a=(
>     foo
>     )
>     b=
>     ()
>     c=(
>     )
>     d=(bar)
>     e=(baz)
>     f=
>     ()
>     g=(
>     )
>
>     ## differs in $'...' not '...' and \n between d and e:
>
>     % x=$'a=(foo) b=() c=() d=(bar)\ne=(baz) f=() g=()'
>     % print -rl -- "${(z)x}"
>     a=(
>     foo
>     )
>     b=
>     ()
>     c=(
>     )
>     d=(bar)
>     ;
>     e=(
>     baz
>     )
>     f=
>     ()
>     g=(
>     )
>


-- 
Sebastian Gniazdowski
News: https://twitter.com/ZdharmaI
IRC: https://kiwiirc.com/client/chat.freenode.net:+6697/#zplugin
Blog: http://zdharma.org

Re: ${(z)} parsing of multiple array assignments

[Peter Stephenson]

On Mon, 2019-12-23 at 17:31 +0000, Daniel Shahaf wrote:
> In the following two cases, why are the assignments to $b
> parsed differently to the assignments to $a?
> 
>     % pz() { print -rl -- ${(qqqq)${(z)1}} }
> 
>     % pz 'a=() b=()' 
>     $'a=('
>     $')'
>     $'b='
>     $'()'
> 
>     % pz 'a=(foo) b=(bar)' 
>     $'a=('
>     $'foo'
>     $')'
>     $'b=(bar)'

Unless I'm missing some trick, bufferwords() is the function where we
need to update any parser state --- ctxtlex() is too low level for that,
it just handles the next token given the current state.  So it's
probably something like this.  Will need a new test adding.

Presumably the commenting out of ENVSTRING in ctxtlex() means we stay in
incmdpos in that case --- as this is just a single word assignment that
probably handles that case OK.

pws

diff --git a/Src/hist.c b/Src/hist.c
index 74116e82f..37cba4579 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -3321,6 +3321,7 @@ bufferwords(LinkList list, char *buf, int *index, int flags)
     int owb = wb, owe = we, oadx = addedx, onc = nocomments;
     int ona = noaliases, ocs = zlemetacs, oll = zlemetall;
     int forloop = 0, rcquotes = opts[RCQUOTES];
+    int envarray = 0;
     char *p, *addedspaceptr;
 
     if (!list)
@@ -3404,6 +3405,14 @@ bufferwords(LinkList list, char *buf, int *index, int flags)
 	ctxtlex();
 	if (tok == ENDINPUT || tok == LEXERR)
 	    break;
+	/*
+	 * After an array assignment, return to the initial
+	 * start-of-command state.  There could be a second ENVARRAY.
+	 */
+	if (tok == OUTPAR && envarray) {
+	    incmdpos = 1;
+	    envarray = 0;
+	}
 	if (tok == FOR) {
 	    /*
 	     * The way for (( expr1 ; expr2; expr3 )) is parsed is:
@@ -3441,6 +3450,7 @@ bufferwords(LinkList list, char *buf, int *index, int flags)
 	    switch (tok) {
 	    case ENVARRAY:
 		p = dyncat(tokstr, "=(");
+		envarray = 1;
 		break;
 
 	    case DINPAR:

Re: ${(z)} parsing of multiple array assignments

[Peter Stephenson]

On Sun, 2019-12-29 at 20:55 +0000, Peter Stephenson wrote:
> Unless I'm missing some trick, bufferwords() is the function where we
> need to update any parser state --- ctxtlex() is too low level for that,
> it just handles the next token given the current state.  So it's
> probably something like this.  Will need a new test adding.

We also need to do something similar for completion, otherwise

  foo=(stuff) bar=(...

looks like a glob qualifier.

In this case it's already half handled, so there's a variable to hook
into.

pws


diff --git a/Src/Zle/zle_tricky.c b/Src/Zle/zle_tricky.c
index 2b25d6b2e..fdd168763 100644
--- a/Src/Zle/zle_tricky.c
+++ b/Src/Zle/zle_tricky.c
@@ -1236,8 +1236,10 @@ get_comp_string(void)
 	else if (tok == OUTPAR) {
 	    if (parct)
 		parct--;
-	    else
+	    else if (linarr) {
 		linarr = 0;
+		incmdpos = 1;
+	    }
 	}
 	if (inredir && IS_REDIROP(tok)) {
             rdstr = rdstrbuf;
diff --git a/Src/hist.c b/Src/hist.c
index 74116e82f..5281e8718 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -3321,6 +3321,7 @@ bufferwords(LinkList list, char *buf, int *index, int flags)
     int owb = wb, owe = we, oadx = addedx, onc = nocomments;
     int ona = noaliases, ocs = zlemetacs, oll = zlemetall;
     int forloop = 0, rcquotes = opts[RCQUOTES];
+    int envarray = 0;
     char *p, *addedspaceptr;
 
     if (!list)
@@ -3404,6 +3405,14 @@ bufferwords(LinkList list, char *buf, int *index, int flags)
 	ctxtlex();
 	if (tok == ENDINPUT || tok == LEXERR)
 	    break;
+	/*
+	 * After an array assignment, return to the initial
+	 * start-of-command state.  There could be a second ENVARRAY.
+	 */
+	if (tok == OUTPAR && envarray) {
+	    incmdpos = 1;
+	    envarray = 0;
+	}
 	if (tok == FOR) {
 	    /*
 	     * The way for (( expr1 ; expr2; expr3 )) is parsed is:
@@ -3441,6 +3450,7 @@ bufferwords(LinkList list, char *buf, int *index, int flags)
 	    switch (tok) {
 	    case ENVARRAY:
 		p = dyncat(tokstr, "=(");
+		envarray = 1;
 		break;
 
 	    case DINPAR:

Re: ${(z)} parsing of multiple array assignments

[Daniel Shahaf]

Peter Stephenson wrote on Sun, Dec 29, 2019 at 23:45:00 +0000:
> On Sun, 2019-12-29 at 20:55 +0000, Peter Stephenson wrote:
> > Unless I'm missing some trick, bufferwords() is the function where we
> > need to update any parser state --- ctxtlex() is too low level for that,
> > it just handles the next token given the current state.  So it's
> > probably something like this.  Will need a new test adding.
> 
> We also need to do something similar for completion, otherwise
> 
>   foo=(stuff) bar=(...
> 
> looks like a glob qualifier.
> 
> In this case it's already half handled, so there's a variable to hook
> into.

Thanks for the fix; it didn't occur to me to change bufferwords().

Here's a test.

diff --git a/Test/D04parameter.ztst b/Test/D04parameter.ztst
index c91af1a9c..899b8f498 100644
--- a/Test/D04parameter.ztst
+++ b/Test/D04parameter.ztst
@@ -640,6 +640,74 @@
 >echo
 >$(|||) bar
 
+ argv=(
+  $'a=() b=()'
+  $'a=(foo) b=(bar)'
+  $'a=(foo) b=() c=() d=(bar) e=(baz) f=() g=()'
+  $'a=(foo) b=() c=() d=(bar)\ne=(baz) f=() g=()'
+  $'a=(foo) b=() d=(bar)'
+ )
+ for 1; print -rl -- ${(z)1} && print
+0:${(z)} regression test: multiple array assignments
+>a=(
+>)
+>b=(
+>)
+>
+>a=(
+>foo
+>)
+>b=(
+>bar
+>)
+>
+>a=(
+>foo
+>)
+>b=(
+>)
+>c=(
+>)
+>d=(
+>bar
+>)
+>e=(
+>baz
+>)
+>f=(
+>)
+>g=(
+>)
+>
+>a=(
+>foo
+>)
+>b=(
+>)
+>c=(
+>)
+>d=(
+>bar
+>)
+>;
+>e=(
+>baz
+>)
+>f=(
+>)
+>g=(
+>)
+>
+>a=(
+>foo
+>)
+>b=(
+>)
+>d=(
+>bar
+>)
+>
+
   foo=$'\x06ZUI\x1f text-field example: \x1azuitfieldtfield1_1\x1a\'\'\x1a\'\'\x1a1\x1aZUI\\[my_tfield1_width\\]\x1aZUI\\[my_tfield1_start\\]\x1aZUI\\[my_tfield1_data\\]\x1c'
   print "${#${(z@)foo}}"
 0:Test real-world data that once seemed to fail

Cheers,

Daniel

Re: ${(z)} parsing of multiple array assignments

[Daniel Shahaf]

Daniel Shahaf wrote on Mon, Dec 30, 2019 at 18:13:39 +0000:
> Peter Stephenson wrote on Sun, Dec 29, 2019 at 23:45:00 +0000:
> > On Sun, 2019-12-29 at 20:55 +0000, Peter Stephenson wrote:
> > > Unless I'm missing some trick, bufferwords() is the function where we
> > > need to update any parser state --- ctxtlex() is too low level for that,
> > > it just handles the next token given the current state.  So it's
> > > probably something like this.  Will need a new test adding.
> > 
> > We also need to do something similar for completion, otherwise
> > 
> >   foo=(stuff) bar=(...
> > 
> > looks like a glob qualifier.
> > 
> > In this case it's already half handled, so there's a variable to hook
> > into.
> 
> Thanks for the fix; it didn't occur to me to change bufferwords().
> 
> Here's a test.

One more:

diff --git a/Test/Y01completion.ztst b/Test/Y01completion.ztst
index 7ef319f7b..51f604bcf 100644
--- a/Test/Y01completion.ztst
+++ b/Test/Y01completion.ztst
@@ -216,6 +216,15 @@ F:regression test workers/31611
 >NO:{3pm}
 >NO:{10pm}
 
+ comptest $'a=() b=(\t'
+0:multiple envarrays
+>line: {a=() b=(}{}
+>DESCRIPTION:{file}
+>DI:{dir1}
+>DI:{dir2}
+>FI:{file1}
+>FI:{file2}
+
 %clean
 
   zmodload -ui zsh/zpty

Re: ${(z)} parsing of multiple array assignments

[Peter Stephenson]

On Mon, 2019-12-30 at 18:21 +0000, Daniel Shahaf wrote:
> Daniel Shahaf wrote on Mon, Dec 30, 2019 at 18:13:39 +0000:
> > Here's a test.
> 
> One more:

Thanks; shall I commit my change?  It looks like it ought to be local
enough we can be reasonably confident it's well tested.

pws

Re: ${(z)} parsing of multiple array assignments

[Daniel Shahaf]

Peter Stephenson wrote on Mon, 30 Dec 2019 22:37 +00:00:
> On Mon, 2019-12-30 at 18:21 +0000, Daniel Shahaf wrote:
> > Daniel Shahaf wrote on Mon, Dec 30, 2019 at 18:13:39 +0000:
> > > Here's a test.
> > 
> > One more:
> 
> Thanks; shall I commit my change?  It looks like it ought to be local
> enough we can be reasonably confident it's well tested.

The changes do seem fairly local, and not likely to break anything that
_doesn't_ use array assignments interactively (get_comp_string()) or in
${(z)} (bufferwords()), so small risk.  I don't have a gut feeling for how likely
they are to break anything, but if you feel they're not going to necessitate
a 5.8.1, then sure, go ahead and commit to master.

If you aren't sure, then go ahead and commit to the 5.9 branch. :)

Re: ${(z)} parsing of multiple array assignments

[Daniel Shahaf]

Daniel Shahaf wrote on Mon, 30 Dec 2019 23:46 +00:00:
> Peter Stephenson wrote on Mon, 30 Dec 2019 22:37 +00:00:
> > On Mon, 2019-12-30 at 18:21 +0000, Daniel Shahaf wrote:
> > > Daniel Shahaf wrote on Mon, Dec 30, 2019 at 18:13:39 +0000:
> > > > Here's a test.
> > > 
> > > One more:
> > 
> > Thanks; shall I commit my change?  It looks like it ought to be local
> > enough we can be reasonably confident it's well tested.
> 
> The changes do seem fairly local, and not likely to break anything
> that _doesn't_ use array assignments interactively (get_comp_string())
> or in ${(z)} (bufferwords()), so small risk.  I don't have a gut
> feeling for how likely they are to break anything, but if you feel
> they're not going to necessitate a 5.8.1, then sure, go ahead and
> commit to master.
> 
> If you aren't sure, then go ahead and commit to the 5.9 branch. :)

I suppose that was too teflony.  What I was trying to say is that
I don't know the two functions in question well enough to have a gut
feeling about how destabilizing/risky a particular change is, but in
general, if I had doubts I would err on the side of caution and defer to
the next release.

Cheers,

Daniel

Re: ${(z)} parsing of multiple array assignments

[Peter Stephenson]

On Mon, 2019-12-30 at 23:46 +0000, Daniel Shahaf wrote:
> Peter Stephenson wrote on Mon, 30 Dec 2019 22:37 +00:00:
> > On Mon, 2019-12-30 at 18:21 +0000, Daniel Shahaf wrote:
> > > Daniel Shahaf wrote on Mon, Dec 30, 2019 at 18:13:39 +0000:
> > > > Here's a test.
> > > 
> > > One more:
> > 
> > Thanks; shall I commit my change?  It looks like it ought to be local
> > enough we can be reasonably confident it's well tested.
> 
> The changes do seem fairly local, and not likely to break anything that
> _doesn't_ use array assignments interactively (get_comp_string()) or in
> ${(z)} (bufferwords()), so small risk.  I don't have a gut feeling for how likely
> they are to break anything, but if you feel they're not going to necessitate
> a 5.8.1, then sure, go ahead and commit to master.
> 
> If you aren't sure, then go ahead and commit to the 5.9 branch. :)

I don't see any problem myself, but it was best to check.  I've
committed it.

pws