[PATCH] typeset: set $? on incidental error

[PATCH] typeset: set $? on incidental error

[Daniel Shahaf]

The 'typeset' family of builtins doesn't set $? when one would expect it
to do so:

    % x=$(true) y=$(exit 42); echo $?
    42
    % local x=$(true) y=$(exit 42); echo $?
    0

This patch makes 'typeset' behave as ordiary assignment does.

It's a one-liner but still, I'd appreciate a second pair of eyes over it.

Cheers,

Daniel

diff --git a/Src/builtin.c b/Src/builtin.c
index d8974eb..c4af66f 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -475,9 +475,13 @@ execbuiltin(LinkList args, LinkList assigns, Builtin bn)
 	{
 	    /*
 	     * Takes two sets of arguments.
+	     *
+	     * Return assignfunc's return value, or if it succeeded return any
+	     * preexisting error.
 	     */
 	    HandlerFuncAssign assignfunc = (HandlerFuncAssign)bn->handlerfunc;
-	    return (*(assignfunc)) (name, argv, assigns, &ops, bn->funcid);
+	    int retval = (*(assignfunc)) (name, argv, assigns, &ops, bn->funcid);
+	    return retval ? retval : lastval;
 	}
 	else
 	{
diff --git a/Test/B02typeset.ztst b/Test/B02typeset.ztst
index 7d65cc8..a87ecfb 100644
--- a/Test/B02typeset.ztst
+++ b/Test/B02typeset.ztst
@@ -706,3 +706,13 @@
 >typeset -a array=( '' two '' four )
 >typeset -a array=( one '' three )
 >no really nothing here
+
+  s='local x=$(exit 42); echo $?'
+  eval $s
+  (
+    disable -r local
+    eval $s
+  )
+0:typeset reports errors like parameter assignment does
+>42
+>42

Re: [PATCH] typeset: set $? on incidental error

[Bart Schaefer]

On Jan 14, 12:13am, Daniel Shahaf wrote:
}
} The 'typeset' family of builtins doesn't set $? when one would expect it
} 
} This patch makes 'typeset' behave as ordiary assignment does.

Looks OK to me.  I worry that there's also some examination of errflag
needed, but what you have there doesn't seem to be wrong.

Re: [PATCH] typeset: set $? on incidental error

[Eric Cook]

On 01/13/2016 07:13 PM, Daniel Shahaf wrote:
> The 'typeset' family of builtins doesn't set $? when one would expect it
> to do so:
> 
>     % x=$(true) y=$(exit 42); echo $?
>     42
>     % local x=$(true) y=$(exit 42); echo $?
>     0
> 
> This patch makes 'typeset' behave as ordiary assignment does.


But who expects that? no other shell with a typeset behaves that way.

Re: [PATCH] typeset: set $? on incidental error

[Daniel Shahaf]

Eric Cook wrote on Thu, Jan 14, 2016 at 00:24:36 -0500:
> On 01/13/2016 07:13 PM, Daniel Shahaf wrote:
> > The 'typeset' family of builtins doesn't set $? when one would expect it
> > to do so:
> > 
> >     % x=$(true) y=$(exit 42); echo $?
> >     42
> >     % local x=$(true) y=$(exit 42); echo $?
> >     0
> > 
> > This patch makes 'typeset' behave as ordiary assignment does.
> 
> 
> But who expects that?

