re() with a number fails intermittently

[FGasper]

use strict;
use warnings;

use Test::More;
use Test::FailWarnings;
use Test::Deep;

cmp_deeply(
    [ qw( h 6 z ) ],
    bag( re('h'), re(6), re('z') ),
    'expected warnings',
);

done_testing;

^^ The above intermittently fails for me.

Happens w/ Perl 5.26 & T::D 1.128.

What seems to happen is that bag() creates two qr/6/ entries; that’s what causes the failure:

> pbpaste | perl
# bless( {
#   'IgnoreDupes' => 0,
#   'SubSup' => '',
#   'val' => [
#     bless( {
#       'val' => qr/h/
#     }, 'Test::Deep::Regexp' ),
#     bless( {
#       'val' => qr/6/
#     }, 'Test::Deep::Regexp' ),
#     $VAR1->{'val'}[1]
#   ]
# }, 'Test::Deep::Set' )
not ok 1 - expected warnings
#   Failed test 'expected warnings'
#   at - line 11.
# Comparing $data as a Bag
# Missing: 1 reference
# Extra: 'z'
1..1
# Looks like you failed 1 test of 1.

ERROR 1
felipe@felipes-mbp 16:42:22 /cpanel
> pbpaste | perl
# bless( {
#   'IgnoreDupes' => 0,
#   'SubSup' => '',
#   'val' => [
#     bless( {
#       'val' => qr/h/
#     }, 'Test::Deep::Regexp' ),
#     bless( {
#       'val' => qr/6/
#     }, 'Test::Deep::Regexp' ),
#     bless( {
#       'val' => qr/z/
#     }, 'Test::Deep::Regexp' )
#   ]
# }, 'Test::Deep::Set' )
ok 1 - expected warnings
1..1

[karenetheridge]

The problem is that Test::Deep::descend is trying to compare the two objects even though they're both blessed, and then Test::Deep::Regexp::descend, on the very last line, does:

return ($got =~ $re) ? 1 : 0;

If the $got refaddr happens to contain a 6 in it, then the two Test::Deep::Regexp values (containing qr/6/ and qr/z/ respectively) falsely match.

[karenetheridge]

(previous response edited. no I'm not intending on providing a fix; this was a drive-by diagnosis provided gratis.)

[rjbs]

I'm confused by this diagnosis, although it's probably right and seemed brilliant at first glance.

The example code given was:

cmp_deeply(
    [ qw( h 6 z ) ],
    bag( re('h'), re(6), re('z') ),
    'expected warnings',
);

So I'd expect the return ($got =~ $re) ? 1 : 0 in &descent to have one of those three one-character strings from the qw(…) as $got while descending, right? Not an object.

I added print statement, because that is how I debug. I find that first we see it =~ the Test::Deep::Regexp test objects, only followed afterwards by the strings. Here are some stacks:

First, when we compare the object:

# Warning was 'ABOUT TO COMPARE at lib/Test/Deep/Regexp.pm line 59.
# 	Test::Deep::Regexp::descend(Test::Deep::Regexp=HASH(0x1334b0ef0), Test::Deep::Regexp=HASH(0x1334e4910)) called at lib/Test/Deep.pm line 418
# 	Test::Deep::descend(Test::Deep::Regexp=HASH(0x1334e4910), Test::Deep::Regexp=HASH(0x1334b0ef0)) called at lib/Test/Deep.pm line 241
# 	Test::Deep::cmp_details(Test::Deep::Regexp=HASH(0x1334e4910), Test::Deep::Regexp=HASH(0x1334b0ef0)) called at lib/Test/Deep.pm line 250
# 	Test::Deep::eq_deeply(Test::Deep::Regexp=HASH(0x1334e4910), Test::Deep::Regexp=HASH(0x1334b0ef0)) called at lib/Test/Deep/Set.pm line 147
# 	Test::Deep::Set::add(Test::Deep::Set=HASH(0x132826040), Test::Deep::Regexp=HASH(0x1334b0ef0), Test::Deep::Regexp=HASH(0x1334e4910), Test::Deep::Regexp=HASH(0x1334e4898)) called at lib/Test/Deep/Set.pm line 17
# 	Test::Deep::Set::init(Test::Deep::Set=HASH(0x132826040), 0, "", Test::Deep::Regexp=HASH(0x1334b0ef0), Test::Deep::Regexp=HASH(0x1334e4910), Test::Deep::Regexp=HASH(0x1334e4898)) called at lib/Test/Deep/Cmp.pm line 34
# 	Test::Deep::Cmp::new("Test::Deep::Set", 0, "", Test::Deep::Regexp=HASH(0x1334b0ef0), Test::Deep::Regexp=HASH(0x1334e4910), Test::Deep::Regexp=HASH(0x1334e4898)) called at lib/Test/Deep.pm line 582
# 	Test::Deep::bag(Test::Deep::Regexp=HASH(0x1334b0ef0), Test::Deep::Regexp=HASH(0x1334e4910), Test::Deep::Regexp=HASH(0x1334e4898)) called at /Users/rjbs/test line 8'

Next, when we compare the strings:

not ok 4 - Test::FailWarnings should catch no warnings
#   Failed test 'Test::FailWarnings should catch no warnings'
#   at /Users/rjbs/.plenv/versions/5.37.2/lib/perl5/5.37.2/Carp.pm line 294.
# Warning was 'ABOUT TO COMPARE at lib/Test/Deep/Regexp.pm line 59.
# 	Test::Deep::Regexp::descend(Test::Deep::Regexp=HASH(0x1334b0ef0), "z") called at lib/Test/Deep.pm line 418
# 	Test::Deep::descend("z", Test::Deep::Regexp=HASH(0x1334b0ef0)) called at lib/Test/Deep.pm line 269
# 	Test::Deep::eq_deeply_cache("z", Test::Deep::Regexp=HASH(0x1334b0ef0)) called at lib/Test/Deep/Set.pm line 57
# 	Test::Deep::Set::descend(Test::Deep::Set=HASH(0x132826040), ARRAY(0x132825fb0)) called at lib/Test/Deep.pm line 418
# 	Test::Deep::descend(ARRAY(0x132825fb0), Test::Deep::Set=HASH(0x132826040)) called at lib/Test/Deep.pm line 241
# 	Test::Deep::cmp_details(ARRAY(0x132825fb0), Test::Deep::Set=HASH(0x132826040)) called at lib/Test/Deep.pm line 222
# 	Test::Deep::cmp_deeply(ARRAY(0x132825fb0), Test::Deep::Set=HASH(0x132826040), "expected warnings") called at /Users/rjbs/test line 8'
got: "z"
re: h

[rjbs]

Aha!

The "bad" comparison happens when Test::Deep::Set::add is being called. What happens is that when you write bag(1,2,3); under the hood this creates a Set object and calls ->add(1,2,3) on it. Because a bag ignores any duplicated condition, it checks each new condition against previously added ones. (This has to be like O(N²) right? 🙁). It does this by calling eq_deeply($old, $new) for each element already in the set. That is:

my @added;
for my $item (@to_add) {
  next if grep {; eq_deeply($_, $item) } @added;
  push @added, $item;
}

…and so the problem is that then we end up using the re() comparator against another re(). Silly. The use of eq_deeply here seems really specious to me.

Putting that aside, I think that when old and new are both Cmp objects, we should always treat them as distinct. Except you can't just check ->isa here because 🤯

  DB<1> use Test::Deep

  DB<2>  x @Test::Deep::Regexp::ISA
  empty array
  DB<3> re(qr(1))

  DB<4>  x @Test::Deep::Regexp::ISA
0  'Test::Deep::Cmp'
1  'Test::Deep::RegexpMatches'

Whyyyyyy?

[rjbs]

No, wait, the debugger snippet above is no problem, right? Because we don't load the library until we have an re(). That isn't the problem, because I already have one. I'll keep digging. Thank you for subscribing to my newsletter.

[rjbs]

Okay, so I overcame my silly problem and wrote a fix(?) which is in #85. The problem is that it breaks some test tests, which are sort of poorly annotated so I'm not 100% sure what they're doing. I'll come back to those unless somebody gets there before me.