Skip to content

Commit 570fa43

Browse files
maukemauke
committed
warn about !$x == $y, !$x =~ /abc/, and similar constructs
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
1 parent 869f245 commit 570fa43

File tree

7 files changed

+163
-29
lines changed

7 files changed

+163
-29
lines changed

‎embed.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1242,6 +1242,7 @@
12421242
# define ck_rfun(a) Perl_ck_rfun(aTHX_ a)
12431243
# define ck_rvconst(a) Perl_ck_rvconst(aTHX_ a)
12441244
# define ck_sassign(a) Perl_ck_sassign(aTHX_ a)
1245+
# define ck_scmp(a) Perl_ck_scmp(aTHX_ a)
12451246
# define ck_select(a) Perl_ck_select(aTHX_ a)
12461247
# define ck_shift(a) Perl_ck_shift(aTHX_ a)
12471248
# define ck_sort(a) Perl_ck_sort(aTHX_ a)

‎op.c

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4297,6 +4297,11 @@ Perl_bind_match(pTHX_ I32 type, OP *left, OP *right)
42974297
o = right;
42984298
}
42994299
else {
4300+
if (left->op_type == OP_NOT && !(left->op_flags & OPf_PARENS)) {
4301+
Perl_ck_warner(aTHX_ packWARN(WARN_PRECEDENCE),
4302+
"Possible precedence problem between ! and %s", PL_op_desc[rtype]
4303+
);
4304+
}
43004305
right->op_flags |= OPf_STACKED;
43014306
if (rtype != OP_MATCH && rtype != OP_TRANSR &&
43024307
! (rtype == OP_TRANS &&
@@ -12104,6 +12109,18 @@ Perl_ck_bitop(pTHX_ OP *o)
1210412109
return o;
1210512110
}
1210612111

12112+
static void
12113+
check_precedence_not_vs_cmp(pTHX_ const OP *const o)
12114+
{
12115+
/* warn for !$x == 42, but not !$x == !$y */
12116+
const OP *const kid = cUNOPo->op_first;
12117+
if (kid->op_type == OP_NOT && !(kid->op_flags & OPf_PARENS) && OpSIBLING(kid)->op_type != OP_NOT) {
12118+
Perl_ck_warner(aTHX_ packWARN(WARN_PRECEDENCE),
12119+
"Possible precedence problem between ! and %s", OP_DESC(o)
12120+
);
12121+
}
12122+
}
12123+
1210712124
PERL_STATIC_INLINE bool
1210812125
is_dollar_bracket(pTHX_ const OP * const o)
1210912126
{
@@ -12151,6 +12168,8 @@ Perl_ck_cmp(pTHX_ OP *o)
1215112168
"$[ used in %s (did you mean $] ?)", OP_DESC(o));
1215212169
}
1215312170

12171+
check_precedence_not_vs_cmp(aTHX_ o);
12172+
1215412173
/* convert (index(...) == -1) and variations into
1215512174
* (r)index/BOOL(,NEG)
1215612175
*/
@@ -12230,6 +12249,17 @@ Perl_ck_cmp(pTHX_ OP *o)
1223012249
return indexop;
1223112250
}
1223212251

12252+
/* for slt, sgt, sle, sge, seq, sne */
12253+
12254+
OP *
12255+
Perl_ck_scmp(pTHX_ OP *o)
12256+
{
12257+
PERL_ARGS_ASSERT_CK_SCMP;
12258+
12259+
check_precedence_not_vs_cmp(aTHX_ o);
12260+
12261+
return o;
12262+
}
1223312263

1223412264
OP *
1223512265
Perl_ck_concat(pTHX_ OP *o)
@@ -15125,7 +15155,7 @@ Perl_ck_isa(pTHX_ OP *o)
1512515155
/* !$x isa Some::Class # probably meant !($x isa Some::Class) */
1512615156
if (objop->op_type == OP_NOT && !(objop->op_flags & OPf_PARENS)) {
1512715157
Perl_ck_warner(aTHX_ packWARN(WARN_PRECEDENCE),
15128-
"Possible precedence problem on isa operator"
15158+
"Possible precedence problem between ! and %s", OP_DESC(o)
1512915159
);
1513015160
}
1513115161

‎opcode.h

Lines changed: 6 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pod/perldiag.pod

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -5427,6 +5427,42 @@ Note this may be also triggered for constructs like:
54275427

54285428
sub { 1 if die; }
54295429

5430+
=item Possible precedence problem between ! and %s
5431+
5432+
(W precedence) You wrote something like
5433+
5434+
!$x < $y # parsed as: (!$x) < $y
5435+
!$x eq $y # parsed as: (!$x) eq $y
5436+
!$x =~ /regex/ # parsed as: (!$x) =~ /regex/
5437+
!$obj isa Some::Class # parsed as: (!$obj) isa Some::Class
5438+
5439+
but because C<!> has higher precedence than comparison operators, C<=~>, and
5440+
C<isa>, this is interpreted as comparing/matching the logical negation of the
5441+
first operand, instead of negating the result of the comparison/match.
5442+
5443+
To disambiguate, either use a negated comparison/binding operator:
5444+
5445+
$x >= $y
5446+
$x ne $y
5447+
$x !~ /regex/
5448+
5449+
... or parentheses:
5450+
5451+
!($x < $y)
5452+
!($x eq $y)
5453+
!($x =~ /regex/)
5454+
!($obj isa Some::Class)
5455+
5456+
... or the low-precedence C<not> operator:
5457+
5458+
not $x < $y
5459+
not $x eq $y
5460+
not $x =~ /regex/
5461+
not $obj isa Some::Class
5462+
5463+
(If you did mean to compare the boolean result of negating the first operand,
5464+
parenthesize as C<< (!$x) < $y >>, C<< (!$x) eq $y >>, etc.)
5465+
54305466
=item Possible precedence problem on bitwise %s operator
54315467

54325468
(W precedence) Your program uses a bitwise logical operator in conjunction
@@ -5453,19 +5489,6 @@ If instead you intended to match the word 'foo' at the end of the line
54535489
followed by whitespace and the word 'bar' on the next line then you can use
54545490
C<m/$(?)\/> (for example: C<m/foo$(?)\s+bar/>).
54555491

5456-
=item Possible precedence problem on isa operator
5457-
5458-
(W precedence) You wrote something like
5459-
5460-
!$obj isa Some::Class
5461-
5462-
but because C<!> has higher precedence than C<isa>, this is interpreted as
5463-
C<(!$obj) isa Some::Class>, which checks whether a boolean is an instance of
5464-
C<Some::Class>. If you want to negate the result of C<isa> instead, use one of:
5465-
5466-
!($obj isa Some::Class) # explicit parentheses
5467-
not $obj isa Some::Class # low-precedence 'not' operator
5468-
54695492
=item Possible unintended interpolation of %s in string
54705493

54715494
(W ambiguous) You said something like '@foo' in a double-quoted string

‎proto.h

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎regen/opcodes

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -153,12 +153,12 @@ i_ne integer ne (!=) ck_cmp ifs2 S S<
153153
ncmp numeric comparison (<=>) ck_null Iifst2 S S<
154154
i_ncmp integer comparison (<=>) ck_null ifst2 S S<
155155

156-
slt string lt ck_null ifs2 S S
157-
sgt string gt ck_null ifs2 S S
158-
sle string le ck_null ifs2 S S
159-
sge string ge ck_null ifs2 S S
160-
seq string eq ck_null ifs2 S S
161-
sne string ne ck_null ifs2 S S
156+
slt string lt ck_scmp ifs2 S S
157+
sgt string gt ck_scmp ifs2 S S
158+
sle string le ck_scmp ifs2 S S
159+
sge string ge ck_scmp ifs2 S S
160+
seq string eq ck_scmp ifs2 S S
161+
sne string ne ck_scmp ifs2 S S
162162
scmp string comparison (cmp) ck_null ifst2 S S
163163

164164
bit_and bitwise and (&) ck_bitop fst2 S S|

‎t/lib/warnings/op