I did.  I ran into this in practice:
.
     https://github.com/zsh-users/zsh-syntax-highlighting/blob/28932316cca6004253ec57f5d87981c72be64e86/tests/test-highlighting.zsh#L118-L121
     118	run_test() {
     119	  # Do not combine the declaration and initialization: «local x="$(false)"» does not set $?.
     120	  local __tests_tempdir
     121	  __tests_tempdir="$(mktemp -d)" && [[ -d $__tests_tempdir ]] || {
.
Having such comments in source code suggests the language semantics are
surprising / non-obvious.

This was also reported to debian years ago (http://bugs.debian.org/479841).

> no other shell with a typeset behaves that way.

So if this change is made, zsh users who switch to other shells might be
surprised that those other shells don't set $? of typeset.  On the other
hand, if we agree that «typeset x=$(exit 42)» should set $?, then
perhaps zsh can be the first to implement that and other shells will
follow.

Somebody has to be the first shell to change behaviour, after all.

Cheers,

Daniel

Re: [PATCH] typeset: set $? on incidental error

[Mikael Magnusson]

On Fri, Jan 15, 2016 at 7:26 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Eric Cook wrote on Thu, Jan 14, 2016 at 00:24:36 -0500:
>> On 01/13/2016 07:13 PM, Daniel Shahaf wrote:
>> > The 'typeset' family of builtins doesn't set $? when one would expect it
>> > to do so:
>> >
>> >     % x=$(true) y=$(exit 42); echo $?
>> >     42
>> >     % local x=$(true) y=$(exit 42); echo $?
>> >     0
>> >
>> > This patch makes 'typeset' behave as ordiary assignment does.
>>
>>
>> But who expects that?
>
> I did.

local/typeset is a command, and it was successful, so I don't see why
$? should be set to anything else than 0.
% true x=$(false); echo $?
0
is technically exactly the same situation as your above second command.

-- 
Mikael Magnusson

Re: [PATCH] typeset: set $? on incidental error

[Eric Cook]

On 01/15/2016 09:46 AM, Mikael Magnusson wrote:
> On Fri, Jan 15, 2016 at 7:26 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>> Eric Cook wrote on Thu, Jan 14, 2016 at 00:24:36 -0500:
>>> On 01/13/2016 07:13 PM, Daniel Shahaf wrote:
>>>> The 'typeset' family of builtins doesn't set $? when one would expect it
>>>> to do so:
>>>>
>>>>     % x=$(true) y=$(exit 42); echo $?
>>>>     42
>>>>     % local x=$(true) y=$(exit 42); echo $?
>>>>     0
>>>>
>>>> This patch makes 'typeset' behave as ordiary assignment does.
>>>
>>>
>>> But who expects that?
>>
>> I did.
> 
> local/typeset is a command, and it was successful, so I don't see why
> $? should be set to anything else than 0.
> % true x=$(false); echo $?
> 0
> is technically exactly the same situation as your above second command.
> 

But typeset should exit > 0 when it does fail to assign a parameter.

% ksh -c 'typeset -r foo=foo; typeset -r foo=bar'; echo $?
ksh: foo: is read only
1
% bash -c 'typeset -r foo=foo; typeset -r foo=bar'; echo $?
bash: line 0: typeset: foo: readonly variable
1
% zsh -c 'typeset -r foo=foo; typeset -r foo=bar'; echo $?
zsh:1: read-only variable: foo
0

Re: [PATCH] typeset: set $? on incidental error

[Peter Stephenson]

On Fri, 15 Jan 2016 09:54:37 -0500
Eric Cook <llua@gmx.com> wrote:
> But typeset should exit > 0 when it does fail to assign a parameter.

This looks about the minimal fix for this.

This is already a hard error, so will cause other stuff later in the
command to fail, however the error wasn't propagated properly within the
command handler.

pws

diff --git a/Src/builtin.c b/Src/builtin.c
index e1a7990..dd20f9e 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2213,6 +2213,8 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
 				 mkarray(NULL), 0)))
 		return NULL;
 	}
+	if (errflag)
+	    return NULL;
 	pm->node.flags |= (on & PM_READONLY);
 	if (OPT_ISSET(ops,'p'))
 	    paramtab->printnode(&pm->node, PRINT_TYPESET);
diff --git a/Test/B02typeset.ztst b/Test/B02typeset.ztst
index 681fe73..d6d2421 100644
--- a/Test/B02typeset.ztst
+++ b/Test/B02typeset.ztst
@@ -706,3 +706,8 @@
 >typeset -a array=( '' two '' four )
 >typeset -a array=( one '' three )
 >no really nothing here
+
+  readonly isreadonly=yes
+  typeset isreadonly=still
+1:typeset returns status 1 if setting readonly variable
+?(eval):2: read-only variable: isreadonly

Re: [PATCH] typeset: set $? on incidental error

[Mikael Magnusson]

On Mon, Jan 18, 2016 at 3:25 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Mikael Magnusson wrote on Fri, Jan 15, 2016 at 15:46:09 +0100:
>> On Fri, Jan 15, 2016 at 7:26 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>> > Eric Cook wrote on Thu, Jan 14, 2016 at 00:24:36 -0500:
>> >> On 01/13/2016 07:13 PM, Daniel Shahaf wrote:
>> >> > The 'typeset' family of builtins doesn't set $? when one would expect it
>> >> > to do so:
>> >> >
>> >> >     % x=$(true) y=$(exit 42); echo $?
>> >> >     42
>> >> >     % local x=$(true) y=$(exit 42); echo $?
>> >> >     0
>> >> >
>> >> > This patch makes 'typeset' behave as ordiary assignment does.
>> >>
>> >>
>> >> But who expects that?
>> >
>> > I did.
>>
>> local/typeset is a command, and it was successful, so I don't see why
>> $? should be set to anything else than 0.
>> % true x=$(false); echo $?
>> 0
>> is technically exactly the same situation as your above second command.
>
> I wouldn't call it successful: I asked for the parameter x to be created
> as a scalar and assigned a value and only part of my request was
> accomplished.

That's not true, the parameter is created and assigned the value you
asked for (your command subst had empty output so the parameter is ""
but if you did local y=$(echo hi; exit 42) it would be "42".)

-- 
Mikael Magnusson

Re: [PATCH] typeset: set $? on incidental error

[Mikael Magnusson]

