fix: resolve -1 sentinel collision for NV ops (-M, -C, -A)

FileCheck.xs

@@ -119,14 +119,25 @@ int _overload_ft_ops(pTHX) {
   return check_status;
 }
 
-SV* _overload_ft_ops_sv(pTHX) {
+/*
+* NV-specific helper for -M, -C, -A ops.
+*
+* _check() returns a (status, value) pair for NV ops to avoid the -1
+* sentinel collision: FALLBACK_TO_REAL_OP is -1, but -1.0 is a valid
+* NV result (file modified exactly 1 day in the future).
+*
+* Returns:
+*   *status_out = -1  -> FALLBACK_TO_REAL_OP (nv_out is unused)
+*   *status_out = -2  -> CHECK_IS_NULL / undef (nv_out is unused)
+*   *status_out =  1  -> success, *nv_out has the value
+*/
+void _overload_ft_ops_nv(pTHX_ int *status_out, NV *nv_out) {
   dMY_CXT;
   SV *const arg = *PL_stack_sp;
-  int optype = PL_op->op_type;  /* this is the current op_type we are mocking */
-  SV *status;        /* 1 -> YES ; 0 -> FALSE ; -1 -> delegate */
+  int optype = PL_op->op_type;
+  int count;
 
   dSP;
-  int count;
 
   ENTER;
   SAVETMPS;
@@ -138,21 +149,39 @@ SV* _overload_ft_ops_sv(pTHX) {
 
   PUTBACK;
 
-  count = call_pv("Overload::FileCheck::_check", G_SCALAR);
+  count = call_pv("Overload::FileCheck::_check", G_ARRAY);
 
   SPAGAIN;
 
-  if (count != 1)
+  if (count < 1)
     croak("No return value from Overload::FileCheck::_check for OP #%d\n", optype);
 
-  status = POPs;
-  SvREFCNT_inc( status );
+  if (count == 1) {
+    /* Single return: FALLBACK_TO_REAL_OP or CHECK_IS_NULL */
+    SV *sv = POPs;
+    if (!SvOK(sv))
+      *status_out = -2;  /* undef => CHECK_IS_NULL */
+    else
+      *status_out = SvIV(sv);  /* -1 for FALLBACK, -2 for NULL */
+    *nv_out = 0;
+  }
+  else if (count == 2) {
+    /* Pair return: (status_code, nv_value) */
+    SV *value_sv = POPs;
+    SV *status_sv = POPs;
+    *status_out = SvIV(status_sv);
+    *nv_out = SvNV(value_sv);
+  }
+  else {
+    /* Pop excess values to avoid stack corruption */
+    int orig_count = count;
+    while (count-- > 0) (void)POPs;
+    croak("Overload::FileCheck::_check returned %d values for NV OP #%d, expected 1 or 2\n", orig_count, optype);
+  }
 
-  OFC_DEBUG("_overload_ft_ops_sv: optype=%d\n", optype);
+  OFC_DEBUG("_overload_ft_ops_nv: status=%d optype=%d\n", *status_out, optype);
 
   LEAVE_PRESERVING_ERRNO();
-
-  return status;
 }
 
 /*
@@ -326,7 +355,8 @@ PP(pp_overload_ft_int) {
 
 PP(pp_overload_ft_nv) {
   dMY_CXT;
-  SV *status;
+  int check_status;
+  NV nv_value;
   int saved_errno;
 
   if (!gl_overload_ft)
@@ -336,36 +366,28 @@ PP(pp_overload_ft_nv) {
   RETURN_CALL_REAL_OP_IF_UNMOCK();
   RETURN_CALL_REAL_OP_IF_CALL_WITH_DEFGV();
 
-  status = _overload_ft_ops_sv(aTHX);
+  /* _overload_ft_ops_nv uses G_ARRAY and a status code to avoid the -1
+   * sentinel collision: FALLBACK_TO_REAL_OP is -1, but -1.0 is a valid
+   * NV result (e.g. file modified exactly 1 day in the future). */
+  _overload_ft_ops_nv(aTHX_ &check_status, &nv_value);
 
-  if ( !SvOK(status) ) { /* CHECK_IS_NULL — undef */
-    SvREFCNT_dec(status);
+  if ( check_status == -1 )
+    return CALL_REAL_OP();
+
+  if ( check_status == -2 ) { /* CHECK_IS_NULL */
     FT_SETUP_dSP_IF_NEEDED;
     FT_RETURNUNDEF;
   }
 
-  /* Save errno — sv_setnv()/sv_setiv() and FT_RETURN_TARG can trigger
-   * allocations or other Perl internals that clobber errno. */
+  /* Save errno — sv_setnv() and FT_RETURN_TARG can trigger allocations
+   * or other Perl internals that clobber errno. */
   saved_errno = errno;
 
-  /* Check for FALLBACK_TO_REAL_OP sentinel (-1) across all SV types */
-  if ( SvNV(status) == -1 ) {
-    SvREFCNT_dec(status);
-    return CALL_REAL_OP();
-  }
-
   {
     dTARGET;
     FT_SETUP_dSP_IF_NEEDED;
 
-    if ( SvNOK(status) )
-      sv_setnv(TARG, (NV) SvNV(status) );
-    else if ( SvIOK(status) )
-      sv_setiv(TARG, (IV) SvIV(status) );
-    else
-      sv_setnv(TARG, (NV) SvNV(status) );
-
-    SvREFCNT_dec(status);
+    sv_setnv(TARG, nv_value);
     errno = saved_errno;
     FT_RETURN_TARG;
   }

lib/Overload/FileCheck.pm

@@ -142,7 +142,8 @@ my %MAP_STAT_T_IX = (
 # op_type_id => check
 my %REVERSE_MAP = reverse %MAP_FC_OP;
 
-my %OP_CAN_RETURN_INT   = map { $MAP_FC_OP{$_} => 1 } qw{ s M C A };
+my %OP_CAN_RETURN_INT   = map { $MAP_FC_OP{$_} => 1 } qw{ s };
+my %OP_RETURNS_NV       = map { $MAP_FC_OP{$_} => 1 } qw{ M C A };
 my %OP_IS_STAT_OR_LSTAT = map { $MAP_FC_OP{$_} => 1 } qw{ stat lstat };
 #
 # This is listing the default ERRNO codes
@@ -363,17 +364,19 @@ sub _check_from_stat {
         c => sub { _check_mode_type( $lstat[ST_MODE], S_IFCHR ) },       # character special file
 
         # Age calculations: (basetime - timestamp) / seconds_per_day
+        # Returns scalar ref to distinguish real NV values from the
+        # FALLBACK_TO_REAL_OP sentinel (-1) — see _check() NV handling.
         M => sub {
             return CHECK_IS_NULL unless scalar @stat && defined $stat[ST_MTIME];
-            ( get_basetime() - $stat[ST_MTIME] ) / 86400.0;              # days since modification
+            \( ( get_basetime() - $stat[ST_MTIME] ) / 86400.0 );         # days since modification
         },
         A => sub {
             return CHECK_IS_NULL unless scalar @stat && defined $stat[ST_ATIME];
-            ( get_basetime() - $stat[ST_ATIME] ) / 86400.0;              # days since access
+            \( ( get_basetime() - $stat[ST_ATIME] ) / 86400.0 );         # days since access
         },
         C => sub {
             return CHECK_IS_NULL unless scalar @stat && defined $stat[ST_CTIME];
-            ( get_basetime() - $stat[ST_CTIME] ) / 86400.0;              # days since inode change
+            \( ( get_basetime() - $stat[ST_CTIME] ) / 86400.0 );         # days since inode change
         },
     );
 
@@ -479,6 +482,22 @@ sub _check {
         return $out;
     }
 
+    # NV ops (-M, -C, -A) return a (status, value) pair to avoid the -1
+    # sentinel collision: FALLBACK_TO_REAL_OP is -1, but -1.0 is a valid
+    # NV result (file modified exactly 1 day in the future).  The XS
+    # handler uses the status code instead of checking value == -1.
+    if ( $OP_RETURNS_NV{$optype} ) {
+        # Scalar ref: from _check_from_stat — always a real value, never FALLBACK
+        if ( ref $out eq 'SCALAR' ) {
+            return ( CHECK_IS_TRUE, $$out );
+        }
+        # Bare scalar: from direct mock_file_check callbacks
+        if ( !ref $out && $out == FALLBACK_TO_REAL_OP ) {
+            return (FALLBACK_TO_REAL_OP);
+        }
+        return ( CHECK_IS_TRUE, $out );
+    }
+
     if ( !$out ) {
 
         # Set a default ERRNO when the user didn't provide one,

t/nv-sentinel-collision.t

@@ -0,0 +1,139 @@
+#!/usr/bin/perl -w
+
+# Test that -M/-C/-A correctly return -1.0 when a file's timestamp is
+# exactly basetime + 86400 (1 day in the future), instead of silently
+# falling back to the real OP.
+#
+# The FALLBACK_TO_REAL_OP sentinel is -1.  Before the fix, the XS handler
+# pp_overload_ft_nv checked `SvNV(status) == -1` which collided with the
+# legitimate NV value -1.0.
+
+use strict;
+use warnings;
+
+use Test2::Bundle::Extended;
+use Test2::Tools::Explain;
+use Test2::Plugin::NoWarnings;
+
+use Overload::FileCheck q{:all};
+
+my $basetime = Overload::FileCheck::get_basetime();
+
+subtest 'mock_all_from_stat: -M returns -1.0 for mtime 1 day in future' => sub {
+    my $mtime_future = $basetime + 86400;
+
+    mock_all_from_stat(sub {
+        my ($op, $file) = @_;
+        return stat_as_file( mtime => $mtime_future ) if $file eq '/test/future';
+        return FALLBACK_TO_REAL_OP;
+    });
+
+    my $age = -M '/test/future';
+    ok( defined $age, '-M returns a defined value (not undef from fallback)' );
+    ok( abs($age - (-1.0)) < 0.01, "-M returns approximately -1.0 (got: $age)" )
+        if defined $age;
+
+    unmock_all_file_checks();
+    unmock_stat();
+};
+
+subtest 'mock_all_from_stat: -A returns -1.0 for atime 1 day in future' => sub {
+    my $atime_future = $basetime + 86400;
+
+    mock_all_from_stat(sub {
+        my ($op, $file) = @_;
+        return stat_as_file( atime => $atime_future ) if $file eq '/test/future';
+        return FALLBACK_TO_REAL_OP;
+    });
+
+    my $age = -A '/test/future';
+    ok( defined $age, '-A returns a defined value (not undef from fallback)' );
+    ok( abs($age - (-1.0)) < 0.01, "-A returns approximately -1.0 (got: $age)" )
+        if defined $age;
+
+    unmock_all_file_checks();
+    unmock_stat();
+};
+
+subtest 'mock_all_from_stat: -C returns -1.0 for ctime 1 day in future' => sub {
+    my $ctime_future = $basetime + 86400;
+
+    mock_all_from_stat(sub {
+        my ($op, $file) = @_;
+        return stat_as_file( ctime => $ctime_future ) if $file eq '/test/future';
+        return FALLBACK_TO_REAL_OP;
+    });
+
+    my $age = -C '/test/future';
+    ok( defined $age, '-C returns a defined value (not undef from fallback)' );
+    ok( abs($age - (-1.0)) < 0.01, "-C returns approximately -1.0 (got: $age)" )
+        if defined $age;
+
+    unmock_all_file_checks();
+    unmock_stat();
+};
+
+subtest 'mock_all_from_stat: FALLBACK still works for non-mocked files' => sub {
+    mock_all_from_stat(sub {
+        my ($op, $file) = @_;
+        return stat_as_file( mtime => $basetime ) if $file eq '/test/now';
+        return FALLBACK_TO_REAL_OP;
+    });
+
+    # /nonexistent should fall back to real stat => undef
+    my $age = -M '/nonexistent/path/should/not/exist';
+    ok( !defined $age, 'FALLBACK_TO_REAL_OP still delegates to real OP' );
+
+    # mocked file should return ~0 days
+    my $now_age = -M '/test/now';
+    ok( defined $now_age, '-M returns defined for mocked file' );
+    ok( abs($now_age) < 0.01, "-M returns approximately 0 (got: $now_age)" )
+        if defined $now_age;
+
+    unmock_all_file_checks();
+    unmock_stat();
+};
+
+subtest 'direct mock_file_check: -M FALLBACK_TO_REAL_OP still works' => sub {
+    mock_file_check( '-M' => sub {
+        my ($file) = @_;
+        return 42.5 if $file eq '/test/custom';
+        return FALLBACK_TO_REAL_OP;
+    });
+
+    my $age = -M '/test/custom';
+    ok( defined $age, '-M returns defined for mocked file' );
+    ok( abs($age - 42.5) < 0.01, "-M returns 42.5 (got: $age)" )
+        if defined $age;
+
+    my $real = -M '/nonexistent/direct/mock/fallback';
+    ok( !defined $real, 'FALLBACK_TO_REAL_OP works for direct -M mock' );
+
+    unmock_file_check('-M');
+};
+
+subtest 'mock_all_from_stat: other NV values pass through correctly' => sub {
+    mock_all_from_stat(sub {
+        my ($op, $file) = @_;
+        if ( $file eq '/test/past' ) {
+            return stat_as_file( mtime => $basetime - 2 * 86400 );  # 2 days ago
+        }
+        if ( $file eq '/test/far_future' ) {
+            return stat_as_file( mtime => $basetime + 5 * 86400 );  # 5 days future
+        }
+        return FALLBACK_TO_REAL_OP;
+    });
+
+    my $past = -M '/test/past';
+    ok( defined $past, '-M defined for 2 days ago' );
+    ok( abs($past - 2.0) < 0.01, "-M returns ~2.0 (got: $past)" ) if defined $past;
+
+    my $far = -M '/test/far_future';
+    ok( defined $far, '-M defined for 5 days in future' );
+    ok( abs($far - (-5.0)) < 0.01, "-M returns ~-5.0 (got: $far)" ) if defined $far;
+
+    unmock_all_file_checks();
+    unmock_stat();
+};
+
+done_testing;