zsh-workers
 help / color / mirror / code / Atom feed
* zmathfunc: min, max, sum throw error if result equals 0
@ 2021-03-07 16:37 Nikolaus Thiel
  2021-03-07 17:17 ` Daniel Shahaf
  0 siblings, 1 reply; 13+ messages in thread
From: Nikolaus Thiel @ 2021-03-07 16:37 UTC (permalink / raw)
  To: zsh-workers

There seems to be a bug in zmathfunc:

When the result of min, max or sum equals 0, the functions throw an error. If
they are used within a script with the option "set -e" then the script aborts,
cf. test script below.

I have confirmed this behaviour on two systems:


(1) Apple MacBook Air, Ubuntu 20.04.2 LTS

Linux Luftbuch 5.4.0-66-generic #74-Ubuntu SMP Wed Jan 27 22:54:38 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux
zsh 5.8 (x86_64-ubuntu-linux-gnu)


(2) Apple iMac, macOS 11.2.2

Darwin Kernel Version 20.3.0: Thu Jan 21 00:07:06 PST 2021; root:xnu-7195.81.3~1/RELEASE_X86_64
zsh 5.8 (x86_64-apple-darwin20.0)



-------- Test Script --------

#!/usr/bin/env zsh
set -e
OS=$(uname)
if [[ ${OS} == "Darwin" ]]; then
    ### Mac OS X
    source /usr/share/zsh/5.8/functions/zmathfunc
else
    ### Ubuntu 20.04
    source /usr/share/zsh/functions/Math/zmathfunc
fi

echo "OS = ${OS}"
zsh --version

echo "works fine"

x=$(( min(2,-1,3) ))
echo "min(2,1,3)  = $((x))"
x=$(( max(2,-1,3) ))
echo "max(2,-1,3) = $((x))"
x=$(( sum(2,-1,3) ))
echo "sum(2,-1,3) = $((x))"


echo "min, max, sum throw an error if result=0"
echo " => set -e causes script to abort"

x=$(( sum(-2,2) ))
echo "sum(-2,2)  = $((x))"

x=$(( min(2,0,3) ))
echo "min(2,0,3)  = $((x))"

x=$(( max(-2,0,-3) ))
echo "max(-2,0,-3)  = $((x))"



-------- Output on Mac OS X --------
OS = Darwin
zsh 5.8 (x86_64-apple-darwin20.0)
works fine
min(2,1,3)  = -1
max(2,-1,3) = 3
sum(2,-1,3) = 4
min, max, sum throw an error if result=0
 => set -e causes script to abort


-------- Output on Ubuntu --------
OS = Linux
zsh 5.8 (x86_64-ubuntu-linux-gnu)
works fine
min(2,1,3)  = -1
max(2,-1,3) = 3
sum(2,-1,3) = 4
min, max, sum throw an error if result=0
 => set -e causes script to abort


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: zmathfunc: min, max, sum throw error if result equals 0
  2021-03-07 16:37 zmathfunc: min, max, sum throw error if result equals 0 Nikolaus Thiel
@ 2021-03-07 17:17 ` Daniel Shahaf
  2021-03-07 21:39   ` Bart Schaefer
  2021-04-16 18:21   ` Daniel Shahaf
  0 siblings, 2 replies; 13+ messages in thread
From: Daniel Shahaf @ 2021-03-07 17:17 UTC (permalink / raw)
  To: Nikolaus Thiel; +Cc: zsh-workers

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

Nikolaus Thiel wrote on Sun, Mar 07, 2021 at 17:37:09 +0100:
> There seems to be a bug in zmathfunc:
> 
> When the result of min, max or sum equals 0, the functions throw an error. If
> they are used within a script with the option "set -e" then the script aborts,
> cf. test script below.

Ouch.  Patch series attached.

However, the examples in zshbuiltins(1) (under `functions -M`) have the
same bug.  Would you perchance be interested in fixing those?  The
manual sources are in Doc/Zsh/*.yo.

Review below.

> if [[ ${OS} == "Darwin" ]]; then
>     ### Mac OS X
>     source /usr/share/zsh/5.8/functions/zmathfunc
> else
>     ### Ubuntu 20.04
>     source /usr/share/zsh/functions/Math/zmathfunc
> fi

autoload -Uz zmathfunc && zmathfunc

> echo "OS = ${OS}"

typeset -p OS

or use the builtin variable:

typeset -p OSTYPE

> zsh --version

echo $ZSH_VERSION $ZSH_PATCHLEVEL

>

Cheers,

Daniel


[-- Attachment #2: 0001-tests-Add-a-unit-test-for-zmathfunc-and-a-regres.patch.txt --]
[-- Type: text/plain, Size: 995 bytes --]

From eed36947fb1a35cdaf21638fb49ecfc4f40cdfc5 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Sun, 7 Mar 2021 16:58:03 +0000
Subject: [PATCH 1/2] tests: Add a unit test for zmathfunc and a regression
 test for workers/48146 affecting it.

---
 Test/Z02zmathfunc.ztst | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Test/Z02zmathfunc.ztst

diff --git a/Test/Z02zmathfunc.ztst b/Test/Z02zmathfunc.ztst
new file mode 100644
index 000000000..94bc59576
--- /dev/null
+++ b/Test/Z02zmathfunc.ztst
@@ -0,0 +1,23 @@
+%prep
+  autoload -Uz zmathfunc && zmathfunc
+
+%test
+
+  echo $(( min(42, 43) )) $(( max(42, 43) )) $(( sum(42, 43) ))
+  echo $(( min(42) )) $(( max(42) )) $(( sum(42) ))
+  echo $(( sum() ))
+0:basic functionality test
+>42 43 85
+>42 42 42
+>0
+
+
+  (set -e; echo $(( min(0,   42) )))
+  (set -e; echo $(( max(0,  -42) )))
+  (set -e; echo $(( sum(42, -42) )))
+-f:regression test for ERR_EXIT 
+>0
+>0
+>0
+
+%clean

[-- Attachment #3: 0002-zmathfunc-Fix-bug-where-the-exit-code-would-be-n.patch.txt --]
[-- Type: text/plain, Size: 1853 bytes --]

From dca20086146e7c6022e394cc760ad39b12d96db2 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Sun, 7 Mar 2021 17:07:06 +0000
Subject: [PATCH 2/2] zmathfunc: Fix bug where the exit code would be non-zero
 if the expression evaluted to zero.

---
 Functions/Math/zmathfunc | 10 ++++++++--
 Test/Z02zmathfunc.ztst   |  2 +-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/Functions/Math/zmathfunc b/Functions/Math/zmathfunc
index 4ff40700d..8e4b78549 100644
--- a/Functions/Math/zmathfunc
+++ b/Functions/Math/zmathfunc
@@ -1,34 +1,40 @@
 #autoload
 
 zsh_math_func_min() {
+  emulate -L zsh
   local result=$1
   shift
   local arg
   for arg ; do
     (( $arg < result )) && result=$arg
   done
-  (( result )) # return
+  (( result ))
+  true # Careful here: `return 0` evaluates an arithmetic expression
 }
 functions -M min 1 -1 zsh_math_func_min # at least one argument
 
 zsh_math_func_max() {
+  emulate -L zsh
   local result=$1
   shift
   local arg
   for arg ; do
     (( $arg > result )) && result=$arg
   done
-  (( result )) # return
+  (( result ))
+  true # Careful here: `return 0` evaluates an arithmetic expression
 }
 functions -M max 1 -1 zsh_math_func_max # at least one argument
 
 zsh_math_func_sum() {
+  emulate -L zsh
   local sum
   local arg
   for arg ; do
     (( sum += $arg ))
   done
   (( sum ))
+  true # Careful here: `return 0` evaluates an arithmetic expression
 }
 functions -M sum 0 -1 zsh_math_func_sum
 
diff --git a/Test/Z02zmathfunc.ztst b/Test/Z02zmathfunc.ztst
index 94bc59576..43a0a0d76 100644
--- a/Test/Z02zmathfunc.ztst
+++ b/Test/Z02zmathfunc.ztst
@@ -15,7 +15,7 @@
   (set -e; echo $(( min(0,   42) )))
   (set -e; echo $(( max(0,  -42) )))
   (set -e; echo $(( sum(42, -42) )))
--f:regression test for ERR_EXIT 
+0:regression test for ERR_EXIT 
 >0
 >0
 >0

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: zmathfunc: min, max, sum throw error if result equals 0
  2021-03-07 17:17 ` Daniel Shahaf
@ 2021-03-07 21:39   ` Bart Schaefer
  2021-03-07 21:56     ` Daniel Shahaf
  2021-04-16 18:21   ` Daniel Shahaf
  1 sibling, 1 reply; 13+ messages in thread
From: Bart Schaefer @ 2021-03-07 21:39 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: Nikolaus Thiel, zsh-workers

On Sun, Mar 7, 2021 at 9:17 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> Nikolaus Thiel wrote on Sun, Mar 07, 2021 at 17:37:09 +0100:
> >
> > When the result of min, max or sum equals 0, the functions throw an error.

I would not equate "return nozero" with "throw an error", FWIW.

> Ouch.  Patch series attached.

I think your regression tests cover this, but use of "true" avoids
changing the result in "functions -M" context, correct?

Is it worth testing invalid cases?  Such as uses outside math context
where the arguments are not syntax checked?

As long as we're on the subject:

zsh_math_func_min() {
  local result=$1
  shift
  local arg
  for arg ; do
    (( $arg < result )) && result=$arg
  done
  (( result ))
  true # Careful here: `return 0` evaluates an arithmetic expression
}

Because of the way math context works, if any of $@ is a string that
can be interpreted as a math expression, the above will evaluate it at
least twice (and up to $# times in the case of $1).  This could have
side-effects.

zsh_math_func_min() {
  local result=$(( $1 ))
  shift
  local arg n
  for arg ; do
    (( n = arg )) # evaluate arg exactly once
    (( n < result && (result = n) ))
  done
  (( result ))
  true # Careful here: `return 0` evaluates an arithmetic expression
}


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: zmathfunc: min, max, sum throw error if result equals 0
  2021-03-07 21:39   ` Bart Schaefer
@ 2021-03-07 21:56     ` Daniel Shahaf
  2021-03-08  2:25       ` Bart Schaefer
  0 siblings, 1 reply; 13+ messages in thread
From: Daniel Shahaf @ 2021-03-07 21:56 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: Nikolaus Thiel, zsh-workers

Bart Schaefer wrote on Sun, 07 Mar 2021 21:39 +00:00:
> On Sun, Mar 7, 2021 at 9:17 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > Nikolaus Thiel wrote on Sun, Mar 07, 2021 at 17:37:09 +0100:
> > >
> > > When the result of min, max or sum equals 0, the functions throw an error.
> 
> I would not equate "return nozero" with "throw an error", FWIW.
> 
> > Ouch.  Patch series attached.
> 
> I think your regression tests cover this, but use of "true" avoids
> changing the result in "functions -M" context, correct?

Yeah.  «return 0» causes the arithmetic expression to evaluate to 0,
because the argument to «return» is math-evaluated.

> Is it worth testing invalid cases?  Such as uses outside math context
> where the arguments are not syntax checked?

I'd say this specific example of an invalid case is lower priority,
since there's no promise of any particular failure mode.  We could add
a test to ensure it doesn't format C:, of course.

A test for min() and max() with zero arguments would make sense, since
those cases actually do promise a specific failure mode.

Could also add tests with three arguments in various permutations.

If you have ideas, feel free to write them and post them; I'll
transplant them into Z02 once I have committed it.

> As long as we're on the subject:
> 
> zsh_math_func_min() {
>   local result=$1
>   shift
>   local arg
>   for arg ; do
>     (( $arg < result )) && result=$arg
>   done
>   (( result ))
>   true # Careful here: `return 0` evaluates an arithmetic expression
> }
> 
> Because of the way math context works, if any of $@ is a string that
> can be interpreted as a math expression, the above will evaluate it at
> least twice (and up to $# times in the case of $1).  This could have
> side-effects.

Could you post a regression test for this?

Related to code as it is in master, are «(( $arg < result ))» and
«(( arg < result ))» equivalent?

> zsh_math_func_min() {
>   local result=$(( $1 ))

Doesn't this still do multiple string-to-number conversions?

>   shift
>   local arg n
>   for arg ; do
>     (( n = arg )) # evaluate arg exactly once
>     (( n < result && (result = n) ))
>   done
>   (( result ))
>   true # Careful here: `return 0` evaluates an arithmetic expression
> }

Hmm.  I'll go ahead and push the series now, then, so it'll be easier
for you to work on revisions.  If anyone has commit reviews, don't let
the fact that I've pushed things affect you.

Cheers,

Daniel


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: zmathfunc: min, max, sum throw error if result equals 0
  2021-03-07 21:56     ` Daniel Shahaf
@ 2021-03-08  2:25       ` Bart Schaefer
  2021-04-16 18:26         ` Daniel Shahaf
  2021-04-16 19:50         ` Daniel Shahaf
  0 siblings, 2 replies; 13+ messages in thread
From: Bart Schaefer @ 2021-03-08  2:25 UTC (permalink / raw)
  To: zsh-workers

On Sun, Mar 7, 2021 at 1:57 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> Bart Schaefer wrote on Sun, 07 Mar 2021 21:39 +00:00:
>
> > Is it worth testing invalid cases?  Such as uses outside math context
> > where the arguments are not syntax checked?
>
> If you have ideas, feel free to write them and post them; I'll
> transplant them into Z02 once I have committed it.

One possibility below; I'll follow up if I think of more.

With your patch adding "true" at the end, we get this:

% zsh_math_func_min "foo bar" x y z
zsh_math_func_min:7: bad math expression: operator expected at `bar'
zsh_math_func_min:7: bad math expression: operator expected at `bar'
zsh_math_func_min:7: bad math expression: operator expected at `bar'
zsh_math_func_min:9: bad math expression: operator expected at `bar'
toltec-ubuntu% echo $?
0

Doesn't seem as though anything that prints that many error messages
should return zero.

> > Because of the way math context works, if any of $@ is a string that
> > can be interpreted as a math expression, the above will evaluate it at
> > least twice (and up to $# times in the case of $1).  This could have
> > side-effects.
>
> Could you post a regression test for this?

In thinking more about it, I believe this only matters when the
function is called outside of math context.  In math context, the
arguments are all going to be evaluated down to numbers before they
are passed to the function.

% (( x = 0 )); zsh_math_func_min "x += 2" 4 5 6
% print $x
8
% (( (x = 0), min("x += 2", 4, 5, 6) ))
% print $x
2

> Related to code as it is in master, are «(( $arg < result ))» and
> «(( arg < result ))» equivalent?

No, but again math context matters.

% arg="x += 2" x=0 result=0
% (( $arg > result )) && result=$arg
% print $result $x $(( result )) $x
x += 2 1 3 3
% x=0 result=0
% (( arg > result )) && (( result=arg ))
% print $result $x $(( result )) $x
4 4 4 4

> > zsh_math_func_min() {
> >   local result=$(( $1 ))
>
> Doesn't this still do multiple string-to-number conversions?

I don't think so ... the $(( ... )) should turn $1 into a single
number assigned to result.  What are you thinking about?


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: zmathfunc: min, max, sum throw error if result equals 0
  2021-03-07 17:17 ` Daniel Shahaf
  2021-03-07 21:39   ` Bart Schaefer
@ 2021-04-16 18:21   ` Daniel Shahaf
  2021-05-16 14:54     ` Lawrence Velázquez
  1 sibling, 1 reply; 13+ messages in thread
From: Daniel Shahaf @ 2021-04-16 18:21 UTC (permalink / raw)
  To: zsh-workers; +Cc: Nikolaus Thiel

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

Daniel Shahaf wrote on Sun, Mar 07, 2021 at 17:17:12 +0000:
> However, the examples in zshbuiltins(1) (under `functions -M`) have the
> same bug.  Would you perchance be interested in fixing those?  The
> manual sources are in Doc/Zsh/*.yo.

Patch series attached.

I base64'd the attachments (Mutt ☺) to avoid triggering workers/48587.
Would break grepping the raw mbox for them.  (I didn't want to spam
5 separate messages onto the list.  I do already have a draft fix for
workers/48587, but haven't deployed it yet.)

I can't say the semantics are very user-friendly.  For «return 0» or
«foo[42]=success» to change the result of an arithmetic function is
surprising.  At least it'll be a documented surprise, as opposed to an
undocumented one.

Cheers,

Daniel

[-- Attachment #2: 0001-docs-functions-M-Move-an-example-to-be-near-the-.patch.txt --]
[-- Type: text/plain, Size: 1652 bytes --]

From c2bbcbf4baa9d9f182ff25b79e2de7c0bbd1c320 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Fri, 16 Apr 2021 17:33:52 +0000
Subject: [PATCH 1/5] docs: functions -M: Move an example to be near the
 specification of the relevant flag

---
 Doc/Zsh/builtins.yo | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index a7afe42cf..62c30c917 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -924,6 +924,12 @@ opening and matching closing parenthesis is passed to the function as a
 single argument, even if it includes commas or white space.  The minimum
 and maximum argument specifiers must therefore be 1 if given.  An empty
 argument list is passed as a zero-length string.
+Thus, the following string function takes a single argument, including
+the commas, and prints 11:
+
+example(stringfn+LPAR()RPAR() { (( $#1 )) }
+functions -Ms stringfn
+print $(( stringfn+LPAR()foo,bar,rod+RPAR() )))
 
 tt(functions -M) with no arguments lists all such user-defined functions in
 the same form as a definition.  With the additional option tt(-m) and
@@ -941,13 +947,6 @@ For example, the following prints the cube of 3:
 example(zmath_cube+LPAR()RPAR() { (( $1 * $1 * $1 )) }
 functions -M cube 1 1 zmath_cube
 print $(( cube+LPAR()3+RPAR() )))
-
-The following string function takes a single argument, including
-the commas, so prints 11:
-
-example(stringfn+LPAR()RPAR() { (( $#1 )) }
-functions -Ms stringfn
-print $(( stringfn+LPAR()foo,bar,rod+RPAR() )))
 )
 module(getcap)(zsh/cap)
 findex(getln)

[-- Attachment #3: 0002-docs-functions-M-Document-the-return-status-resu.patch.txt --]
[-- Type: text/plain, Size: 3668 bytes --]

From 5d32996b704b0680528c75acce8ade975d4670de Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Fri, 16 Apr 2021 18:03:30 +0000
Subject: [PATCH 2/5] docs: functions -M: Document the return status / result
 interdependency gotchas (cf. 48147).

---
 Doc/Zsh/builtins.yo | 58 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index 62c30c917..80a9e97db 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -914,9 +914,53 @@ expressions.  The name of the function in tt($0) is var(mathfn) (not
 var(shellfn) as would usually be the case), provided the option
 tt(FUNCTION_ARGZERO) is in effect.  The positional parameters in the shell
 function correspond to the arguments of the mathematical function call.
-The result of the last arithmetical expression evaluated
-inside the shell function (even if it is a form that normally only returns
-a status) gives the result of the mathematical function.
+
+The result of the last arithmetical expression evaluated inside the shell
+function gives the result of the mathematical function.  This is not limited to
+arithmetic substitutions of the form tt($+LPAR()+LPAR())var(...)tt(+RPAR()+RPAR()),
+but also includes arithmetical expressions evaluated in any other way, including
+by the tt(let) builtin,
+by tt(+LPAR()+LPAR())var(...)tt(+RPAR()+RPAR()) statements,
+and even
+by the tt(return) builtin
+and
+by array subscripts.
+Therefore, care must be taken not to use syntactical constructs that perform
+arithmetic evaluation after evaluating what is to be the result of the function.
+For example:
+
+example(# WRONG
+zmath_cube+LPAR()+RPAR() {
+  (( $1 * $1 * $1 ))
+  return 0
+}
+functions -M cube 1 1 zmath_cube
+print $(( cube+LPAR()3+RPAR() )))
+
+This will print `tt(0)' because of the tt(return).
+
+Commenting the tt(return) out would lead to a different problem: the
+tt(+LPAR()+LPAR())var(...)tt(+RPAR()+RPAR()) statement would become
+the last statement in the function, so the em(return status) (tt($?)) of the
+function would be non-zero (indicating failure) whenever the em(arithmetic
+result) of the function would happen to be zero (numerically):
+
+example(# WRONG
+zmath_cube+LPAR()+RPAR() {
+  (( $1 * $1 * $1 ))
+}
+functions -M cube 1 1 zmath_cube
+print $(( cube+LPAR()0+RPAR() )))
+
+Instead, the tt(true) builtin can be used:
+
+example(# RIGHT
+zmath_cube+LPAR()+RPAR() {
+  (( $1 * $1 * $1 ))
+  true
+}
+functions -M cube 1 1 zmath_cube
+print $(( cube+LPAR()3+RPAR() )))
 
 If the additional option tt(-s) is given to tt(functions -M), the
 argument to the function is a single string: anything between the
@@ -927,7 +971,7 @@ argument list is passed as a zero-length string.
 Thus, the following string function takes a single argument, including
 the commas, and prints 11:
 
-example(stringfn+LPAR()RPAR() { (( $#1 )) }
+example(stringfn+LPAR()RPAR() { (( $#1 )); true }
 functions -Ms stringfn
 print $(( stringfn+LPAR()foo,bar,rod+RPAR() )))
 
@@ -941,12 +985,6 @@ additional option tt(-m) the arguments are treated as patterns and
 all functions whose var(mathfn) matches the pattern are removed.  Note
 that the shell function implementing the behaviour is not removed
 (regardless of whether its name coincides with var(mathfn)).
-
-For example, the following prints the cube of 3:
-
-example(zmath_cube+LPAR()RPAR() { (( $1 * $1 * $1 )) }
-functions -M cube 1 1 zmath_cube
-print $(( cube+LPAR()3+RPAR() )))
 )
 module(getcap)(zsh/cap)
 findex(getln)

[-- Attachment #4: 0003-docs-functions-M-Add-a-subheading-and-index-entr.patch.txt --]
[-- Type: text/plain, Size: 1226 bytes --]

From 90a76320587e7a9e6a978809674451495a538fec Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Fri, 16 Apr 2021 18:03:45 +0000
Subject: [PATCH 3/5] docs: functions -M: Add a subheading and index entries.

---
 Doc/Zsh/builtins.yo | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index 80a9e97db..264ee9484 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -893,6 +893,10 @@ without affecting the other.  A typical idiom is that var(oldfn) is the
 name of a library shell function which is then redefined to call
 tt(newfn), thereby installing a modified version of the function.
 
+em(The )tt(-M)em( and )tt(+M)em( flags)
+cindex(defining mathematical functions)
+cindex(functions, defining mathematical)
+
 Use of the tt(-M) option may not be combined with any of the options
 handled by tt(typeset -f).
 
@@ -929,6 +933,8 @@ Therefore, care must be taken not to use syntactical constructs that perform
 arithmetic evaluation after evaluating what is to be the result of the function.
 For example:
 
+findex(zmath_cube)
+findex(cube)
 example(# WRONG
 zmath_cube+LPAR()+RPAR() {
   (( $1 * $1 * $1 ))

[-- Attachment #5: 0004-docs-return-Give-examples-of-using-arithmetic-ev.patch.txt --]
[-- Type: text/plain, Size: 1651 bytes --]

From 08cee6879c28b5f4761050f12a3ec2b22544df62 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Fri, 16 Apr 2021 18:04:13 +0000
Subject: [PATCH 4/5] docs: return: Give examples of using arithmetic
 evaluation.

---
 Doc/Zsh/builtins.yo | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index 264ee9484..640cfb674 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -1626,9 +1626,15 @@ cindex(functions, returning from)
 item(tt(return) [ var(n) ])(
 Causes a shell function or `tt(.)' script to return to
 the invoking script with the return status specified by
-an arithmetic expression var(n). If var(n)
+an arithmetic expression var(n).  
+For example, the following prints `tt(42)':
+
+example(() { integer foo=40; return "foo + 2" }
+echo $?)
+
+If var(n)
 is omitted, the return status is that of the last command
-executed.
+executed. 
 
 If tt(return) was executed from a trap in a tt(TRAP)var(NAL) function,
 the effect is different for zero and non-zero return status.  With zero
@@ -1637,7 +1643,7 @@ will return to whatever it was previously processing; with a non-zero
 status, the shell will behave as interrupted except that the return
 status of the trap is retained.  Note that the numeric value of the signal
 which caused the trap is passed as the first argument, so the statement
-`tt(return $((128PLUS()$1)))' will return the same status as if the signal
+`tt(return "128PLUS()$1")' will return the same status as if the signal
 had not been trapped.
 )
 module(sched)(zsh/sched)

[-- Attachment #6: 0005-zmathfuncdef-Fix-the-workers-48147-return-status.patch.txt --]
[-- Type: text/plain, Size: 788 bytes --]

From 1f195629a23ded3ba4294bf4838eada6a695f07c Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Fri, 16 Apr 2021 18:04:39 +0000
Subject: [PATCH 5/5] zmathfuncdef: Fix the workers/48147 return status / 'set
 -e' bug.

Not tested.
---
 Functions/Misc/zmathfuncdef | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Functions/Misc/zmathfuncdef b/Functions/Misc/zmathfuncdef
index e5692e769..5ed991f68 100644
--- a/Functions/Misc/zmathfuncdef
+++ b/Functions/Misc/zmathfuncdef
@@ -78,7 +78,7 @@ if ! zmodload -e zsh/mathfunc; then
 fi
 
 {
-  eval "$fname() { (( $body )) }"
+  eval "$fname() { (( $body )); true }"
 } always {
   # Remove math function if shell function definition failed.
   if (( TRY_BLOCK_ERROR )); then

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: zmathfunc: min, max, sum throw error if result equals 0
  2021-03-08  2:25       ` Bart Schaefer
@ 2021-04-16 18:26         ` Daniel Shahaf
  2021-04-16 18:47           ` Bart Schaefer
  2021-04-16 19:50         ` Daniel Shahaf
  1 sibling, 1 reply; 13+ messages in thread
From: Daniel Shahaf @ 2021-04-16 18:26 UTC (permalink / raw)
  To: zsh-workers

Bart Schaefer wrote on Sun, Mar 07, 2021 at 18:25:00 -0800:
> On Sun, Mar 7, 2021 at 1:57 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Bart Schaefer wrote on Sun, 07 Mar 2021 21:39 +00:00:
> > > zsh_math_func_min() {
> > >   local result=$(( $1 ))
> >
> > Doesn't this still do multiple string-to-number conversions?
> 
> I don't think so ... the $(( ... )) should turn $1 into a single
> number assigned to result.  What are you thinking about?

When «$foo» appears inside «$((…))», it's expanded as a string that's
then parsed as math.  The result of «$((…))» is then assigned to the
$result, which is eventually used as «(( result ))», where — because
${(t)result} is non-integer scalar — there will be another
string-to-integer conversion.

Sorry for the delay on this.

Daniel


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: zmathfunc: min, max, sum throw error if result equals 0
  2021-04-16 18:26         ` Daniel Shahaf
@ 2021-04-16 18:47           ` Bart Schaefer
  0 siblings, 0 replies; 13+ messages in thread
From: Bart Schaefer @ 2021-04-16 18:47 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: Zsh hackers list

On Fri, Apr 16, 2021 at 11:26 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> When «$foo» appears inside «$((…))», it's expanded as a string that's
> then parsed as math.  The result of «$((…))» is then assigned to the
> $result, which is eventually used as «(( result ))», where — because
> ${(t)result} is non-integer scalar — there will be another
> string-to-integer conversion.

Yes, there's another string-to-integer conversion, but I haven't
figured out how «result=$((...))» can possibly assign any string that
is not already a plain number, so the string-to-integer conversion is
free of meaningful side-effects.  Have I missed something?


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: zmathfunc: min, max, sum throw error if result equals 0
  2021-03-08  2:25       ` Bart Schaefer
  2021-04-16 18:26         ` Daniel Shahaf
@ 2021-04-16 19:50         ` Daniel Shahaf
  2021-04-16 20:05           ` Bart Schaefer
  1 sibling, 1 reply; 13+ messages in thread
From: Daniel Shahaf @ 2021-04-16 19:50 UTC (permalink / raw)
  To: zsh-workers

Again sorry for the delay on getting back to this.

Bart Schaefer wrote on Sun, Mar 07, 2021 at 18:25:00 -0800:
> On Sun, Mar 7, 2021 at 1:57 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > Bart Schaefer wrote on Sun, 07 Mar 2021 21:39 +00:00:
> >
> > > Is it worth testing invalid cases?  Such as uses outside math context
> > > where the arguments are not syntax checked?
> >
> > If you have ideas, feel free to write them and post them; I'll
> > transplant them into Z02 once I have committed it.
> 
> One possibility below; I'll follow up if I think of more.
> 
> With your patch adding "true" at the end, we get this:
> 
> % zsh_math_func_min "foo bar" x y z
> zsh_math_func_min:7: bad math expression: operator expected at `bar'
> zsh_math_func_min:7: bad math expression: operator expected at `bar'
> zsh_math_func_min:7: bad math expression: operator expected at `bar'
> zsh_math_func_min:9: bad math expression: operator expected at `bar'
> toltec-ubuntu% echo $?
> 0
> 
> Doesn't seem as though anything that prints that many error messages
> should return zero.
> 

Patch below.

> > > Because of the way math context works, if any of $@ is a string that
> > > can be interpreted as a math expression, the above will evaluate it at
> > > least twice (and up to $# times in the case of $1).  This could have
> > > side-effects.
> >
> > Could you post a regression test for this?
> 
> In thinking more about it, I believe this only matters when the
> function is called outside of math context.  In math context, the
> arguments are all going to be evaluated down to numbers before they
> are passed to the function.
> 
> % (( x = 0 )); zsh_math_func_min "x += 2" 4 5 6
> % print $x
> 8
> % (( (x = 0), min("x += 2", 4, 5, 6) ))
> % print $x
> 2

I see.  Still, going to fix this, if only because those min() max()
sum() implementations are virtually the only example of `functions -M`
in the tree, and it'd be good to have a complete example.

> > Related to code as it is in master, are «(( $arg < result ))» and
> > «(( arg < result ))» equivalent?
> 
> No, but again math context matters.
> 
> % arg="x += 2" x=0 result=0
> % (( $arg > result )) && result=$arg
> % print $result $x $(( result )) $x
> x += 2 1 3 3
> % x=0 result=0
> % (( arg > result )) && (( result=arg ))
> % print $result $x $(( result )) $x
> 4 4 4 4

Nice.  So the first one interpolates $arg as in double-quoted strings
and then parses the string " x += 2 > result ", where the > operator has
higher precedence, and «2 > result» evaluates to 1, and the «print» then
evaluates everything left to right; whereas the latter sees «arg» and
tries to evaluate that _as a number_, which works out to 2 (with a side
effect), so the condition evaluates to true, and the assignment on the
RHS then works out to 4 (with a side effect).  Clear as a cloudless day ☺

Many thanks.

Daniel

diff --git a/Functions/Math/zmathfunc b/Functions/Math/zmathfunc
index 8e4b78549..28f21e562 100644
--- a/Functions/Math/zmathfunc
+++ b/Functions/Math/zmathfunc
@@ -6,7 +6,12 @@ zsh_math_func_min() {
   shift
   local arg
   for arg ; do
-    (( $arg < result )) && result=$arg
+    (( $arg < result ))
+    case $? in
+      (0) result=$arg;;
+      (1) ;;
+      (*) return $?;;
+    esac
   done
   (( result ))
   true # Careful here: `return 0` evaluates an arithmetic expression
@@ -19,7 +24,12 @@ zsh_math_func_max() {
   shift
   local arg
   for arg ; do
-    (( $arg > result )) && result=$arg
+    (( $arg > result ))
+    case $? in
+      (0) result=$arg;;
+      (1) ;;
+      (*) return $?;;
+    esac
   done
   (( result ))
   true # Careful here: `return 0` evaluates an arithmetic expression
@@ -31,7 +41,7 @@ zsh_math_func_sum() {
   local sum
   local arg
   for arg ; do
-    (( sum += $arg ))
+    (( sum += arg ))
   done
   (( sum ))
   true # Careful here: `return 0` evaluates an arithmetic expression


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: zmathfunc: min, max, sum throw error if result equals 0
  2021-04-16 19:50         ` Daniel Shahaf
@ 2021-04-16 20:05           ` Bart Schaefer
  2021-04-16 20:08             ` Daniel Shahaf
  0 siblings, 1 reply; 13+ messages in thread
From: Bart Schaefer @ 2021-04-16 20:05 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: Zsh hackers list

On Fri, Apr 16, 2021 at 12:51 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> [...] the first one interpolates $arg as in double-quoted strings
> and then parses the string " x += 2 > result ", where the > operator has
> higher precedence, and «2 > result» evaluates to 1, and the «print» then
> evaluates everything left to right; whereas the latter sees «arg» and
> tries to evaluate that _as a number_, which works out to 2 (with a side
> effect), so the condition evaluates to true, and the assignment on the
> RHS then works out to 4 (with a side effect).  Clear as a cloudless day

Given that, I'm puzzled why you did

> +    (( $arg < result ))
> +    case $? in
> +      (0) result=$arg;;
> +      (1) ;;
> +      (*) return $?;;
> +    esac
>    done
>    (( result ))

and why not

  (( arg < result ))
  case $? in
    (0) (( result = arg ));;
    (1) ;;
    (*) return $?;;
  esac
done
(( result ))

??  The (( $arg < result )) formulation still potentially has
different operator precedence in and out of math context, and
result=$arg followed by (( result )) still does repeat interpretation
of $arg.  Why ever interpolate $arg as a string?


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: zmathfunc: min, max, sum throw error if result equals 0
  2021-04-16 20:05           ` Bart Schaefer
@ 2021-04-16 20:08             ` Daniel Shahaf
  0 siblings, 0 replies; 13+ messages in thread
From: Daniel Shahaf @ 2021-04-16 20:08 UTC (permalink / raw)
  To: zsh-workers

Bart Schaefer wrote on Fri, 16 Apr 2021 20:05 +00:00:
> and why not
> 
>   (( arg < result ))
>   case $? in
>     (0) (( result = arg ));;
>     (1) ;;
>     (*) return $?;;
>   esac
> done
> (( result ))
> 
> ??

I simply missed that in the pre-posting review.  I'll change it before committing.

Thanks for the review, Bart.

Daniel


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: zmathfunc: min, max, sum throw error if result equals 0
  2021-04-16 18:21   ` Daniel Shahaf
@ 2021-05-16 14:54     ` Lawrence Velázquez
  2021-05-18  2:02       ` Daniel Shahaf
  0 siblings, 1 reply; 13+ messages in thread
From: Lawrence Velázquez @ 2021-05-16 14:54 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers

Hi Daniel,

On Fri, Apr 16, 2021, at 2:21 PM, Daniel Shahaf wrote:
> Daniel Shahaf wrote on Sun, Mar 07, 2021 at 17:17:12 +0000:
> > However, the examples in zshbuiltins(1) (under `functions -M`) have the
> > same bug.  Would you perchance be interested in fixing those?  The
> > manual sources are in Doc/Zsh/*.yo.
> 
> Patch series attached.

Were you still planning on pushing these?

vq


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: zmathfunc: min, max, sum throw error if result equals 0
  2021-05-16 14:54     ` Lawrence Velázquez
@ 2021-05-18  2:02       ` Daniel Shahaf
  0 siblings, 0 replies; 13+ messages in thread
From: Daniel Shahaf @ 2021-05-18  2:02 UTC (permalink / raw)
  To: Lawrence Velázquez; +Cc: zsh-workers

Lawrence Velázquez wrote on Sun, May 16, 2021 at 10:54:13 -0400:
> Hi Daniel,
> 
> On Fri, Apr 16, 2021, at 2:21 PM, Daniel Shahaf wrote:
> > Daniel Shahaf wrote on Sun, Mar 07, 2021 at 17:17:12 +0000:
> > > However, the examples in zshbuiltins(1) (under `functions -M`) have the
> > > same bug.  Would you perchance be interested in fixing those?  The
> > > manual sources are in Doc/Zsh/*.yo.
> > 
> > Patch series attached.
> 
> Were you still planning on pushing these?

Pushed, with the extraneous whitespace change in 0004 reverted.  Thanks
once again, Lawrence.

Daniel


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-05-18  2:02 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-07 16:37 zmathfunc: min, max, sum throw error if result equals 0 Nikolaus Thiel
2021-03-07 17:17 ` Daniel Shahaf
2021-03-07 21:39   ` Bart Schaefer
2021-03-07 21:56     ` Daniel Shahaf
2021-03-08  2:25       ` Bart Schaefer
2021-04-16 18:26         ` Daniel Shahaf
2021-04-16 18:47           ` Bart Schaefer
2021-04-16 19:50         ` Daniel Shahaf
2021-04-16 20:05           ` Bart Schaefer
2021-04-16 20:08             ` Daniel Shahaf
2021-04-16 18:21   ` Daniel Shahaf
2021-05-16 14:54     ` Lawrence Velázquez
2021-05-18  2:02       ` Daniel Shahaf

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).