On Mon, Jan 18, 2016 at 5:38 AM, Mikael Magnusson <mikachu@gmail.com> wrote:
> On Mon, Jan 18, 2016 at 3:25 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>> Mikael Magnusson wrote on Fri, Jan 15, 2016 at 15:46:09 +0100:
>>> On Fri, Jan 15, 2016 at 7:26 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>>> > Eric Cook wrote on Thu, Jan 14, 2016 at 00:24:36 -0500:
>>> >> On 01/13/2016 07:13 PM, Daniel Shahaf wrote:
>>> >> > The 'typeset' family of builtins doesn't set $? when one would expect it
>>> >> > to do so:
>>> >> >
>>> >> >     % x=$(true) y=$(exit 42); echo $?
>>> >> >     42
>>> >> >     % local x=$(true) y=$(exit 42); echo $?
>>> >> >     0
>>> >> >
>>> >> > This patch makes 'typeset' behave as ordiary assignment does.
>>> >>
>>> >>
>>> >> But who expects that?
>>> >
>>> > I did.
>>>
>>> local/typeset is a command, and it was successful, so I don't see why
>>> $? should be set to anything else than 0.
>>> % true x=$(false); echo $?
>>> 0
>>> is technically exactly the same situation as your above second command.
>>
>> I wouldn't call it successful: I asked for the parameter x to be created
>> as a scalar and assigned a value and only part of my request was
>> accomplished.
>
> That's not true, the parameter is created and assigned the value you
> asked for (your command subst had empty output so the parameter is ""
> but if you did local y=$(echo hi; exit 42) it would be "42".)

It would be "hi" rather, of course.

-- 
Mikael Magnusson

Re: [PATCH] typeset: set $? on incidental error

[Daniel Shahaf]

Mikael Magnusson wrote on Mon, Jan 18, 2016 at 05:38:17 +0100:
> On Mon, Jan 18, 2016 at 3:25 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Mikael Magnusson wrote on Fri, Jan 15, 2016 at 15:46:09 +0100:
> >> On Fri, Jan 15, 2016 at 7:26 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >> > Eric Cook wrote on Thu, Jan 14, 2016 at 00:24:36 -0500:
> >> >> On 01/13/2016 07:13 PM, Daniel Shahaf wrote:
> >> >> > The 'typeset' family of builtins doesn't set $? when one would expect it
> >> >> > to do so:
> >> >> >
> >> >> >     % x=$(true) y=$(exit 42); echo $?
> >> >> >     42
> >> >> >     % local x=$(true) y=$(exit 42); echo $?
> >> >> >     0
> >> >> >
> >> >> > This patch makes 'typeset' behave as ordiary assignment does.
> >> >>
> >> >>
> >> >> But who expects that?
> >> >
> >> > I did.
> >>
> >> local/typeset is a command, and it was successful, so I don't see why
> >> $? should be set to anything else than 0.
> >> % true x=$(false); echo $?
> >> 0
> >> is technically exactly the same situation as your above second command.
> >
> > I wouldn't call it successful: I asked for the parameter x to be created
> > as a scalar and assigned a value and only part of my request was
> > accomplished.
> 
> That's not true, the parameter is created and assigned the value you
> asked for (your command subst had empty output so the parameter is ""
> but if you did local y=$(echo hi; exit 42) it would be "hi".)

In real-life examples, if the command subst had a non-zero exit status,
the value would typically _not_ be the one I asked for:

    % cd $(mktemp -d)
    % local mtime=$(zstat +mtime myfile)
    zstat: myfile: no such file or directory
    % echo ${(q)mtime} $? 
    '' 1

Re: [PATCH] typeset: set $? on incidental error

[Eric Cook]

On 01/20/2016 02:47 AM, Daniel Shahaf wrote:
> Mikael Magnusson wrote on Mon, Jan 18, 2016 at 05:38:17 +0100:
>> On Mon, Jan 18, 2016 at 3:25 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>>> Mikael Magnusson wrote on Fri, Jan 15, 2016 at 15:46:09 +0100:
>>>> On Fri, Jan 15, 2016 at 7:26 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>>>>> Eric Cook wrote on Thu, Jan 14, 2016 at 00:24:36 -0500:
>>>>>> On 01/13/2016 07:13 PM, Daniel Shahaf wrote:
>>>>>>> The 'typeset' family of builtins doesn't set $? when one would expect it
>>>>>>> to do so:
>>>>>>>
>>>>>>>     % x=$(true) y=$(exit 42); echo $?
>>>>>>>     42
>>>>>>>     % local x=$(true) y=$(exit 42); echo $?
>>>>>>>     0
>>>>>>>
>>>>>>> This patch makes 'typeset' behave as ordiary assignment does.
>>>>>>
>>>>>>
>>>>>> But who expects that?
>>>>>
>>>>> I did.
>>>>
>>>> local/typeset is a command, and it was successful, so I don't see why
>>>> $? should be set to anything else than 0.
>>>> % true x=$(false); echo $?
>>>> 0
>>>> is technically exactly the same situation as your above second command.
>>>
>>> I wouldn't call it successful: I asked for the parameter x to be created
>>> as a scalar and assigned a value and only part of my request was
>>> accomplished.
>>
>> That's not true, the parameter is created and assigned the value you
>> asked for (your command subst had empty output so the parameter is ""
>> but if you did local y=$(echo hi; exit 42) it would be "hi".)
> 
> In real-life examples, if the command subst had a non-zero exit status,
> the value would typically _not_ be the one I asked for:
> 
>     % cd $(mktemp -d)
>     % local mtime=$(zstat +mtime myfile)
>     zstat: myfile: no such file or directory
>     % echo ${(q)mtime} $? 
>     '' 1
> 
But local is still the last command to run and local successfully created
the parameter mtime. The commands that may possibly execute during the
parsing of the line, affecting the actual command's exit status is a weird
side effect that doesn't happen elsewhere.

The change is only useful if mtime is the last argument for local.
x=$(exit 42) y=$(true); echo $?
0

So in the event that you actually care about the exit status of a command
during assignment, explicitly testing a normal assignment after making the
parameter local makes sense.

local mtime
if mtime=$(zstat +mtime myfile); then ...

The last command to run in the conditional is actually zstat, so $? changing
is logically there.

Re: [PATCH] typeset: set $? on incidental error

[Daniel Shahaf]

Between all the replies I'm convinced that the builtin interface of
'local' shouldn't be changed: «builtin local x=$(false)» should set $?
to 0.  (I meant to say that in a previous email.)

However, the question remains, in my eyes, whether the change should be
made to the reserved word interface.  The reserved word 'typeset' is not
a command, but part of the shell's language, so in
.
    typeset x=$(foo) y=$(bar)
.
the last command executed is 'bar'.  I would expect that line to behave
as similarly to
.
    x=$(foo) y=$(bar)
.
as possible, so for example, I'd expect the former to set $? to the exit
code of 'bar', as the latter statement does.

Cheers,

Daniel

Re: [PATCH] typeset: set $? on incidental error

[Eric Cook]

On 01/23/2016 06:53 PM, Daniel Shahaf wrote:
> Between all the replies I'm convinced that the builtin interface of
> 'local' shouldn't be changed: «builtin local x=$(false)» should set $?
> to 0.  (I meant to say that in a previous email.)
> 
> However, the question remains, in my eyes, whether the change should be
> made to the reserved word interface.  The reserved word 'typeset' is not
> a command, but part of the shell's language, so in
> .
>     typeset x=$(foo) y=$(bar)
> .
> the last command executed is 'bar'.  I would expect that line to behave
> as similarly to
> .
>     x=$(foo) y=$(bar)
> .
> as possible, so for example, I'd expect the former to set $? to the exit
> code of 'bar', as the latter statement does.
> 
> Cheers,
> 
> Daniel
> 

1(again): the other shells with typeset don't behave that way,
the chances of those shells seeing this a good idea and implementing it
are pretty slim in my opinion.

typeset originated in ksh and ksh93's development has pretty much stalled
over the past few years. When not following posix sh, bash tends to align with
the way ksh does things. needless to say that the other kshs tend to do the same.

Most likely resulting in one more way our typeset differs. zsh's typeset became
a reserved word recently in an attempt behave more like the other typesets.

2: typeset while now part of the `language', still retains it's own exit statuses
from when it was a builtin command.
''
typeset -T foo=bar baz=qux; echo $?; typeset -p foo baz
typeset: second argument of tie must be array: baz
1
typeset: no such variable: foo
typeset: no such variable: baz
''

So in the event that typeset _and_ your arbitrary commands errors, what should $?
be set to?
''
typeset -Z 1.1 foo=$(exit 42)
echo $? # 1 or 42?
''

3: in the `real world' if you are assigning a parameter via command substitution
and there is a possibility that the command substitution could return nothing.
You would get the parameter a default value. During assignment or during expansion.
''
typeset foo=${"$(exit 42)":-default}
# or
echo ${foo:=default} # or :-
''

Re: [PATCH] typeset: set $? on incidental error

[Daniel Shahaf]

Eric Cook wrote on Sat, Jan 23, 2016 at 22:32:48 -0500:
> 2: typeset while now part of the `language', still retains it's own exit statuses
> from when it was a builtin command.
...
> So in the event that typeset _and_ your arbitrary commands errors, what should $?
> be set to?

$? would reflect the failure of the typeset, which would shadow the
failure of the command substitution.

What would you expect «repeat 0+++ (exit 42)» to set $? to?

       repeat WORD do LIST done
              WORD is expanded and treated as an arithmetic expression, which
              must evaluate to a number N.  LIST is then executed N times.

       repeat WORD SUBLIST
              This is a short form of repeat.

    % : $(( 0+++ ))
    zsh: bad math expression: lvalue required

> 3: in the `real world' if you are assigning a parameter via command substitution
> and there is a possibility that the command substitution could return nothing.
> You would get the parameter a default value. During assignment or during expansion.
> ''
> typeset foo=${"$(exit 42)":-default}
> # or
> echo ${foo:=default} # or :-
> ''

"stdout is empty" is less sensitive a test than "the exit code is
non-zero": for example,
.
    zcat =(gzip <<<foo; gzip <<<bar | perl -pe 's/\x08/\xff/')
.
is caught by the latter but not by the former.

> 1(again): the other shells with typeset don't behave that way,

If existing typesets always exit zero in the circumstance that the patch
proposes to change, no code would be broken by this change (no existing
code, no new zsh-targeted code, no new ksh-targeted code), except for
new ksh-targeted code written by zsh users who won't be aware of this
difference.

There is another such difference: SH_WORD_SPLIT.

So, playing devil's advocate, we could apply the patch, provided that we
document the resulting difference from other shells' 'typeset' at the
same places we document that other shells have noshwordsplit by default.

Thanks for the discussion; I realize the patch's chances of being
accepted are slim, but I appreciate the time people take to explain why.

Cheers,

Daniel

