From c4a74db010bee194363ae3856553d385c98e31e0 Mon Sep 17 00:00:00 2001 From: Kang-min Liu Date: Wed, 4 Sep 2019 17:58:53 +0900 Subject: [PATCH 1/8] add a failed test for a non-Int being a valid Int. This is originally identified by @Ovid as a bug in Type::Tiny::XS: If I call int($num) on a floating point number, that number is subsequently identified as an integer, even when it's not. I suspected this is because the pIOK flag is set, but this might be a red herring (more later). Here's sample code which fails on every version (even .001) of Type:Tiny::XS that I've tested. See: https://github.com/tobyink/p5-type-tiny-xs/issues/8 But it turns out it is also reproducble with Mouse::XS -- not with Mouse::PurePerl though. --- t/101_issues/Int.t | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 t/101_issues/Int.t diff --git a/t/101_issues/Int.t b/t/101_issues/Int.t new file mode 100644 index 00000000..96520b05 --- /dev/null +++ b/t/101_issues/Int.t @@ -0,0 +1,17 @@ +#!/usr/bin/perl +use strict; +use warnings; +use Test::More; +use Mouse::Util::TypeConstraints; + +my $Int = find_type_constraint 'Int'; + +my $num = 3.14; + +ok !$Int->check($num), "\$num (== 3.14) is not Int"; + +{ no warnings; int($num) }; + +ok !$Int->check($num), "\$num (== 3.14) is still not Int"; + +done_testing; From c25a065f4edf7c2699e63cb71edf2e15edb325df Mon Sep 17 00:00:00 2001 From: Kang-min Liu Date: Wed, 4 Sep 2019 21:52:39 +0900 Subject: [PATCH 2/8] Port the fix from Type::Tiny::XS See also: https://github.com/tobyink/p5-type-tiny-xs/commit/09c47a138e2318f77c3e95eeb307754aad43576d --- xs-src/MouseTypeConstraints.xs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/xs-src/MouseTypeConstraints.xs b/xs-src/MouseTypeConstraints.xs index fd581875..75a67e50 100644 --- a/xs-src/MouseTypeConstraints.xs +++ b/xs-src/MouseTypeConstraints.xs @@ -172,12 +172,12 @@ mouse_tc_Int(pTHX_ SV* const data PERL_UNUSED_DECL, SV* const sv) { int const num_type = grok_number(SvPVX(sv), SvCUR(sv), NULL); return num_type && !(num_type & IS_NUMBER_NOT_INT); } - else if(SvIOKp(sv)){ - return TRUE; - } else if(SvNOKp(sv)) { return S_nv_is_integer(aTHX_ SvNVX(sv)); } + else if(SvIOKp(sv)){ + return TRUE; + } return FALSE; } From d31293094a2db543fa4a440956aae5647b71370d Mon Sep 17 00:00:00 2001 From: Kang-min Liu Date: Wed, 4 Sep 2019 22:02:27 +0900 Subject: [PATCH 3/8] expload the test cases for Int ... with different kinds of ways to initialize a scalar varible with "number" inside. --- t/101_issues/Int.t | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/t/101_issues/Int.t b/t/101_issues/Int.t index 96520b05..a1d4a4b4 100644 --- a/t/101_issues/Int.t +++ b/t/101_issues/Int.t @@ -2,16 +2,37 @@ use strict; use warnings; use Test::More; +use Devel::Peek qw; use Mouse::Util::TypeConstraints; my $Int = find_type_constraint 'Int'; -my $num = 3.14; +subtest "Non-Int from numerical literal: my \$num = 3.14", sub { + my $num = 3.14; + ok !$Int->check($num), "\$num is not Int"; + { no warnings; int($num) }; + ok !$Int->check($num), "\$num is still not Int"; +}; -ok !$Int->check($num), "\$num (== 3.14) is not Int"; +subtest "Non-Int from string literal: my \$num = \"3.14\"", sub { + my $num = "3.14"; + ok !$Int->check($num), "\$num is not Int"; + { no warnings; int($num) }; + ok !$Int->check($num), "\$num is still not Int"; +}; -{ no warnings; int($num) }; +subtest "Int from string literal: my \$num = \"3\"", sub { + my $num = "3"; + ok $Int->check($num), "\$num is Int"; + { no warnings; int($num) }; + ok $Int->check($num), "\$num is still Int"; +}; -ok !$Int->check($num), "\$num (== 3.14) is still not Int"; +subtest "Int from integer literal: my \$num = 3", sub { + my $num = 3; + ok $Int->check($num), "\$num is Int"; + { no warnings; int($num) }; + ok $Int->check($num), "\$num is still Int"; +}; done_testing; From 03d48ef532eee881a5a41ca0053e52c7e01bf3d5 Mon Sep 17 00:00:00 2001 From: Kang-min Liu Date: Wed, 4 Sep 2019 22:24:00 +0900 Subject: [PATCH 4/8] Port the fix for large unsigned integers. As @haarg demonstrated in https://github.com/tobyink/p5-type-tiny-xs/pull/9, previous "fix" breaks for large unsigned integers. @haarg also pointed out that macro ended with "p" such as SvIOKp should be avoided, since that check the flag for ["non-public integer values"][1], and the one without p ("SvIOK") checks the flag for ["public integer values"][2] [1]: https://perl5.git.perl.org/perl.git/blob/HEAD:/sv.h#l369 [2]: https://perl5.git.perl.org/perl.git/blob/HEAD:/sv.h#l364 --- t/101_issues/Int.t | 7 +++++++ xs-src/MouseTypeConstraints.xs | 10 +++++----- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/t/101_issues/Int.t b/t/101_issues/Int.t index a1d4a4b4..d724ae7f 100644 --- a/t/101_issues/Int.t +++ b/t/101_issues/Int.t @@ -35,4 +35,11 @@ subtest "Int from integer literal: my \$num = 3", sub { ok $Int->check($num), "\$num is still Int"; }; +subtest "MAXUINT", sub { + my $maxuint = ~0; + ok $Int->check( $maxuint ), 'yes MAXUINT'; + my $as_string = sprintf '%f', $maxuint; + ok $Int->check( $maxuint ), 'yes MAXUINT after use as float'; +}; + done_testing; diff --git a/xs-src/MouseTypeConstraints.xs b/xs-src/MouseTypeConstraints.xs index 75a67e50..303a2731 100644 --- a/xs-src/MouseTypeConstraints.xs +++ b/xs-src/MouseTypeConstraints.xs @@ -168,16 +168,16 @@ S_nv_is_integer(pTHX_ NV const nv) { int mouse_tc_Int(pTHX_ SV* const data PERL_UNUSED_DECL, SV* const sv) { assert(sv); - if(SvPOKp(sv)){ + if(SvPOK(sv)){ int const num_type = grok_number(SvPVX(sv), SvCUR(sv), NULL); return num_type && !(num_type & IS_NUMBER_NOT_INT); } - else if(SvNOKp(sv)) { - return S_nv_is_integer(aTHX_ SvNVX(sv)); - } - else if(SvIOKp(sv)){ + else if(SvIOK(sv)){ return TRUE; } + else if(SvNOK(sv)) { + return S_nv_is_integer(aTHX_ SvNVX(sv)); + } return FALSE; } From 6d9d77901a0453764ce0ee792c7a9ed3ff5d3bc8 Mon Sep 17 00:00:00 2001 From: Kang-min Liu Date: Thu, 5 Sep 2019 09:49:42 +0900 Subject: [PATCH 5/8] Add another edge case, harvested p5p mailing list. See: https://www.nntp.perl.org/group/perl.perl5.porters/2019/09/msg256131.html --- t/101_issues/Int.t | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/t/101_issues/Int.t b/t/101_issues/Int.t index d724ae7f..7287309a 100644 --- a/t/101_issues/Int.t +++ b/t/101_issues/Int.t @@ -42,4 +42,12 @@ subtest "MAXUINT", sub { ok $Int->check( $maxuint ), 'yes MAXUINT after use as float'; }; +subtest "A value with number inside that would be computed as an integer in perl internal. \$str = \" 1 \"", sub { + my $str = " 1 "; + ok !$Int->check( $str ), 'Not Int'; + { no warnings; int($str) }; + ok !$Int->check( $str ), 'Still not Int'; + ok $str == 1, "\$str == 1"; +}; + done_testing; From 073a9b599370b09182509c4616758e8b2db833c8 Mon Sep 17 00:00:00 2001 From: Kang-min Liu Date: Sun, 8 Sep 2019 16:34:14 +0900 Subject: [PATCH 6/8] Revert "Add another edge case, harvested p5p mailing list." This reverts commit 6d9d77901a0453764ce0ee792c7a9ed3ff5d3bc8. It is unclear what kind of fixes are needed yet, we shall review this topic latter. --- t/101_issues/Int.t | 8 -------- 1 file changed, 8 deletions(-) diff --git a/t/101_issues/Int.t b/t/101_issues/Int.t index 7287309a..d724ae7f 100644 --- a/t/101_issues/Int.t +++ b/t/101_issues/Int.t @@ -42,12 +42,4 @@ subtest "MAXUINT", sub { ok $Int->check( $maxuint ), 'yes MAXUINT after use as float'; }; -subtest "A value with number inside that would be computed as an integer in perl internal. \$str = \" 1 \"", sub { - my $str = " 1 "; - ok !$Int->check( $str ), 'Not Int'; - { no warnings; int($str) }; - ok !$Int->check( $str ), 'Still not Int'; - ok $str == 1, "\$str == 1"; -}; - done_testing; From 21e33019a6311d011ff2466c0927419fb99a6fa9 Mon Sep 17 00:00:00 2001 From: Kang-min Liu Date: Sun, 8 Sep 2019 20:57:49 +0900 Subject: [PATCH 7/8] remove Devel::Peek from import. --- t/101_issues/Int.t | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/t/101_issues/Int.t b/t/101_issues/Int.t index d724ae7f..2ef53cea 100644 --- a/t/101_issues/Int.t +++ b/t/101_issues/Int.t @@ -2,10 +2,9 @@ use strict; use warnings; use Test::More; -use Devel::Peek qw; use Mouse::Util::TypeConstraints; -my $Int = find_type_constraint 'Int'; +my $Int = find_type_constraint 'Int'; subtest "Non-Int from numerical literal: my \$num = 3.14", sub { my $num = 3.14; From 84383325d2383650f45306f29a870b24ecef5ffa Mon Sep 17 00:00:00 2001 From: Kang-min Liu Date: Sun, 8 Sep 2019 21:55:50 +0900 Subject: [PATCH 8/8] Deal with 2 different kinds of magic SVs in tc_Int. This commit fix tests related to validate Int in tied array/scalar, on perl5.8 ... perl5.16. The 2 types of magic SVs are: 1. SvTYPE(sv) == SVt_PVMG 2. SvMAGIC(sv) It appears to me that, when they are assigned integer values, -- even if it's originated from integer literals -- there are no "public integer" inside these magical SV, only "private" ones. We have been using SvIOKp(sv) to probe the Integer-ness of an SV and that just happen to be seemingly the only ways to do so for magic SVs. --- xs-src/MouseTypeConstraints.xs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/xs-src/MouseTypeConstraints.xs b/xs-src/MouseTypeConstraints.xs index 303a2731..4ef92c39 100644 --- a/xs-src/MouseTypeConstraints.xs +++ b/xs-src/MouseTypeConstraints.xs @@ -168,6 +168,7 @@ S_nv_is_integer(pTHX_ NV const nv) { int mouse_tc_Int(pTHX_ SV* const data PERL_UNUSED_DECL, SV* const sv) { assert(sv); + if(SvPOK(sv)){ int const num_type = grok_number(SvPVX(sv), SvCUR(sv), NULL); return num_type && !(num_type & IS_NUMBER_NOT_INT); @@ -178,6 +179,10 @@ mouse_tc_Int(pTHX_ SV* const data PERL_UNUSED_DECL, SV* const sv) { else if(SvNOK(sv)) { return S_nv_is_integer(aTHX_ SvNVX(sv)); } + else if (SvOK(sv) && (SvMAGIC(sv) || SvTYPE(sv) == SVt_PVMG) && SvIOKp(sv)) { + return TRUE; + } + return FALSE; }