search

Perl / perl5

🐪 The Perl programming language
https://dev.perl.org/perl5/
Other
1.96k stars 555 forks source link

BBC: Blead Breaks Math::Calc::Parser #19639

Open cjg-cguevara opened 2 years ago

cjg-cguevara commented 2 years ago

This is a bug report for perl from "Carlos Guevara" carlos@carlosguevara.com, generated with the help of perlbug 1.42 running under perl 5.35.11.

[Please describe your issue here]

BBC: Blead Breaks Math::Calc::Parser

Please see http://matrix.cpantesters.org/?dist=Math::Calc::Parser

[Please do not change anything below this line]

Flags: category=core severity=low

Site configuration information for perl 5.35.11:

Configured by cpan at Sun Apr 17 12:40:23 EDT 2022.

Summary of my perl5 (revision 5 version 35 subversion 11) configuration: Commit id: ba92e234c6d5f1dd98358dd39575ca6d1b00de7e Platform: osname=openbsd osvers=7.0 archname=OpenBSD.amd64-openbsd-thread-multi uname='openbsd cjg-openbsd7 7.0 generic#6 amd64 ' config_args='-des -Dprefix=~/bin/perl-blead -Dscriptdir=~/bin/perl-blead/bin -Dusedevel -Duse64bitall -Duseithreads' hint=recommended useposix=true d_sigaction=define useithreads=define usemultiplicity=define use64bitint=define use64bitall=define uselongdouble=undef usemymalloc=n default_inc_excludes_dot=define Compiler: cc='cc' ccflags ='-pthread -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include' optimize='-O2' cppflags='-pthread -fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include' ccversion='' gccversion='OpenBSD Clang 11.1.0' gccosandvers='' intsize=4 longsize=8 ptrsize=8 doublesize=8 byteorder=12345678 doublekind=3 d_longlong=define longlongsize=8 d_longdbl=define longdblsize=16 longdblkind=3 ivtype='long' ivsize=8 nvtype='double' nvsize=8 Off_t='off_t' lseeksize=8 alignbytes=8 prototype=define Linker and Libraries: ld='cc' ldflags ='-pthread -Wl,-E -fstack-protector-strong -L/usr/local/lib' libpth=/usr/lib /usr/local/lib libs=-lpthread -lm -lutil -lc perllibs=-lpthread -lm -lutil -lc libc=/usr/lib/libc.so.96.1 so=so useshrplib=false libperl=libperl.a gnulibc_version='' Dynamic Linking: dlsrc=dl_dlopen.xs dlext=so d_dlsymun=undef ccdlflags=' ' cccdlflags='-DPIC -fPIC ' lddlflags='-shared -fPIC -L/usr/local/lib -fstack-protector-strong'

@INC for perl 5.35.11: /home/cpan/bin/perl-blead/lib/site_perl/5.35.11/OpenBSD.amd64-openbsd-thread-multi /home/cpan/bin/perl-blead/lib/site_perl/5.35.11 /home/cpan/bin/perl-blead/lib/5.35.11/OpenBSD.amd64-openbsd-thread-multi /home/cpan/bin/perl-blead/lib/5.35.11

Environment for perl 5.35.11: HOME=/home/cpan LANG (unset) LANGUAGE (unset) LC_ALL=C LD_LIBRARY_PATH (unset) LOGDIR (unset) PATH=/home/cpan/bin/perl-blead/bin:/home/cpan/bin:/home/cpan/bin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/X11R6/bin:/usr/local/bin:/usr/local/sbin:/usr/games PERL_BADLANG (unset) SHELL=/usr/local/bin/bash

hvds commented 2 years ago

Sigh, comparing the test reports http://www.cpantesters.org/cpan/report/30680458-b661-11ec-96c2-d0a2df0dbbbc (pass) and http://www.cpantesters.org/cpan/report/b01bbe34-be71-11ec-9a61-cf6edf0dbbbc (fail), it looks likely this is being affected by the updates to Math::Big*.

I'll try to have a dig tomorrow.

jkeenan commented 2 years ago

When testing Math::Calc::Parser at ba92e234c6d5f1dd98358dd39575ca6d1b00de7e or later, I keep getting a hang at this point in its test suite:

t/00-report-prereqs.t .. ok   
t/ath.t ................ ok   
t/bignum.t ............. 1/? Deep recursion on subroutine "Math::BigInt::batan" at /tmp/IVkprm3GDA/lib/perl5/5.35.11/Math/BigInt.pm line 2858.

Among other things, this appears to stop Porting/bisect.pl in its tracks.

pjacklam commented 2 years ago

Being the maintainer of both the "bignum" and "Math-BigInt" distributions, I am looking into this to see if I can shed some light on what is going on.

hvds commented 2 years ago

For the record, the core issue here is both BigInt and BigFloat trying to defer to each other:

% ./perl -Ilib -Mbignum -E '
  say "BigInt upgrade: $Math::BigInt::upgrade";
  say "BigFloat downgrade: $Math::BigFloat::downgrade";
  my $x = 1;
  say 'type of $x: ', ref($x);
  local $SIG{__WARN__} = sub { die @_ };
  $x->batan();
'
BigInt upgrade: Math::BigFloat
BigFloat downgrade: Math::BigInt
type of $x: Math::BigInt
Deep recursion on subroutine "Math::BigInt::batan" at lib/Math/BigInt.pm line 2858.
%
Grinnz commented 2 years ago

FWIW, Math::Calc::Parser has a somewhat nonstandard setup for BigInt and BigFloat, emulating the bigint and bigrat pragmas as it can't use them directly. https://metacpan.org/dist/Math-Calc-Parser/source/lib/Math/Calc/Parser.pm#L145

This is also an optional feature of a non-critical module (I am the author) so apart from finding if a new issue has been introduced, should not block Perl in any way.

neilb commented 2 years ago

@pjacklam any news on this one please?

pjacklam commented 2 years ago

For the record, the core issue here is both BigInt and BigFloat trying to defer to each other:

% ./perl -Ilib -Mbignum -E '
  say "BigInt upgrade: $Math::BigInt::upgrade";
  say "BigFloat downgrade: $Math::BigFloat::downgrade";
  my $x = 1;
  say 'type of $x: ', ref($x);
  local $SIG{__WARN__} = sub { die @_ };
  $x->batan();
'
BigInt upgrade: Math::BigFloat
BigFloat downgrade: Math::BigInt
type of $x: Math::BigInt
Deep recursion on subroutine "Math::BigInt::batan" at lib/Math/BigInt.pm line 2858.
%

When done correctly, this is not a problem. Since atan() is a function that can produce a non-integer result, Math::BigInt::batan() should pass everything on to Math::BigFloat::batan(), which should compute the result, and when that is done – and if the result turns out to be an integer – the result should be downgraded to a Math.:BigInt. The deep recursion reveals a bug.

Consider the following:

Math::BigFloat -> downgrade("Math::BigInt");
Math::BigInt -> upgrade("Math::BigFloat");

$x = Math::BigFloat -> new("6");
$y = Math::BigFloat -> new("2");
$z = $x -> bdiv($y);

Even though Math::BigFloats are requested, two Math::BigInt objects are returned due to downgrading. Then Math::BigInt::bdiv() attempts to perform the division. Now, Math::BigInt::bdiv() knows that dividing $x by $y, where 1 < |$x|, |$y| < ∞, might produce a non-integer result, and since upgrading is enabled, everything is passed to Math::BigFloat::bdiv(). When Math::BigFloat::bdiv() has computed the result, it checks to see if the result is an integer, which it is in this case, and the result is downgraded to a Math::BigInt. Again, this should work fine without any deep recursion.

pjacklam commented 2 years ago

@pjacklam any news on this one please?

This has been more work than I expected. Upgrading and downgrading between Math::BigInt and Math::BigFloat seems to work fine now. The same with upgrading and downgrading between Math::BigInt and Math::BigRat. So the "bignum.t" test file passes now.

However, there are still some tests in "bigrat.t" that fail. That is presumably because Math::BigFloat used to upgrade to Math::BigRat in some cases. I fail to see the logic behind it. It seems arbitrary when Math::BigFloat would upgrade to Math::BigRat.

Math::BigInt upgrades to Math::BigFloat when the result of a computation can't be represented as an integer. That makes sense. However the same logic doesn't seem to apply to the upgrading in Math::BigFloat.

In any case, I believe the new code is better than the currently released code, and so hopefully I'll manage to make a new release today.

jkeenan commented 2 years ago

Today I installed perl from the v5.36.0-RC3 tag, threaded build on FreeBSD-12. I attempted to install Math::Calc::Parser, but got these (now familiar) failures in the test suite:

t/bignum.t           (Wstat: 9 (Signal: KILL) Tests: 24 Failed: 0)
  Non-zero wait status: 9
  Parse errors: No plan found in TAP output
t/bigrat.t           (Wstat: 65280 (exited 255) Tests: 20 Failed: 4)
  Failed tests:  2-3, 7, 11
  Non-zero exit status: 255
  Parse errors: No plan found in TAP output
pjacklam commented 2 years ago

I'm away until Sunday, so I can't look into this right now, but when I testes Math::Calc::Parser with the latest version of the Math-BigInt distribution, I only got failed test, if I remember correctly. I believe it was when testing 3 >> 1, which returns 1.5, but where the expected value is 1. I don't know which version of Math-BigInt that ships with 5.36.0-RC3.

Anyway, I will look into this as soon as I am back.

jkeenan commented 2 years ago

Here are results from an attempted installation against perl-5.36.0, built from the tarball released yesterday.

t/00-report-prereqs.t .. ok
t/ath.t ................ ok
Deep recursion on subroutine "Math::BigInt::batan" at /home/jkeenan/testing/v5.36.0/lib/perl5/5.36.0/Math/BigInt.pm line 2858.
t/bignum.t ............. 
All 24 subtests passed 

#   Failed test 'Evaluated 3/9'
#   at t/bigrat.t line 14.
#          got: '0.3333333333333333333333333333333333333333'
#     expected: '1/3'

#   Failed test 'Evaluated 3>>2'
#   at t/bigrat.t line 16.
#          got: '0.75'
#     expected: '3/4'

#   Failed test 'Division'
#   at t/bigrat.t line 23.
#          got: '1.5'
#     expected: '3/2'

#   Failed test 'Right shift'
#   at t/bigrat.t line 27.
#          got: '1.5'
#     expected: '3/2'
Error in function "round": Can't call Math::BigFloat->as_float, not a valid method
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 255 just after 20.
t/bigrat.t ............. 
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 4/20 subtests 
t/errors.t ............. ok
t/eval.t ............... ok
t/functions.t .......... ok
t/operators.t .......... ok
t/parse.t .............. ok

Test Summary Report
-------------------
t/bignum.t           (Wstat: 9 (Signal: KILL) Tests: 24 Failed: 0)
  Non-zero wait status: 9
  Parse errors: No plan found in TAP output
t/bigrat.t           (Wstat: 65280 (exited 255) Tests: 20 Failed: 4)
  Failed tests:  2-3, 7, 11
  Non-zero exit status: 255
  Parse errors: No plan found in TAP output
Files=9, Tests=236, 439 wallclock secs ( 0.05 usr  0.55 sys + 307.84 cusr 22.86 csys = 331.30 CPU)
Result: FAIL
Failed 2/9 test programs. 4/236 subtests failed.
*** Error code 255

Stop.
make: stopped in /usr/home/jkeenan/.cpan/build/Math-Calc-Parser-1.005-0
(/usr/bin/make test exited with 256)
[snip]

  DBOOK/Math-Calc-Parser-1.005.tar.gz
  /usr/bin/make test -- NOT OK
pjacklam commented 2 years ago

It looks like perl-5.36.0 is not shipped with the latest release of Math-BigInt. What do you get when you run the tests with the latest Math-BigInt?

jkeenan commented 2 years ago

It looks like perl-5.36.0 is not shipped with the latest release of Math-BigInt. What do you get when you run the tests with the latest Math-BigInt?

$ ./bin/perl -Ilib -MMath::BigInt -E 'say $Math::BigInt::VERSION;'
1.999835
$ ./bin/cpan -i Math::Calc::Parser
...
t/00-report-prereqs.t .. ok
t/ath.t ................ ok

#   Failed test 'Right shift'
#   at t/bignum.t line 29.
#          got: '1.5'
#     expected: '1'
# Looks like you failed 1 test of 36.
t/bignum.t ............. 
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/36 subtests 
Can't call Math::BigFloat->_e_add, not a valid method at /usr/home/jkeenan/.cpan/build/Math-Calc-Parser-1.005-1/blib/lib/Math/Calc/Parser.pm line 385.
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 255 just after 1.
t/bigrat.t ............. 
Dubious, test returned 255 (wstat 65280, 0xff00)
All 1 subtests passed 
t/errors.t ............. ok
t/eval.t ............... ok
t/functions.t .......... ok
t/operators.t .......... ok
t/parse.t .............. ok

Test Summary Report
-------------------
t/bignum.t           (Wstat: 256 (exited 1) Tests: 36 Failed: 1)
  Failed test:  10
  Non-zero exit status: 1
t/bigrat.t           (Wstat: 65280 (exited 255) Tests: 1 Failed: 0)
  Non-zero exit status: 255
  Parse errors: No plan found in TAP output
Files=9, Tests=229,  1 wallclock secs ( 0.05 usr  0.02 sys +  1.34 cusr  0.12 csys =  1.52 CPU)
Result: FAIL
Failed 2/9 test programs. 1/229 subtests failed.
*** Error code 255
...
jkeenan commented 1 year ago

Attempting to install this today against blead (one year later than previously), I got a failure in one file:

$ uname -mrs
Linux 5.19.0-41-generic x86_64

$ bleadperl -v | head -2 | tail -1
This is perl 5, version 37, subversion 12 (v5.37.12 (v5.37.11-38-ga12bd18fcb)) built for x86_64-linux

$ bleadprove -vb t/bignum.t 
t/bignum.t .. 
ok 1 - Evaluated 2+2
...
ok 9 - Left shift
not ok 10 - Right shift

#   Failed test 'Right shift'
#   at t/bignum.t line 29.
#          got: '1.5'
#     expected: '1'
ok 11 - Factorial
...
ok 36 - Random number
1..36
# Looks like you failed 1 test of 36.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/36 subtests 

Test Summary Report
-------------------
t/bignum.t (Wstat: 256 (exited 1) Tests: 36 Failed: 1)
  Failed test:  10
  Non-zero exit status: 1
Files=1, Tests=36,  0 wallclock secs ( 0.01 usr  0.00 sys +  0.19 cusr  0.01 csys =  0.21 CPU)
Result: FAIL
sisyphus commented 1 year ago

I see the same with 5.37.11 on Windows. This test just reflects the behaviour of Math::BigFloat's overloading of '>>" which, I think, was changed in the not-too-distant past:

>perl -MMath::BigFloat -le "print Math::BigFloat->new(3) >> 1;"
1.5

So it's either a bug in Math::BigFloat, or the expectation of the failing test in Math::Calc::Parser needs to be changed to 1.5.

Having shift operations for NVs is questionable. When perl is given an operand that's an NV, then left or right shifts done on that operand will be done on int(op). M::BF, however, presently overloads the shift operators to simply multiply by 2 (for left shifts) or divide by 2 (for right shifts).

I think that M::BF was, at some point, altered to follow the same procedure as perl, but then subsequently changed back to its original (ie present) behaviour in response to user complaints about that initial alteration.

Personally, I would prefer that the failing M::C::P test be altered so that it passes. I think that the Math::BigFloat behaviour is pretty easy to comprehend once you understand that << and >> are just synonyms for multiply/divide by 2.

Afterthought: Perhaps the views of @pjacklam (current Math::BigFloat maintainer) should also be sought.

Cheers, Rob

hvds commented 1 year ago

Afterthought: Perhaps the views of @pjacklam (current Math::BigFloat maintainer) should also be sought.

@pjacklam's prior comment on the same issue here was:

Some years ago I changed Math::BigFloat's behaviour so that >>/brsft() and <</blsft() always returned an integer, but people were furious, so I changed it back.

But it's a shame, since it clearly is not uncommon that people are bitten by this.

Since it is shipped with perl, I guess it would be possible to have a lexically-scoped flag to choose the behaviour; but it isn't immediately obvious if that would make things better or just more confusing.

pjacklam commented 1 year ago

I see the same with 5.37.11 on Windows. This test just reflects the behaviour of Math::BigFloat's overloading of '>>" which, I think, was changed in the not-too-distant past:

>perl -MMath::BigFloat -le "print Math::BigFloat->new(3) >> 1;"
1.5

The behaviour shown above is how it has been since Math-BigInt-1.48 (released by TELS in 2001) with the exception of Math-BigInt-1.999718 (which I released in 2016). I immediately got so much negative feedback that I changed it back in Math-BigInt-1.999719. (I was declared completely incompetent and told to step down immediately as the maintainer of the Math-Big* distributions and whatnot.)

