Test::Builder: Fix "return $a or $b" precedence issue

[nthykier]

perl parses "return $a or $b" effectively as "return $a;", which is
not what was intended.

Signed-off-by: Niels Thykier niels@thykier.net

[cpansprout]

This issue is blocking a patch at https://rt.perl.org/rt3/Ticket/Display.html?id=59802, so I would appreciate it if a new release could manifest itself soon.

[rjbs]

I'm adding my puppy dog eyes to this request for a new release so we can try to break everything as soon as possible, rather than wait too long. 🐶 👀

[pdl]

Branch

Note: This bug is on branch TestBuilder-1.5, but I think it should be on master, especially if @rjbs wants it for including in perl.

Desired Outcome

I've tried to write some tests for this at pdl@75dedee but doing so has left me not totally clear what the purpose of the code is.

The comment preceding _is_dualvar claims

This is a hack to detect a dualvar such as $!

But the documentation of Scalar::Util at isdual claims that:

Note that although $! appears to be dual-valued variable, it is actually implemented using a tied scalar

Whether or not you add the brackets, _is_dualvar doesn't reliably detect dualvars. And I don't know if it handles $! correctly either.

NB: Although @schwern has elsewhere bitten the bullet and required Scalar::Util elsewhere in Test::More, the version of Scalar::Util which ships with perl does not include isdual, which would have been a handy shortcut here. Nor can we copy the code as it is implemented in C at http://cpansearch.perl.org/src/PEVANS/Scalar-List-Utils-1.31/ListUtil.xs

Hm. Ideas?

Precedence

Because of the confusion over the desired outcome, I'm not sure what the intended precedence is. I believe the following are equivalent, which suggests ? 1 : 0 is redundant. Or that some other precedence is intended.

return ($numval != 0 and $numval ne $val ? 1 : 0);
return ( ( ($numval != 0) and ($numval ne $val) ) ? 1 : 0);
return ( ( ($numval != 0) and ($numval ne $val) ) ? 1 : 0);
return ( ($numval != 0) and ($numval ne $val) );

[cpansprout]

The return value of _is_dualvar affects the diagnostic output when the test fails. Take this for instance:

use Test::More 'no_plan';
cmp_ok($!=1, '==', "foo");

The output is:

Argument "foo" isn't numeric in numeric eq (==) at (eval in cmp_ok) - line 2.
not ok 1
#   Failed test at - line 2.
#          got: 1
#     expected: foo
1..1
# Looks like you failed 1 test of 1.

I think the intent is to display the numeric part of a dualvar if the number and string forms are different. But the current logic is such that anything non-zero is nummified (except references).
Come to think of it, though, that logic seems sufficient, as the way _is_dualvar was intended to be written, "6.0" would be considered a dualvar because (0+"6.0") ne "6.0". So ultimately just checking that the value is non-zero is what _is_dualvar would be doing anyway, even without the precedence bug.
I hope my rambling makes some sense.

[cpansprout]

In the interest of getting the proposed perl warning tested for as long as possible before perl 5.20, I have applied Niels’ Test::Builder patch to bleadperl as commit 13c65ef8cd5. I bumped the version to 0.98_06, which has not been used in any CPAN release yet.

[rjbs]

I've merged this to test-more/master and will get a trial release out soon and a production release following that.

[rjbs]

I am closing this issue. 0.98_06 is on CPAN now and a production release will follow after two weeks (or so) of testing.

[cpansprout]

I know I’m too late in mentioning it, but using a version number on CPAN that blead has already used for a different version is going to cause confusion....

(To prevent that was the whole reason I mentioned the version bump in this thread.)

[kentfredric]

I'm getting a precedence issue with the latest on CPAN(1.001002) vs perl 5.19.5 , complaining in YAML's test suite about this line:

    return $numval != 0 and $numval ne $val ? 1 : 0;

in the _is_dualvar function.

http://www.cpantesters.org/cpan/report/43dc9cca-4936-11e3-b097-b376ba4edfeb

[kentfredric]

Oh God, sorry, I misread, YAML bundles Test::Builder, so its bundling an old one >:( , My mistake.