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

Fix an edge case of non-Int being wrongly validated as Int #103

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

gugod
Copy link

@gugod gugod commented Sep 4, 2019

This PR is basically a port of the issue #8 in Type::Tiny::XS repo as well as PR #9 to Mouse::XS.

The newly added test file includes the fail test cases provided by @Ovid and @haarg. I expend it to include some other scenarios too.

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.
... with different kinds of ways to initialize a scalar varible with
"number" inside.
As @haarg demonstrated in tobyink/p5-type-tiny-xs#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
@gugod gugod changed the title Fix an edge case of Int TypeConstraint would wrongly consider a non-Int scalar Int. Fix an edge case of non-Int being wrongly validated as Int Sep 4, 2019
@gugod
Copy link
Author

gugod commented Sep 6, 2019

https://metacpan.org/changes/distribution/Type-Tiny-XS#L9

Latest release of Type::Tiny::XS already include the same changes.

@skaji
Copy link
Member

skaji commented Sep 8, 2019

@gugod Can you take a look at CI failures?

@gugod
Copy link
Author

gugod commented Sep 8, 2019

@skaji I committed an extra edge case 6d9d779 that failed with Mouse::XS, but passed with Mouse::PurePerl. More described at #104

But now I realize that this becomes a release blocker... I will revert that commit so the rest of this PR can be reviewed and released.

This reverts commit 6d9d779.

It is unclear what kind of fixes are needed yet, we shall review this
topic latter.
@gugod
Copy link
Author

gugod commented Sep 8, 2019

https://travis-ci.org/gugod/p5-Mouse/builds/582260726

I tweaked travis config to run this branch with all stable versions.

It looks like this PR cannot be directly applied for perl5 versions older than perl-5.18. I'll see what I can do keep them supported.

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.
@gugod
Copy link
Author

gugod commented Sep 8, 2019

@skaji The latest commit fix the regressions. There were all related to magic SVs and evidently magic SVs effect SV flags differently in new perl versions. At the end, SvIOKp() is still used, but only on magic SVs, as I cloudn't find other slots that also provides states the "Integer-ness" of an SV.

passed for all perl versions: https://travis-ci.org/gugod/p5-Mouse/builds/582310868

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

Successfully merging this pull request may close these issues.

2 participants