warn about !$x == $y, !$x =~ /abc/, and similar constructs

[mauke]

Commit 2f48e46b3f6d2d introduced a warning for logical negation as the left operand of the isa operator, which likely indicates a precedence problem (i.e. the programmer wrote ! $x isa $y when they probably meant !($x isa $y)).

This commit extends the warning to all (in)equality comparisons (==, !=, <, >, <=, >=, eq, ne, lt, gt, le, ge) as well as binding operations (=~, !~).

As an indication that such a warning is useful in the real world, the core currently contains two files with (likely broken) code that triggers this warning:

• ./cpan/Test-Simple/lib/Test2/Hub.pm
• ./cpan/Scalar-List-Utils/t/uniqnum.t

[mauke]

Shout-outs to https://github.com/Dual-Life/Scalar-List-Utils/pull/134 and https://github.com/Test-More/test-more/pull/999.

[jkeenan]

Commit 2f48e46 introduced a warning for logical negation as the left operand of the isa operator, which likely indicates a precedence problem (i.e. the programmer wrote ! $x isa $y when they probably meant !($x isa $y)).

This commit extends the warning to all (in)equality comparisons (==, !=, <, >, <=, >=, eq, ne, lt, gt, le, ge) as well as binding operations (=~, !~).

As an indication that such a warning is useful in the real world, the core currently contains two files with (likely broken) code that triggers this warning:

* ./cpan/Test-Simple/lib/Test2/Hub.pm

* ./cpan/Scalar-List-Utils/t/uniqnum.t

This change is also causing test failures in t/porting/utils.t. I am very skeptical as to the need for this warning and I predict it will be very unpopular.

[mauke]

This change is also causing test failures in t/porting/utils.t.

That's the Test2::Hub issue I mentioned above. Once we sync the latest Test::Simple from CPAN, the error should disappear.

I am very skeptical as to the need for this warning and I predict it will be very unpopular.

Why? It found two long-standing bugs, one in Test2 and one in the test suite of List::Util, basically for free.

[haarg]

Taking a quick look at CPAN, most uses of this construct are intentionally comparing booleans, so this warning would be a false positive.

[mauke]

@haarg Can you show me a few examples you found? I want to see if it is possible to avoid most false positives or if I should just throw the whole thing away.

Note that it doesn't warn if ! appears on both sides of the comparison, so code like the following is "safe":