Lines changed: 76 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1582,9 +1582,82 @@ $a = (!$b) isa Some::Class;
15821582
$a = !($b) isa Some::Class; # should warn
15831583
$a = !($b isa Some::Class);
15841584
$a = not $b isa Some::Class;
1585-
EXPECT
1586-
Possible precedence problem on isa operator at - line 4.
1587-
Possible precedence problem on isa operator at - line 6.
1585+
1586+
my $code = "#line 1 [cmp]\n";
1587+
for my $op (qw( == != < > <= >= eq ne lt gt le ge )) {
1588+
$code .= "\$a = !\$a $op \$b;\n"; # should warn
1589+
$code .= "\$a = (!\$a) $op \$b;\n";
1590+
$code .= "\$a = !(\$a) $op \$b;\n"; # should warn
1591+
$code .= "\$a = !(\$a $op \$b);\n";
1592+
$code .= "\$a = not \$a $op \$b;\n";
1593+
}
1594+
$a = $b = 0;
1595+
eval $code;
1596+
die $@ if $@;
1597+
1598+
$_ = '';
1599+
$a = !/x/;
1600+
1601+
$code = "#line 1 [bind]\n";
1602+
for my $rhs (qw( /x/ $b tr/x/x/ tr!x!y!r s!x!y!r )) {
1603+
for my $bind ('=~', $rhs =~ /!r\z/ ? () : '!~') {
1604+
$code .= "\$a = !\$a $bind $rhs;\n"; # should warn
1605+
$code .= "\$a = (!\$a) $bind $rhs;\n";
1606+
$code .= "\$a = !(\$a) $bind $rhs;\n"; # should warn
1607+
$code .= "\$a = !(\$a $bind $rhs);\n";
1608+
$code .= "\$a = not \$a $bind $rhs;\n";
1609+
}
1610+
}
1611+
$a = $b = 0;
1612+
eval $code;
1613+
die $@ if $@;
1614+
1615+
# doesn't compile, but should warn anyway
1616+
eval "#line 1 [extra]\n" . '$a = !$a =~ s/x/y/';
1617+
EXPECT
1618+
Possible precedence problem between ! and derived class test at - line 4.
1619+
Possible precedence problem between ! and derived class test at - line 6.
1620+
Possible precedence problem between ! and numeric eq (==) at [cmp] line 1.
1621+
Possible precedence problem between ! and numeric eq (==) at [cmp] line 3.
1622+
Possible precedence problem between ! and numeric ne (!=) at [cmp] line 6.
1623+
Possible precedence problem between ! and numeric ne (!=) at [cmp] line 8.
1624+
Possible precedence problem between ! and numeric lt (<) at [cmp] line 11.
1625+
Possible precedence problem between ! and numeric lt (<) at [cmp] line 13.
1626+
Possible precedence problem between ! and numeric gt (>) at [cmp] line 16.
1627+
Possible precedence problem between ! and numeric gt (>) at [cmp] line 18.
1628+
Possible precedence problem between ! and numeric le (<=) at [cmp] line 21.
1629+
Possible precedence problem between ! and numeric le (<=) at [cmp] line 23.
1630+
Possible precedence problem between ! and numeric ge (>=) at [cmp] line 26.
1631+
Possible precedence problem between ! and numeric ge (>=) at [cmp] line 28.
1632+
Possible precedence problem between ! and string eq at [cmp] line 31.
1633+
Possible precedence problem between ! and string eq at [cmp] line 33.
1634+
Possible precedence problem between ! and string ne at [cmp] line 36.
1635+
Possible precedence problem between ! and string ne at [cmp] line 38.
1636+
Possible precedence problem between ! and string lt at [cmp] line 41.
1637+
Possible precedence problem between ! and string lt at [cmp] line 43.
1638+
Possible precedence problem between ! and string gt at [cmp] line 46.
1639+
Possible precedence problem between ! and string gt at [cmp] line 48.
1640+
Possible precedence problem between ! and string le at [cmp] line 51.
1641+
Possible precedence problem between ! and string le at [cmp] line 53.
1642+
Possible precedence problem between ! and string ge at [cmp] line 56.
1643+
Possible precedence problem between ! and string ge at [cmp] line 58.
1644+
Possible precedence problem between ! and pattern match (m//) at [bind] line 1.
1645+
Possible precedence problem between ! and pattern match (m//) at [bind] line 3.
1646+
Possible precedence problem between ! and pattern match (m//) at [bind] line 6.
1647+
Possible precedence problem between ! and pattern match (m//) at [bind] line 8.
1648+
Possible precedence problem between ! and pattern match (m//) at [bind] line 11.
1649+
Possible precedence problem between ! and pattern match (m//) at [bind] line 13.
1650+
Possible precedence problem between ! and pattern match (m//) at [bind] line 16.
1651+
Possible precedence problem between ! and pattern match (m//) at [bind] line 18.
1652+
Possible precedence problem between ! and transliteration (tr///) at [bind] line 21.
1653+
Possible precedence problem between ! and transliteration (tr///) at [bind] line 23.
1654+
Possible precedence problem between ! and transliteration (tr///) at [bind] line 26.
1655+
Possible precedence problem between ! and transliteration (tr///) at [bind] line 28.
1656+
Possible precedence problem between ! and transliteration (tr///) at [bind] line 31.
1657+
Possible precedence problem between ! and transliteration (tr///) at [bind] line 33.
1658+
Possible precedence problem between ! and substitution (s///) at [bind] line 36.
1659+
Possible precedence problem between ! and substitution (s///) at [bind] line 38.
1660+
Possible precedence problem between ! and substitution (s///) at [extra] line 1.
15881661
########
15891662
# op.c
15901663
use integer;

0 commit comments

Comments
 (0)