Error status of "repeat" (was Re: [PATCH] typeset: set $? on incidental error)

[Bart Schaefer]

On Jan 26, 10:50pm, Daniel Shahaf wrote:
}
} What would you expect "repeat 0+++ (exit 42)" to set $? to?

In fact what it does set $? to, is 0.

}        repeat WORD do LIST done
}               WORD is expanded and treated as an arithmetic expression, which
}               must evaluate to a number N.  LIST is then executed N times.

Hmm, this doesn't actually appear to happen.

torch% x=1+3
torch% repeat x print -n Z
torch% repeat $x print Z
Z
torch% 

So it's just peeling the first thing that looks like a number off the
WORD and repeating that many times, or zero times if WORD is not a number.

BUT!  Even if we change the code --


diff --git a/Src/loop.c b/Src/loop.c
index 4def9b6..61dad09 100644
--- a/Src/loop.c
+++ b/Src/loop.c
@@ -493,7 +493,7 @@ execrepeat(Estate state, UNUSED(int do_exec))
     tmp = ecgetstr(state, EC_DUPTOK, &htok);
     if (htok)
 	singsub(&tmp);
-    count = atoi(tmp);
+    count = mathevali(tmp);
     pushheap();
     cmdpush(CS_REPEAT);
     loops++;


-- that doesn't change the exit status!

torch% repeat 0+++ (exit 42)
zsh: bad math expression: lvalue required
torch% print $?
0

Next question: Do I commit that patch?

Re: Error status of "repeat" (was Re: [PATCH] typeset: set $? on incidental error)

[Bart Schaefer]

On Jan 26,  8:15pm, Bart Schaefer wrote:
}
}      if (htok)
}  	singsub(&tmp);
} -    count = atoi(tmp);
} +    count = mathevali(tmp);

What the heck does that singsub() accomplish there, anyway?  I can't find
any string to be passed to "repeat" that is changed in any way by passing
through singsub().

Re: Error status of "repeat" (was Re: [PATCH] typeset: set $? on incidental error)

[Peter Stephenson]

On Tue, 26 Jan 2016 20:15:13 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Jan 26, 10:50pm, Daniel Shahaf wrote:
> }
> } What would you expect "repeat 0+++ (exit 42)" to set $? to?
>...
> diff --git a/Src/loop.c b/Src/loop.c
> index 4def9b6..61dad09 100644
> --- a/Src/loop.c
> +++ b/Src/loop.c
> @@ -493,7 +493,7 @@ execrepeat(Estate state, UNUSED(int do_exec))
>      tmp = ecgetstr(state, EC_DUPTOK, &htok);
>      if (htok)
>  	singsub(&tmp);
> -    count = atoi(tmp);
> +    count = mathevali(tmp);
>      pushheap();
>      cmdpush(CS_REPEAT);
>      loops++;

As it's documented that this is how it works, and it's hard to see how
it can break anything that already works since that will have to be an
integer at this point, that looks OK.

> -- that doesn't change the exit status!
> 
> torch% repeat 0+++ (exit 42)
> zsh: bad math expression: lvalue required
> torch% print $?
> 0

That message comes from a zerr() in the code.  It suggests repeat isn't
testing the error status between the code you've just added and
executing the loop --- as atoi() won't fail, just give potential
nonsense, this wasn't needed before.  Probably "if (errflag) return;"
before the pushheap() is correct.

The error comes from the expansion, not the loop itself which isn't
executed, so the status is going to be 1, and clearly repeating the loop
0 times if the +'s are stripped is going to give status 0.

The "if (htok) singsub();" is related to some not very obvious syntactic
sequence, but I remember I did manage to trigger the equivalent when
changing the "case" code last year.

pws

Re: Error status of "repeat" (was Re: [PATCH] typeset: set $? on incidental error)

[Daniel Shahaf]

Peter Stephenson wrote on Wed, Jan 27, 2016 at 09:52:12 +0000:
> On Tue, 26 Jan 2016 20:15:13 -0800
> Bart Schaefer <schaefer@brasslantern.com> wrote:
> > On Jan 26, 10:50pm, Daniel Shahaf wrote:
> > }
> > } What would you expect "repeat 0+++ (exit 42)" to set $? to?
> >...
> > diff --git a/Src/loop.c b/Src/loop.c
> > index 4def9b6..61dad09 100644
> > --- a/Src/loop.c
> > +++ b/Src/loop.c
> > @@ -493,7 +493,7 @@ execrepeat(Estate state, UNUSED(int do_exec))
> >      tmp = ecgetstr(state, EC_DUPTOK, &htok);
> >      if (htok)
> >  	singsub(&tmp);
> > -    count = atoi(tmp);
> > +    count = mathevali(tmp);
> >      pushheap();
> >      cmdpush(CS_REPEAT);
> >      loops++;
> 
> As it's documented that this is how it works, and it's hard to see how
> it can break anything that already works since that will have to be an
> integer at this point, that looks OK.