if ( !$block != !$orig ) {                         # ./dist/IO/lib/IO/Socket.pm
ok !!$exp == !!($str1 =~ $re), "$desc str =~ qr";  # ./ext/XS-APItest/t/callregexec.t
ok(!!-e " . . " == !!opendir(FOO, " . . ");        # ./t/win32/stat.t

[mauke]

Taking a quick look at CPAN, most uses of this construct are intentionally comparing booleans, so this warning would be a false positive.

I can't replicate this.

I did a cursory search on https://grep.metacpan.org, only looking for cases where the negated LHS is a plain scalar. I filtered out all the irrelevant results (matches in POD, strings, regexes, shell scripts, etc), analyzed the remaining cases and grouped them in three buckets.

First, the group of false positives:

Distribution	File	Code
Devel-Trepan	t/10test-db-eval.t	ok (!$msg == $is_good, "${code}" . ($msg ? ": $msg" : ''));
MsgPack-RPC	t/encoding-decoding.t	ok !!$next == $_;

These are unfortunate, but they should only produce a little extra output during testing. (And they are easily patched if needed, e.g. with ($msg xor $is_good) or !!$next == !!$_.) Normal users of these modules are not affected.

Next we have the group of potential false positives, but the files in question don't use warnings and so are unaffected:

Distribution	File	Code
Math-Algebra-Symbols	t/complex.t	ok( !$i == $i);
Math-Algebra-Symbols	t/unit.t	ok( !$i == $i );
Math-Zap	t/vector.t	ok(!$x == 1);
Math-Zap	t/vector2.t	ok(!$x == 1);
Object-Boolean	t/03-other.t	ok !$b eq No, 'not b is no';
Object-Boolean	t/03-other.t	ok !$b eq 'No', 'not is "No"';
Object-Boolean	t/03-other.t	ok !$a eq 'Yes', 'yes';
Object-Boolean	t/03-other.t	ok !$d eq 'No', 'no';

(Math-Zap and Math-Algebra-Symbols both use a non-standard overloading of !; e.g. the one for vectors actually returns the vector length.)

And in the third group there is the list of true positives, i.e. actual bugs that are potentially found by this warning (I think this list is incomplete because for one of my searches grep.metacpan.org stopped updating after "Piddy"):

Distribution	File	Code
AnyData	lib/AnyData.pm	&& !$createMode eq 'o';
AnyEvent-Yubico	lib/AnyEvent/Yubico.pm	if(! $signature eq $self->sign($response)) {
App-BCSSH	lib/App/BCSSSH/Command/ssh.pm	if ($auth_key && ! $auth_key ne $key) {
App-SourcePlot	lib/App/SourcePlot.pm	if ((!$yr =~ /$sety/) || $setm != $mo || $setd != $md) {
Array-KeepGrepped	t/01_basic.t	my @grep = grep { !$_ =~ m#a# } @test;
Array-KeepGrepped	t/01_basic.t	my (undef, @kgrep) = kgrep { !$_ =~ m#a# } @test;
Audio-Nama	lib/Audio/Nama/Engine.pm	if( ! $return_value == 256 ){
BalanceOfPower	lib/BalanceOfPower/Role/Warlord.pm	@attacker_coalition = grep { ! $attack_now eq $_ } @attacker_coalition;
Bio-NEXUS	exec/nextool.pl	if (! $s =~ /^\s*(\d+(-\d+)?)([,\s]\s*\d+(-\d+)?)*\s*$/ ) {
Bio-NEXUS	exec/readin_tcoffee.pl	if (! $tcoff =~ /^T-COFFEE/i) { die "\n\tError: File does not start with 'T-COFFEE'; does not appear to be a T-COFFEE file\n\n"; }
Bio-NEXUS	lib/Bio/NEXUS/CharactersBlock.pm	if ( $token =~ /^\d+$/ && !$charnum > 0 ) { $charnum = $token; next; }
Bio-PhyloNetwork	lib/Bio/PhyloNetwork.pm	if (! $subrtlng eq '') {
Bio-PhyloNetwork	lib/Bio/PhyloNetwork.pm	if (! $subrttype eq '') {
BioPerl	Bio/PhyloNetwork.pm	if (! $subrtlng eq '') {
BioPerl	Bio/PhyloNetwork.pm	if (! $subrttype eq '') {
Blitz	lib/Blitz/Validate.pm	elsif (! $type =~ /^(list|alpha|number|udid)$/) {
Business-WorldPay-Junior	Junior.pm	if ( ! $difference < -($hr_trans->{amount} * 0.05) )
CSAF	lib/CSAF/Validator/OptionalTests.pm	if (!$url =~ /^https\:/) {
Chooser	lib/Chooser.pm	if (!$value =~ /^\%/){
Crypt-Simple-SMIME	lib/Crypt/Simple/SMIME.pm	if ( ! $var =~ /\n/ && -f $var ) {
DBI-Easy	lib/DBI/Easy/Record/Collection.pm	if ($limit > $MAX_LIMIT or ! $limit > 0) {
DBIx-Class-InflateColumn-DateTime-WithTimeZone	lib/DBIx/Class/InflateColumn/DateTime/WithTimeZone.pm	if ( !$ic_dt_method || !$ic_dt_method =~ /(?: datetime | timestamp )/x ) {
DBIx-Frame	DBIx/Frame/CGI.pm	if ( ( $labels[$i] eq 'First' && ! $first > 0 )
DBIx-JCL	lib/DBIx/JCL.pm	if ( ! $severity eq 'MESSAGE' ) {
DNS-Bananafonana	Bananafonana.pm	if (! $markup =~ /[-_\.]/) {
Database-ManagedHandle	t/managed-singleton-tempdb-advanced.t	$dbh->begin_work() if ( !$driver eq 'CSV' );
Database-ManagedHandle	t/managed-singleton-tempdb-advanced.t	$dbh->commit if ( !$driver eq 'CSV' );
Database-ManagedHandle	t/managed-singleton-tempdb-advanced.t	$dbh->begin_work() if ( !$driver eq 'CSV' );
Database-ManagedHandle	t/managed-singleton-tempdb-advanced.t	$dbh->commit if ( !$driver eq 'CSV' );
Db-Documentum	test.pl	if (! $OS =~ /Win/i) {
Debug-Statements	lib/Debug/Statements.pm	$options = "-$options" if ! $options =~ /^-/;
E2-Interface	E2Node.pm	if( !$r =~ /<author .*?user_id="(.*?)"/s ) {
Embperl	eg/web/db/epwebapp.pl	$buri .= '/' if (!$buri =~ m#/$#) ;
FFI-Platypus	lib/FFI/Probe.pm	my $complex = !!$type =~ /complex/;
File-SmartTail	lib/File/SmartTail.pm	if (!$rmtopts =~ /\B-type\s+\w/) {
Forks-Super	t/42l-filehandles.t	&& !$line1 !~ /[\x{0100}-\x{CCCC}]/,
FreeBSD-Pkgs-FindUpdates	lib/FreeBSD/Pkgs/FindUpdates.pm	if (!$pkgname =~ /^bsdpan-/) {
GRID-Machine	scripts/gmdb	$sshcommand = $1 if !$sshcommand and !$hostdesc =~ /^#gm\s+sshcommand\s+'([^'\n]*)'/m;
Games-Axmud	lib/Games/Axmud/Client.pm	if (! $line =~ /^#!\s*(.*perl\S*)/) {
Graphics-HotMap	lib/Graphics/HotMap.pm	if ( ! $value > 0) {
HTML-Extract	lib/HTML/Extract.pm	if(!$command eq ""){
HTTP-WebTest-XMLParser	lib/HTTP/WebTest/XMLParser.pm	if (! $parent eq 'list') {
IBM-LoadLeveler	Makefile.PL	if ( ! $LLVER eq "UNKNOWN" )
IO-Util	lib/IO/Util.pm	&&! $mtime > $PARSING_CACHE{$type}{$path}{mtime} # and not old
Image-Math-Constrain	lib/Image/Math/Constrain.pm	!! $value =~ /^[1-9]\d*$/;
Image-Math-Constrain	lib/Image/Math/Constrain.pm	!! $value =~ /^[1-9]\d*$/;
Lab-Measurement-Legacy	lib/Lab/Instrument/YokogawaGS200.pm	if ( !$function eq 'VOLT' ) {
Lemonldap-Handlers-CAS	lib/Lemonldap/Handlers/ValidateCAS.pm	if ( ( !$controle == 0 ) and ( defined $controle->{string} ) ) {
List-Uniqnum	t/01uniqnum.t	$nanish != 0 && !$nanish != $NaN
List-Util-MaybeXS	t/uniqnum.t	$nanish != 0 && !$nanish != $NaN
Log-GELF-Util	lib/Log/GELF/Util.pm	if ( ! $key =~ /^[\w\.\-]+$/ ) {
Mail-BIMI	lib/Mail/BIMI/VMC.pm	next if ! $alt_name =~ /^dns:/;
Mail-Milter-Authentication	lib/Mail/Milter/Authentication/Protocol/SMTP.pm	if ( ! $line =~ /250/ ) {
MarpaX-Languages-C-AST	bin/c2ast	$lexemeCallbackHash{nbLines} = ($preprocessedOutput =~tr/\n/\n/ + ! $preprocessedOutput =~ /\n\z/);
Math-Base-Convert	Convert.pm	! $slen > $maxdlen{$fbase}) {
Mediawiki-Spider	lib/Mediawiki/Spider.pm	if(!$1 eq ""){
Mediawiki-Spider	lib/Mediawiki/Spider.pm	if($topush=~/Category/ && !$word=~/Category/){
Mojolicious-Plugin-Iconify	lib/Mojolicious/Plugin/Iconify/API.pm	next if ( !$file =~ /.json/ );
Myco	lib/Myco/QueryTemplate.pm	if ! $filter_string =~ /$result/;
NCBIx-Geo	lib/NCBIx/Geo.pm	if (! $data_dir =~ m#/$# ) { $self->set_data_dir( $data_dir . '/' ); }
Nes	lib/Nes.pm	return if !$block =~ /$self->{'tag_start'}\s*$self->{'tag_nes'}/;
Net-Amazon-S3	lib/Net/Amazon/S3/Request/Role/HTTP/Header.pm	(init_arg => undef) x!! $name =~ m/^_/,
Net-Blogger	lib/Net/Blogger/Engine/Userland.pm	if (! $child =~ /^(Net::Blogger::Engine::(Manila|Radio))$/) {
Net-DNS-Resolver-DoH	lib/Net/DNS/Resolver/DoH.pm	next if ! $ns =~ /https:\/\//;
Net-DNS-Resolver-DoH	lib/Net/DNS/Resolver/DoH.pm	next if ! $ns =~ /\{dns\}/;
Net-DirectConnect	lib/Net/DirectConnect/pslib/psmisc.pm	return undef if $ip =~ /^(?:0|127)\./ and !$host =~ /^(?:0|127)\./;
Net-PSYC	PSYC/Event/Event.pm	next if($flags && !$flags =~ /$_/);
Net-Telnet-Trango	scripts/su.cgi	if ( !$suid =~ /^\d+$/ ) {
Neuron	Neuron.pm	if(! $s =~ /^Hiddenlayer:/) {
OOPS	t/upgrade1004.t	rcon if $docommit & 2**$tn && ! $tn < $#func;
PDF-API2	contrib/text2pdf.pl	if ( ! $left > 756 ) {
Perl-Repository-APC	scripts/buildaperl	die "patchlevel not > 0" if length $patchlevel && ! $patchlevel > 0;
Piddy	lib/Piddy.pm	if (! $rpid eq getppid()) {
SAS-Parser	lib/SAS/Parser.pm	if (! $mfunc =~ m/$mfuncs/) {
Sakai-Nakamura	lib/Sakai/Nakamura/Content.pm	if ( !$content_filename =~ /.*\..*/x ) {
Spreadsheet-HTML	lib/Spreadsheet/HTML/Presets/Beadwork.pm	my @lines = grep ! $_ =~ /^\s*$/, split /\n/, $args->{art};
Startup	Startup.pm	return $txt if !$txt=~/\$/;
TX	lib/TX.pm	delete @{$I->cache}{grep( ($xor xor !$_=~$re), keys %{$I->cache} )};
TX	lib/TX.pm	delete @{$I->cache}{grep( !$_=~$re, keys %{$I->cache} )};
TX	lib/TX.pm	delete @{$I->cache}{grep( ($xor xor !$_=~$re), keys %{$I->cache} )};
Text-Macro	Macro.pm	if ( ! $path =~ m!/$! ) {

[tonycoz]

I like the change, but it would be nice to see more on the false positives.

[mauke]

I just pushed a change to get rid of a class of false positives (where ! on the right-hand side has been constant-folded away).

@haarg, do you have more examples of false positives?

[mauke]

There have been no new comments in a while. I still think this warning is a good idea, but to collect more feedback, I'd like to merge it into blead and see if anything breaks on CPAN. If it turns out to be too noisy, we can still revert it.

Objections?

[leonerd]

I think it seems reasonable. The only big class of false-positive case I can think of is the !$x == !$y style of comparing booleans, but I see that is already specifically handled and doesn't issue a warning. I think it's good enough to try it out at least, and see what the result is.