Perl-Toolchain-Gang / CPAN-Meta-Requirements

a set of version requirements for a CPAN dist
5 stars 15 forks source link

"0.00" and "!= 0.00" now simplified as "0" and "!= 0" respectively. #18

Closed kentfredric closed 9 years ago

kentfredric commented 9 years ago

I'm not entirely sure where this lies in bug department. I stumbled on to it as it made one of @SineSwiper's dists fail

https://metacpan.org/source/BBYRD/Dist-Zilla-PluginBundle-Prereqs-0.93/t/01-MinimumPrereqs.t#L30

Its rather odd because "1.00" preserves the trailing part. And its not clear if this is an oversight in that CMeta is not retaining data, or its a bug in @SineSwiper's dist being too generous about testing for what "0.00" means.

Test code that is really simple to express this failure:

use strict;
use warnings;

use Test::More tests => 1;
use CPAN::Meta::Requirements;

my $source = CPAN::Meta::Requirements->new();
$source->add_string_requirement('XX' => '0.00');
$source->add_string_requirement('YY' => '!= 0.00');
$source->add_string_requirement('ZZ' => '1.00');
$source->add_string_requirement('aa' => '!= 1.00');

my $merged = CPAN::Meta::Requirements->new();
$merged->add_requirements( $source );
is_deeply(
  $merged->as_string_hash,
  $source->as_string_hash,
  "Structures are the same"
);
note explain {
  source => $source->as_string_hash,
  merged => $merged->as_string_hash
};

Output:

1..1
not ok 1 - Structures are the same
#   Failed test 'Structures are the same'
#   at /tmp/cmeta.pl line 17.
#     Structures begin differing at:
#          $got->{XX} = '0'
#     $expected->{XX} = '0.00'
# {
#   'merged' => {
#     'XX' => '0',
#     'YY' => '!= 0',
#     'ZZ' => '1.00',
#     'aa' => '!= 1.00'
#   },
#   'source' => {
#     'XX' => '0.00',
#     'YY' => '!= 0.00',
#     'ZZ' => '1.00',
#     'aa' => '!= 1.00'
#   }
# }
dagolden commented 9 years ago

What version of CPAN::Meta::Requirements and version.pm?

kentfredric commented 9 years ago

So far tested and replicating this issue: perl = 5.21.8, 5.21.8+leontpatch version.pm = 0.9911, 0.9912 CPAN::Meta::Requirements = 2.131

kentfredric commented 9 years ago

Happens all the way back to version.pm 0.9906 . However, it doesn't happen on CMR 2.130 , even with version.pm 0.9912

kentfredric commented 9 years ago

Ahahahaa.


use overload qw();

my $x = version->new("0.00");

if ( overload::Method( $x, "eq" ) ) { # false
    print "eq is overloaded\n"; 
}
if ( overload::Method( $x, "cmp" ) ) { # True! 
    print "cmp is overloaded\n";
}
if ( not defined $x ) { # false
    print "X is not defined\n";
}

if ( $x eq "0" ) { # This is true! <----------------------------__!!!!!!!!
    print "String equal\n";
}

if ( "$x" eq "0" ) { # False
    print "Stringify string equal\n";
}

if ( "0.00" eq "0" ) { # false
    print "Perl is on drugs\n";
}

   eval {
-    local $SIG{__WARN__} = sub { die "Invalid version: $_[0]" };
-    $vobj  = (! defined $version)                ? version->new(0)
-           : (! Scalar::Util::blessed($version)) ? version->new($version)
-           :                                       $version;
+    if (not defined $version or $version eq '0') {
+      $vobj = $V0;
+    }
+    elsif ( ref($version) eq 'version' || _isa_version($version) ) {
+      $vobj = $version;
+    }
+    else {
+      local $SIG{__WARN__} = sub { die "Invalid version: $_[0]" };
+      $vobj = version->new($version);
+    }
   };
dagolden commented 9 years ago

I figured it related to that. That's... annoying.

I'll amend the logic to avoid that.

dagolden commented 9 years ago

I'd someone else to check this change and comment before I ship it to CPAN.

SineSwiper commented 9 years ago

Neat find, @kentfredric. Is this an 'eq' overload problem with version?

@dagolden, I don't really see anything wrong with the patch here.

kentfredric commented 9 years ago

@SineSwiper its more an unfortunate side effect of version.pm being a bit weird.

"0.00" eq "0" # false
# Under the hood, is pretty much version->new("0.00")->eq("0")
version->new("0.00") eq "0" # true

Because "0" and "0.00" for the purpose of version comparisons are the same.

And so this optimisation to short-circuit "If it equals 0" loses context in the source data unintentionally.