I won't say "break", but here are some syntaxes that will now have
a different meaning:

    setopt octalzeroes
    repeat 010 foo

    setopt NO_cbases
    integer N=$(( [#16] 100 ))
    repeat $N foo

    setopt cbases
    integer N=$(( [#16] 100 ))
    repeat $N foo

    typeset total=1,500
    repeat $total foo

    # the cmdsubst evaluates to "42 total"
    repeat $(wc -l foo.c bar.c | tail -n1) foo

    repeat 2016-01-29 foo

    repeat 5.999999999999999999999 foo

The existing code would find
	10, 16, 0, 1, 42, 2016, 5
The new code would find
	8, 100, 100, 500, error, 1986, 6
(The second and third chop the string off at the '#' ox 'x'.  The fourth
one is a comma operator.  The sixth one is a subtraction.  The seventh one
gets rounded.)

---

While at it, here are a few syntaxes that don't change behaviour:

    repeat -42 foo
    # Should this be a usage error?

    repeat inf foo
    # I hoped this one would spell an infinite loop (per strtod(3)).

    repeat +5 foo
    # Works both before and after the patch, same as 'repeat 5 foo'.

---

Should we add a NEWS entry for this?  I realize it's not likely to
affect many people, but on the other hand it is an incompatibility and
we might want to err on the side of caution and mention it:

    diff --git a/NEWS b/NEWS
    index 15822ad..3568dc8 100644
    --- a/NEWS
    +++ b/NEWS
    @@ -4,5 +4,13 @@ CHANGES FROM PREVIOUS VERSIONS OF ZSH
     
     Note also the list of incompatibilities in the README file.
     
    +Changes from 5.2 to 5.3
    +-----------------------
    +
    +The first argument to 'repeat' is now evaluated as an arithmetic
    +expression.  It was always documented to be an arithmetic expression,
    +but until now the decimal integer at the start of the value was used
    +and the remainder of the value discarded.
    +
     Changes from 5.1.1 to 5.2
     -------------------------

Cheers,

Daniel

P.S. If anyone ever looks back on this mail to find corner cases, here's
another one: 9007199254740993.0.  That number is exactly representable
as an integer (if atoi()'s return type is a 64-bit type) but not as
a double, so the round-trip (casting an integer to double and back) will
be lossy.

Re: Error status of "repeat" (was Re: [PATCH] typeset: set $? on incidental error)

[Peter Stephenson]

On Fri, 29 Jan 2016 09:18:06 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Peter Stephenson wrote on Wed, Jan 27, 2016 at 09:52:12 +0000:
> > On Tue, 26 Jan 2016 20:15:13 -0800
> > Bart Schaefer <schaefer@brasslantern.com> wrote:
> > > On Jan 26, 10:50pm, Daniel Shahaf wrote:
> > > }
> > > } What would you expect "repeat 0+++ (exit 42)" to set $? to?
> 
> I won't say "break", but here are some syntaxes that will now have
> a different meaning:

Yeah, probably stuff people never quite intended to work the way it
actually did, but no guarantees.

> Should we add a NEWS entry for this?

Certainly; it could even be bumped to "incompatibilities" in the README
based on one or two of your examples.

pws

Re: Error status of "repeat" (was Re: [PATCH] typeset: set $? on incidental error)

[Daniel Shahaf]

Peter Stephenson wrote on Fri, Jan 29, 2016 at 09:29:00 +0000:
> On Fri, 29 Jan 2016 09:18:06 +0000
> Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Should we add a NEWS entry for this?
> 
> Certainly; it could even be bumped to "incompatibilities" in the README
> based on one or two of your examples.

Right, it should be next to all other incompatibilities.  Pushed.

I might extend the entry with an example later (or anyone else feel free
to beat me to that, too).

Re: [PATCH] typeset: set $? on incidental error

[Bart Schaefer]

On Jan 26, 10:50pm, Daniel Shahaf wrote:
}
} If existing typesets always exit zero in the circumstance that the patch
} proposes to change, no code would be broken by this change (no existing
} code, no new zsh-targeted code, no new ksh-targeted code), except for
} new ksh-targeted code written by zsh users who won't be aware of this
} difference.

This is not true.  Scripts using "setopt errexit" or "setopt errreturn"
or "set -e" will fail in ways they did not fail before.

Re: [PATCH] typeset: set $? on incidental error

[Daniel Shahaf]

Bart Schaefer wrote on Tue, Jan 26, 2016 at 20:17:24 -0800:
> On Jan 26, 10:50pm, Daniel Shahaf wrote:
> }
> } If existing typesets always exit zero in the circumstance that the patch
> } proposes to change, no code would be broken by this change (no existing
> } code, no new zsh-targeted code, no new ksh-targeted code), except for
> } new ksh-targeted code written by zsh users who won't be aware of this
> } difference.
> 
> This is not true.  Scripts using "setopt errexit" or "setopt errreturn"
> or "set -e" will fail in ways they did not fail before.

Scripts using errexit would have a new early-out failure mode.  I think
taking the new early-out would, at least in some cases, be the right
thing to do: it may be better to abort than to process invalid or
incomplete data.  I agree that it might be a bug in other cases.

All that said, I suppose we may as well stop here: it's clear the
original patch isn't on the verge of being applied.  How about a docs
patch instead, to point out the difference in behaviour? —

diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index fb630a7..6e3d146 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -1826,6 +1826,23 @@ reserved word interface for tt(typeset) may cause problems with the
 output of `tt(typeset -p)', which assumes the reserved word interface is
 available in order to restore array and associative array values.
 
+Unlike parameter assignment statements, tt(typeset)'s exit status on an
+assignment that involves a command substitution does not reflect the exit
+status of the command substitution.  Therefore, to test for an error in
+a command substitution, separate the declaration of the parameter from its
+initialization:
+
+example(# WRONG
+typeset var1=$(exit 1) || echo "Trouble with var1"
+
+# RIGHT
+typeset var1 && var1=$(exit 1) || echo "Trouble with var1"
+)
+
+To initialize a parameter var(param) to a command output and mark it readonly,
+use tt(typeset -r )var(param) or tt(readonly )var(param) after the parameter
+assignment statement.
+
 If the shell option tt(TYPESET_SILENT) is not set, for each remaining
 var(name) that refers to a parameter that is already set, the name and
 value of the parameter are printed in the form of an assignment.

Cheers,

Daniel

typeset docs flow Re: [PATCH] typeset: set $? on incidental error

[Daniel Shahaf]

Daniel Shahaf wrote on Fri, Jan 29, 2016 at 09:18:34 +0000:
> diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
> index fb630a7..6e3d146 100644
> --- a/Doc/Zsh/builtins.yo
> +++ b/Doc/Zsh/builtins.yo
> @@ -1826,6 +1826,23 @@ reserved word interface for tt(typeset) may cause problems with the
>  output of `tt(typeset -p)', which assumes the reserved word interface is
>  available in order to restore array and associative array values.
>  
> +Unlike parameter assignment statements, tt(typeset)'s exit status on an
> +assignment that involves a command substitution does not reflect the exit
> +status of the command substitution.  Therefore, to test for an error in
> +a command substitution, separate the declaration of the parameter from its
> +initialization:
> +
> +example(# WRONG
> +typeset var1=$(exit 1) || echo "Trouble with var1"
> +
> +# RIGHT
> +typeset var1 && var1=$(exit 1) || echo "Trouble with var1"
> +)
> +
> +To initialize a parameter var(param) to a command output and mark it readonly,
> +use tt(typeset -r )var(param) or tt(readonly )var(param) after the parameter
> +assignment statement.
> +

The following paragraph (the context lines of the patch) is the
continuation of the text about var(name) that precedes the segment about
the reserved word syntax.

The fix is probably to move the whole reserved words chunk to right
before the startitem() of the control flags?  I'd fix that but I'm short
on time at the moment and don't want to do a rush job of it.

Daniel

>  If the shell option tt(TYPESET_SILENT) is not set, for each remaining
>  var(name) that refers to a parameter that is already set, the name and
>  value of the parameter are printed in the form of an assignment.

Re: typeset docs flow Re: [PATCH] typeset: set $? on incidental error

[Bart Schaefer]

On Jan 30,  7:46am, Daniel Shahaf wrote:
} Subject: typeset docs flow Re: [PATCH] typeset: set $? on incidental error
}
} The following paragraph (the context lines of the patch) is the
} continuation of the text about var(name) that precedes the segment about
} the reserved word syntax.

How about the below?

Two other things:  (1) I still get a double blank line before any use
of example(...) -- I thought we'd come up with a fix for that (Jun T.
perhaps?).  (2) I noticed in passing that there are a lot of places
where builtins.yo says "the shell will try ..." (sometimes without the
word "try") -- if someone were up for a lot of work, this should all
be rewritten in active voice, i.e., "the shell does", not "the shell
will (do)"; and unless something useful happens when "try" does not
succeed [in which case explain it], remove the wishy-washy. 


diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index cdf405b..365f8e1 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -1763,18 +1763,31 @@ ifnzman(noderef(Local Parameters))\
 retain their special attributes when made local.
 
 For each var(name)tt(=)var(value) assignment, the parameter
-var(name) is set to var(value).  All forms of the command
-handle scalar assignment.
+var(name) is set to var(value).
 
-If any of the reserved words tt(declare), tt(export), tt(float),
+If the shell option tt(TYPESET_SILENT) is not set, for each remaining
+var(name) that refers to a parameter that is already set, the name and
+value of the parameter are printed in the form of an assignment.
+Nothing is printed for newly-created parameters, or when any attribute
+flags listed below are given along with the var(name).  Using
+`tt(PLUS())' instead of minus to introduce an attribute turns it off.
+
+If no var(name) is present, the names and values of all parameters are
+printed.  In this case the attribute flags restrict the display to only
+those parameters that have the specified attributes, and using `tt(PLUS())'
+rather than `tt(-)' to introduce the flag suppresses printing of the values
+of parameters when there is no parameter name.
+
+All forms of the command handle scalar assignment.  Array assignment is
+possible if any of the reserved words tt(declare), tt(export), tt(float),
 tt(integer), tt(local), tt(readonly) or tt(typeset) is matched when the
-line is parsed (N.B. not when it is executed) the shell will try to parse
-arguments as assignments, except that the `tt(+=)' syntax and the
-tt(GLOB_ASSIGN) option are not supported.  This has two major differences
-from normal command line argument parsing: array assignment is possible,
-and scalar values after tt(=) are not split further into words even if
-expanded (regardless of the setting of the tt(KSH_TYPESET) option; this
-option is obsolete).  Here is an example:
+line is parsed (N.B. not when it is executed).  In this case the arguments
+are parsed as assignments, except that the `tt(+=)' syntax and the
+tt(GLOB_ASSIGN) option are not supported, and scalar values after tt(=)
+are em(not) split further into words, even if expanded (regardless of the
+setting of the tt(KSH_TYPESET) option; this option is obsolete).
+
+Examples of the differences between command and reserved word parsing:
 
 example(# Reserved word parsing
 typeset svar=$(echo one word) avar=(several words))
@@ -1843,19 +1856,6 @@ To initialize a parameter var(param) to a command output and mark it readonly,
 use tt(typeset -r )var(param) or tt(readonly )var(param) after the parameter
 assignment statement.
 
-If the shell option tt(TYPESET_SILENT) is not set, for each remaining
-var(name) that refers to a parameter that is already set, the name and
-value of the parameter are printed in the form of an assignment.
-Nothing is printed for newly-created parameters, or when any attribute
-flags listed below are given along with the var(name).  Using
-`tt(PLUS())' instead of minus to introduce an attribute turns it off.
-
-If no var(name) is present, the names and values of all parameters are
-printed.  In this case the attribute flags restrict the display to only
-those parameters that have the specified attributes, and using `tt(PLUS())'
-rather than `tt(-)' to introduce the flag suppresses printing of the values
-of parameters when there is no parameter name.
-
 If no attribute flags are given, and either no var(name) arguments are
 present or the flag tt(+m) is used, then each parameter name printed is
 preceded by a list of the attributes of that parameter (tt(array),

Re: typeset docs flow Re: [PATCH] typeset: set $? on incidental error

[Jun T.]

2016/01/31 04:47, Bart Schaefer <schaefer@brasslantern.com> wrote:
> (1) I still get a double blank line before any use
> of example(...)

In which format? man pages? or pdf/html/info?
I don't experience any such problem with yodl 3.0.
Which version of yodl are you using?

The documents in the official tar balls look also OK.
(I don't know the yodl version used for these official docs).

Re: typeset docs flow Re: [PATCH] typeset: set $? on incidental error

[Bart Schaefer]

On Jan 31,  9:49am, Jun T. wrote:
}
} 
} 2016/01/31 04:47, Bart Schaefer <schaefer@brasslantern.com> wrote:
} > (1) I still get a double blank line before any use
} > of example(...)
} 
} In which format? man pages? or pdf/html/info?

Info format, with the standalone info viewer.

} Which version of yodl are you using?

schaefer[645] info --version
info (GNU texinfo) 4.13

Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

schaefer[646] yodl --version
yodl version 3.00.0


Also happens with yodl 1.3.18 on a much older virtual machine.

Re: typeset docs flow Re: [PATCH] typeset: set $? on incidental error

[Jun T.]

On 2016/02/01, at 2:03, Bart Schaefer <schaefer@brasslantern.com> wrote:
> } In which format? man pages? or pdf/html/info?
> 
> Info format, with the standalone info viewer.

I've found that makeinfo ver.4.8 has the problem (double blank lines
before example), but makeinfo 5.2 works fine.

Maybe you are using makeinfo (texinfo) 4.x?

The corresponding part in zsh.texi looks like:

----
Examples of the differences between command and reserved word parsing:

@noindent
@example
# Reserved word parsing
typeset svar=$(echo one word) avar=(several words)
@end example
----

and the extra blank line is caused by the @noindent. I think outputting
the extra blank line is a bug of makeinfo 4.x.
Since the example is indented anyway, the @noindent is redundant and
could be eliminated in theory. But it would not be easy to do so because
@noindent is output by PARAGRAPH() macro which is called immediately
after yodl finds one or more blank line(s) (without reading ahead).

Re: typeset docs flow Re: [PATCH] typeset: set $? on incidental error

[Bart Schaefer]

On Sun, Jan 31, 2016 at 7:23 PM, Jun T. <takimoto-j@kba.biglobe.ne.jp> wrote:
>
> Maybe you are using makeinfo (texinfo) 4.x?

Yep, 4.13.

Re: [PATCH] typeset: set $? on incidental error

[Vincent Lefevre]

On 2016-01-20 07:47:52 +0000, Daniel Shahaf wrote:
> In real-life examples, if the command subst had a non-zero exit status,
> the value would typically _not_ be the one I asked for:
> 
>     % cd $(mktemp -d)
>     % local mtime=$(zstat +mtime myfile)
>     zstat: myfile: no such file or directory

This is not specific to "local". Same expected behavior with "printf":

cventin% local mtime=$(zstat +mtime myfile)
zsh: command not found: zstat
cventin% echo $?
0
cventin% printf "%s\n" $(zstat +mtime myfile)
zsh: command not found: zstat

cventin% echo $?
0

-- 
Vincent Lefèvre <vincent@vinc17.net> - Web: <https://www.vinc17.net/>
100% accessible validated (X)HTML - Blog: <https://www.vinc17.net/blog/>
Work: CR INRIA - computer arithmetic / AriC project (LIP, ENS-Lyon)