warn about !$x == $y, !$x =~ /abc/, and similar constructs #22507
Conversation
|
Shout-outs to Dual-Life/Scalar-List-Utils#134 and Test-More/test-more#999. |
This change is also causing test failures in |
That's the Test2::Hub issue I mentioned above. Once we sync the latest Test::Simple from CPAN, the error should disappear.
Why? It found two long-standing bugs, one in Test2 and one in the test suite of List::Util, basically for free. |
|
Taking a quick look at CPAN, most uses of this construct are intentionally comparing booleans, so this warning would be a false positive. |
|
@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 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 |
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
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:
These are unfortunate, but they should only produce a little extra output during testing. (And they are easily patched if needed, e.g. with Next we have the group of potential false positives, but the files in question don't
(Math-Zap and Math-Algebra-Symbols both use a non-standard overloading of 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"):
|
|
I like the change, but it would be nice to see more on the false positives. |
|
I just pushed a change to get rid of a class of false positives (where @haarg, do you have more examples of false positives? |
|
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? |
|
I think it seems reasonable. The only big class of false-positive case I can think of is the |
Commit 2f48e46 introduced a warning for logical negation as the left operand of the
isaoperator, which likely indicates a precedence problem (i.e. the programmer wrote! $x isa $ywhen 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: