Dpkg::Version: Fix bool overload behavior

The current bool overload has broken semantics, because it considers the
version "0" to be false.

The bool overload used to have sane semantics (equivalent to is_valid())
before commit 5b9f353b2940de751df47036608afbe71992d622, but there it got
changed to return the stringified version if it was valid, or undef
otherwise, to fix a problem within dpkg-shlibdeps, instead of properly
fixing the local-only problem in the tool. This makes the overload hard
to use, and broke existing callers from external projects.

We will emit a warning until dpkg 1.20.x to notify of the semantic change
in case there is code relying on the broken semantics. For fixed code the
warning can then be quiesced with:

  no warnings qw(Dpkg::Version::semantic_change::overload::bool);

Closes: #895004

5 files changed, 32 insertions, 13 deletions

diff --git a/debian/changelog b/debian/changelog
index 5098e7f2d..56b4c9e12 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -89,6 +89,10 @@ dpkg (1.19.1) UNRELEASED; urgency=medium
     - Dpkg::Vendor::Debian: Mark riscv64 as having gcc builtin PIE.
     - Dpkg::Shlibs::Objdump: Fix ELF program detection, for PIE binaries and
       executable libraries.
+    - Dpkg::Version: Fix bool overload behavior back to be an is_valid()
+      alias. Emit a specific perl warning until 1.20.x so that users can check
+      whether the semantic change has any impact on the code, which can then
+      be quiesced. Closes: #895004
   * Documentation:
     - Update gettext minimal version in README.
     - Add a missing dot on the dpkg-buildflags(1) «lfs» feature paragraph.
diff --git a/scripts/Dpkg/Version.pm b/scripts/Dpkg/Version.pm
index 477082b67..a06ba268d 100644
--- a/scripts/Dpkg/Version.pm
+++ b/scripts/Dpkg/Version.pm
@@ -20,6 +20,7 @@ package Dpkg::Version;
 
 use strict;
 use warnings;
+use warnings::register qw(semantic_change::overload::bool);
 
 our $VERSION = '1.01';
 our @EXPORT = qw(
@@ -55,7 +56,12 @@ use overload
     '<=>' => \&_comparison,
     'cmp' => \&_comparison,
     '""'  => sub { return $_[0]->as_string(); },
-    'bool' => sub { return $_[0]->as_string() if $_[0]->is_valid(); },
+    'bool' => sub {
+        warnings::warnif('Dpkg::Version::semantic_change::overload::bool',
+                         'bool overload behavior has changed back to be ' .
+                         'an is_valid() alias');
+        return $_[0]->is_valid();
+    },
     'fallback' => 1;
 
 =encoding utf8
@@ -121,8 +127,16 @@ sub new {
 =item boolean evaluation
 
 When the Dpkg::Version object is used in a boolean evaluation (for example
-in "if ($v)" or "$v || 'default'") it returns its string representation
-if the version stored is valid ($v->is_valid()) and undef otherwise.
+in "if ($v)" or "$v ? \"$v\" : 'default'") it returns true if the version
+stored is valid ($v->is_valid()) and false otherwise.
+
+B<Notice>: Between dpkg 1.15.7.2 and 1.19.1 this overload used to return
+$v->as_string() if $v->is_valid(), a breaking change in behavior that caused
+"0" versions to be evaluated as false. To catch any possibly intended code
+that relied on those semantics, this overload will emit a warning with
+category "Dpkg::Version::semantic_change::overload::bool" until dpkg 1.20.x.
+Once fixed, or for already valid code the warning can be quiesced with
+C<no warnings qw(Dpkg::Version::semantic_change::overload::bool)>.
 
 =item $v->is_valid()
 
diff --git a/scripts/dpkg-shlibdeps.pl b/scripts/dpkg-shlibdeps.pl
index 323fba9e1..4b18545e9 100755
--- a/scripts/dpkg-shlibdeps.pl
+++ b/scripts/dpkg-shlibdeps.pl
@@ -546,7 +546,8 @@ foreach my $field (reverse @depfields) {
 	    map {
 		# Translate dependency templates into real dependencies
 		my $templ = $_;
-		if ($dependencies{$field}{$templ}) {
+		if ($dependencies{$field}{$templ}->is_valid() and
+		    $dependencies{$field}{$templ}->as_string()) {
 		    $templ =~ s/#MINVER#/(>= $dependencies{$field}{$templ})/g;
 		} else {
 		    $templ =~ s/#MINVER#//g;
diff --git a/scripts/t/Dpkg_Version.t b/scripts/t/Dpkg_Version.t
index 78db7aead..17e9b4b9e 100644
--- a/scripts/t/Dpkg_Version.t
+++ b/scripts/t/Dpkg_Version.t
@@ -20,6 +20,7 @@ use Test::More;
 
 use Dpkg::ErrorHandling;
 use Dpkg::IPC;
+use Dpkg::Version;
 
 report_options(quiet_warnings => 1);
 
@@ -30,7 +31,7 @@ my @ops = ('<', '<<', 'lt',
 	   '>=', 'ge',
 	   '>', '>>', 'gt');
 
-plan tests => scalar(@tests) * (3 * scalar(@ops) + 4) + 30;
+plan tests => scalar(@tests) * (3 * scalar(@ops) + 4) + 27;
 
 sub dpkg_vercmp {
      my ($a, $cmp, $b) = @_;
@@ -57,8 +58,6 @@ sub obj_vercmp {
      return $a gt $b if $cmp eq 'gt';
 }
 
-use_ok('Dpkg::Version');
-
 my $truth = {
     '-1' => {
 	'<<' => 1, 'lt' => 1,
@@ -83,6 +82,12 @@ my $truth = {
     },
 };
 
+# XXX: Some of the tests check the bool overload, which currently emits
+# the semantic_change warning. Disable it until we stop emitting the
+# warning in dpkg 1.20.x.
+## no critic (TestingAndDebugging::ProhibitNoWarnings)
+no warnings(qw(Dpkg::Version::semantic_change::overload::bool));
+
 # Handling of empty/invalid versions
 my $empty = Dpkg::Version->new('');
 ok($empty eq '', "Dpkg::Version->new('') eq ''");
@@ -128,12 +133,6 @@ ok(!$ver->is_native(), 'upstream version w/ revision is not native');
 $ver = Dpkg::Version->new('1.0-1.0-1');
 ok(!$ver->is_native(), 'upstream version w/ dash and revision is not native');
 
-# Other tests
-$ver = Dpkg::Version->new('1.2.3-4');
-is($ver || 'default', '1.2.3-4', 'bool eval returns string representation');
-$ver = Dpkg::Version->new('0');
-is($ver || 'default', 'default', 'bool eval of version 0 is still false...');
-
 # Comparisons
 foreach my $case (@tests) {
     my ($a, $b, $res) = split ' ', $case;
diff --git a/t/pod-spell.t b/t/pod-spell.t
index 6d3f7ffc9..111d592ca 100644
--- a/t/pod-spell.t
+++ b/t/pod-spell.t
@@ -83,6 +83,7 @@ modelines
 multiarch
 nocheck
 qa
+quiesced
 reportfile
 rfc822
 sig