However, the change that is causing the issue with Math::Calc::Parser is more recent. It was introduced in Math-BigInt-1.999832. Before this release, the upgrading and downgrading was inconsistent. TELS' (the orginal author's) idea is that if the operands are Math::BigInt objects, and the operation might give a non-integer result, the operands are upgraded, if upgrading is enabled. And if the operands are Math::BigFloat objects, and the operation gives an integer result, the result is downgraded, if downgrading is enabled.

Unfortunately, the implementation was somewhat inconsistent. Some operations giving a non-integer would not upgrade, and some operations giving an integer would not downgrade. Since there didn't seem to be any pattern, I assumed that this inconsistency was unintentional and changed it to be consistent (although also I might have forgotten a corner case or two). So the following returns 1 before version 1.999832, but from version 1.999832, it returns 1.5:

perl -MMath::BigFloat -le '$Math::BigInt::upgrade = "Math::BigFloat"; print Math::BigInt->new(3) >> 1;'

Afterthought: Perhaps the views of @pjacklam (current Math::BigFloat maintainer) should also be sought.

I have no strong opinion on what the behaviour should be, but I like consistency. :-)

sisyphus commented 1 year ago

I have no strong opinion on what the behaviour should be, but I like consistency. :-)

Yes, me too ;-)

It would be interesting to come up with some documentation that details just what that consistency should look like. Is there any interest in airing ideas upon what that "consistency" is ? I have some views that I don't mind presenting, and I'd be interested in finding out what other ideas are out there. Where would be a good place to post about this ?

For example, this lacks consistency, IMO:

> perl -MMath::BigInt -MMath::BigFloat -le "print Math::BigInt->new(6) * Math::BigFloat->new(2.5);"
12

> perl -MMath::BigInt -MMath::BigFloat -le "print Math::BigFloat->new(2.5) * Math::BigInt->new(6);"
15

> perl -MMath::BigInt -MMath::BigFloat -le "print Math::BigInt->new(6) + Math::BigFloat->new(2.5);"
8

> perl -MMath::BigInt -MMath::BigFloat -le "print Math::BigFloat->new(2.5) + Math::BigInt->new(6);"
8.5

(I find it quite disturbing that the commutative laws of addition and multiplication have been broken.) With my own Math::GMPz and Math::MPFR modules, I have constructed it so that addition and multiplication are overloaded to return the same (Math::MPFR) object, irrespective of the order in which the 2 operands are provided. eg:

>perl -MMath::GMPz -MMath::MPFR -le "print Math::GMPz->new(6) * Math::MPFR->new(2.5);"
1.5e1

>perl -MMath::GMPz -MMath::MPFR -le "print Math::MPFR->new(2.5) * Math::GMPz->new(6);"
1.5e1

A similar situation arises with overloading of += and *= operations:

D:\> perl -MMath::BigInt -MMath::BigFloat -le "$x = Math::BigFloat->new(2.5); $y = Math::BigInt->new(6); $x *= $y; print $x;"
15

D:\> perl -MMath::BigInt -MMath::BigFloat -le "$x = Math::BigFloat->new(2.5); $y = Math::BigInt->new(6); $y *= $x; print $y;"
12

I haven't fully addressed this cross-class overloading of += and *+ wrt Math::GMPz and Math::MPFR:

D:\>perl -MMath::GMPz -MMath::MPFR -le "$x = Math::GMPz->new(6); $y = Math::MPFR->new(2.5); $x *= $y; print $x"
Invalid argument supplied to Math::GMPz::overload_mul_eq at -e line 1.

D:\>perl -MMath::GMPz -MMath::MPFR -le "$x = Math::GMPz->new(6); $y = Math::MPFR->new(2.5); $y *= $x; print $y;"
1.5e1

Which reminds me that I could maybe fix that .... assuming it is possible to construct it so that when I do$GMPz_object *= $MPFR_object, $GMPz_object changes from a Math::GMPz object into a Math::MPFR object. (That also assumes that such a transformation would not cause confusion ... and I'm unsure about that.)

Cheers, Rob

pjacklam commented 1 year ago

It would be interesting to come up with some documentation that details just what that consistency should look like.

If we limit ourselves to upgrading and downgrading: The documentation in Math::BigInt or Math::BigFloat didn't describe precisely how the upgrading and downgrading should work. I based my understanding on the documentation there was, the comments in the code, and the actual behaviour.