Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type::Tiny::XS identifies non-ints as ints #8

Closed
Ovid opened this issue Aug 29, 2019 · 10 comments
Closed

Type::Tiny::XS identifies non-ints as ints #8

Ovid opened this issue Aug 29, 2019 · 10 comments

Comments

@Ovid
Copy link

Ovid commented Aug 29, 2019

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.

This code replicates the issue:

#!/usr/bin/env perl

use Test::More;
use Type::Tiny::XS;
use Devel::Peek;

diag $Type::Tiny::XS::VERSION;

my $num = 3.14;

Dump($num);
ok !Type::Tiny::XS::Int($num), "$num should not be an integer";
Dump($num);
{ no warnings; int($num); }
Dump($num);
ok !Type::Tiny::XS::Int($num), "$num should not be an integer";
Dump($num);

done_testing;

And the output:

type_tiny.t .. # 0.014
SV = NV(0x7fb6e4903088) at 0x7fb6e49030a0
  REFCNT = 1
  FLAGS = (NOK,pNOK)
  NV = 3.14

ok 1 - 3.14 should not be an integer
not ok 2 - 3.14 should not be an integer
SV = PVNV(0x7fb6e48027f0) at 0x7fb6e49030a0
  REFCNT = 1
  FLAGS = (NOK,pNOK)
  IV = 0
  NV = 3.14
  PV = 0x7fb6e45df090 "3.14"\0
  CUR = 4
  LEN = 32
SV = PVNV(0x7fb6e48027f0) at 0x7fb6e49030a0
  REFCNT = 1
  FLAGS = (NOK,pIOK,pNOK)
  IV = 3
  NV = 3.14
  PV = 0x7fb6e45df090 "3.14"\0
  CUR = 4
  LEN = 32

#   Failed test '3.14 should not be an integer'
#   at type_tiny.t line 16.
SV = PVNV(0x7fb6e48027f0) at 0x7fb6e49030a0
  REFCNT = 1
  FLAGS = (NOK,pIOK,pNOK)
  IV = 3
  NV = 3.14
  PV = 0x7fb6e45df090 "3.14"\0
  CUR = 4
  LEN = 32
1..2
# Looks like you failed 1 test of 2.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/2 subtests

Test Summary Report
-------------------
type_tiny.t (Wstat: 256 Tests: 2 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=1, Tests=2,  0 wallclock secs ( 0.01 usr  0.01 sys +  0.06 cusr  0.01 csys =  0.09 CPU)
Result: FAIL

And I suspect this is the cause of the same int bug reported by Peter Mottram. However, all versions of Perl we've tested have pIOK set and this might be a red herring.

I found the problem in a large codebase and reduced it to this:

perl -MTypes::Standard=is_Int -E \
    'my $num = 3.14; say is_Int($num)?"Yes":"No"; int($num); say is_Int($num)?"Yes":"No";say $num'

On my box, that shows:

No
Yes
3.14

But one of our developers did this:

Intriguing -- I just brewed a plain perl-5.26.3, installed Types::Standard, and it passes -- here's the installations I've got:

% perlbrew exec --with perl-5.26.3,perl-5.26.3-veure,perl-5.28.1,perl-5.26.2,perl-5.26.0,perl-5.25.1,perl-5.24.2,perl-5.22.1 perl -MTypes::Standard=is_Int -E 'say "Types::Standard " . $Types::Standard::VERSION; my $num = 3.14; say is_Int($num)?"Yes":"No"; int($num); say is_Int($num)?"Yes":"No";say $num'
perl-5.26.3
==========
Types::Standard 1.004004
No
No
3.14

perl-5.26.3-veure
==========
Types::Standard 1.004004
No
Yes
3.14

perl-5.28.1
==========
Types::Standard 1.004002
No
No
3.14

perl-5.26.2
==========
Types::Standard 1.004004
No
No
3.14

perl-5.26.0
==========
Types::Standard 1.004004
No
No
3.14

perl-5.25.1
==========
Types::Standard 1.004004
No
No
3.14

perl-5.24.2
==========
Types::Standard 1.002001
No
Yes
3.14

perl-5.22.1
==========
Types::Standard 1.000005
No
No
3.14

So I also asked my Twitter followers to run that Types::Standard snippet and they received similarly inconsistent results.

So I don't know what's causing it, but at least I have a small test case for you in the first code snippet I posted above. I hope it helps because this is a serious issue in our current codebase.

@Ovid
Copy link
Author

Ovid commented Aug 29, 2019

Also, it's possible that some of those versions don't have Type::Tiny::XS installed. One of our devs asked (via company Slack channel):

Do all of those have Type::Tiny::XS installed? Pure Perl TT is OK for all my Perls but XS fails on what seems to be a random selection of versions of Perl

And the dev who ran the above confirmed that it appears to be related to Type::Tiny::XS.

@Tekki
Copy link

Tekki commented Aug 30, 2019

I've reported on Twitter that with Perl versions 5.10, 16, 20, 22, 24, 26, 28, 30 from Dockerhub I get 'No / No / 3.14'. But when I install Type::Tiny::XS the result is 'No / Yes / 3.14' on all of them.

@tobyink
Copy link
Owner

tobyink commented Sep 3, 2019

Interesting. Pretty sure I stole the Int check from Mouse's XS, so I do wonder if the same bug exists there. Anybody got any thoughts about the best way to do an int check in XS code?

@Ovid
Copy link
Author

Ovid commented Sep 3, 2019

Not sure, sorry. You might be able to do this:

int is_IV(SV * maybe_num) {
    if(!SvIOK(maybe_num)) return 0;
    // extra logic here, but a naïve return 1 works
    // if SvIOK really checks if it's an int (see below)
}

But I suspect that might return true if the pIOK flag is set. Maybe check if SvIOK(maybe_num) is true and if so, check if SvNOK(maybe_num) is true and see if the slots have the same value? No idea if that will work or not.

tobyink added a commit that referenced this issue Sep 3, 2019
@Ovid
Copy link
Author

Ovid commented Sep 3, 2019

Update: I've posted to P5P asking for help. Dave Mitchell and demerphq have been discussing this prior to that. Hopefully something will come from that.

@tobyink
Copy link
Owner

tobyink commented Sep 3, 2019

I tried stealing code from the latest Mouse release, but that seems even worse, allowing 123\n and \n123 to pass as integers.

@haarg
Copy link
Contributor

haarg commented Sep 3, 2019

This really should not be checking the p flags. The check that makes the most sense to me would be:

        if(SvIOK(sv)){
            return TRUE;
        }
        else if(SvNOK(sv)) {
            return S_nv_is_integer(aTHX_ SvNVX(sv));
        }
        else if(SvPOK(sv)){
            return S_pv_is_integer(aTHX_ SvPVX(sv));
        }

@haarg
Copy link
Contributor

haarg commented Sep 3, 2019

Actually, that would allow through some strings like "0e0". So to be consistent with the perl implementation, I think the check should be:

        if(SvPOK(sv)){
            return S_pv_is_integer(aTHX_ SvPVX(sv));
        }
        else if(SvIOK(sv)){
            return TRUE;
        }
        else if(SvNOK(sv)) {
            return S_nv_is_integer(aTHX_ SvNVX(sv));
        }

This is basically the same as what is in the released version, except not using the p flags.

@xenu
Copy link
Contributor

xenu commented Sep 3, 2019

I'm actually surprised that SV containing 0e0 can have IOK flag. It seems that perl 5.6 was the last perl version that did not do this: https://perl.bot/p/7eefti

@tobyink
Copy link
Owner

tobyink commented Sep 3, 2019

I've pushed out 0.015 which fixes the reported issue and doesn't seem to cause any other problems. Whether or not Type::Tiny::XS should be checking the p flags is an issue for another day.

@tobyink tobyink closed this as completed Sep 3, 2019
gugod added a commit to gugod/p5-Mouse that referenced this issue Sep 4, 2019
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: tobyink/p5-type-tiny-xs#8

But it turns out it is also reproducble with Mouse::XS -- not with
Mouse::PurePerl though.
clrpackages pushed a commit to clearlinux-pkgs/perl-Type-Tiny-XS that referenced this issue Oct 14, 2019
0.016	2019-09-05

 [ Bug Fixes ]
 - Fix Int check on large unsigned integers.
   Graham Knop++
   <tobyink/p5-type-tiny-xs#9>

0.015	2019-09-03

 [ Bug Fixes ]
 - Casting a non-integer number to an integer elsewhere mistakenly caused
   the Int check to think the non-integer was an integer.
   Curtis "Ovid" Poe++
   <tobyink/p5-type-tiny-xs#8>
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Nov 2, 2019
0.016   2019-09-05
 [ Bug Fixes ]
  - Fix Int check on large unsigned integers.
    Graham Knop++
   <tobyink/p5-type-tiny-xs#9>

0.015   2019-09-03
 [ Bug Fixes ]
  - Casting a non-integer number to an integer elsewhere mistakenly caused
    the Int check to think the non-integer was an integer.
    Curtis "Ovid" Poe++
    <tobyink/p5-type-tiny-xs#8>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants