Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

op.c: Warn on "return $a or $b" [perl #59802]

Add a warning for the (likely) unintended use of "return $a or $b"
(and similar expressions), which perl parses as "(return $a) || $b"
(which is effectively just "return $a;").

Note this warning is triggered by some modules (e.g. Test::Builder).
These are not fixed by this commit.

Signed-off-by: Niels Thykier <niels@thykier.net>
  • Loading branch information...
commit f86e1520a36f9b53a86479515c0e8e96814e20ee 1 parent a8fb2a7
@nthykier nthykier authored committed
Showing with 97 additions and 0 deletions.
  1. +38 −0 op.c
  2. +22 −0 pod/perldiag.pod
  3. +37 −0 t/lib/warnings/op
View
38 op.c
@@ -5830,6 +5830,44 @@ S_new_logop(pTHX_ I32 type, I32 flags, OP** firstp, OP** otherp)
first = *firstp;
other = *otherp;
+ /* [perl #59802]: Warn about things like "return $a or $b", which
+ is parsed as "(return $a) or $b" rather than "return ($a or
+ $b)". NB: This also applies to xor, which is why we do it
+ here.
+ */
+ switch (first->op_type) {
+ case OP_NEXT:
+ case OP_LAST:
+ case OP_REDO:
+ /* XXX: Perhaps we should emit a stronger warning for these.
+ Even with the high-precedence operator they don't seem to do
+ anything sensible.
+
+ But until we do, fall through here.
+ */
+ case OP_RETURN:
+ case OP_DIE:
+ case OP_EXIT:
+ case OP_GOTO:
+ /* XXX: Currently we allow people to "shoot themselves in the
+ foot" by explicitly writing "(return $a) or $b".
+
+ Warn unless we are looking at the result from folding or if
+ the programmer explicitly grouped the operators like this.
+ The former can occur with e.g.
+
+ use constant FEATURE => ( $] >= ... );
+ sub { not FEATURE and return or do_stuff(); }
+ */
+ if (!first->op_folded && !(first->op_flags & OPf_PARENS))
+ Perl_ck_warner(aTHX_ packWARN(WARN_SYNTAX),
+ "Possible precedence issue with control flow operator");
+ /* XXX: Should we optimze this to "return $a;" (i.e. remove
+ the "or $b" part)?
+ */
+ break;
+ }
+
if (type == OP_XOR) /* Not short circuit, but here by precedence. */
return newBINOP(type, flags, scalar(first), scalar(other));
View
22 pod/perldiag.pod
@@ -4229,6 +4229,28 @@ higher precedence of C<==>. This is probably not what you want. (If you
really meant to write this, disable the warning, or, better, put the
parentheses explicitly and write C<$x & ($y == 0)>).
+=item Possible precedence issue with control flow operator
+
+(W syntax) There is a possible problem with the mixing of a control
+flow operator (e.g. C<return>) and a low-precedence operator like
+C<or>. Consider:
+
+ sub { return $a or $b; }
+
+This is parsed as:
+
+ sub { (return $a) or $b; }
+
+Which is effectively just:
+
+ sub { return $a; }
+
+Either use parentheses or the high-precedence variant of the operator.
+
+Note this may be also triggered for constructs like:
+
+ sub { 1 if die; }
+
=item Possible unintended interpolation of $\ in regex
(W ambiguous) You said something like C<m/$\/> in a regex.
View
37 t/lib/warnings/op
@@ -1575,3 +1575,40 @@ OPTION regex
?(?s).*
Subroutine DynaLoader::dl_error redefined at \(eval 2\) line 2\.
########
+# op.c
+use warnings;
+sub do_warn_1 { return $a or $b; }
+sub do_warn_2 { return $a and $b; }
+sub do_warn_3 { return $a xor $b; }
+sub do_warn_4 { die $a or $b; }
+sub do_warn_5 { die $a and $b; }
+sub do_warn_6 { die $a xor $b; }
+# These get re-written to "(return/die $a) and $b"
+sub do_warn_7 { $b if return $a; }
+sub do_warn_8 { $b if die $a; }
+
+use constant FEATURE => 1;
+use constant MISSING_FEATURE => 0;
+
+sub dont_warn_1 { return ($a or $b); }
+sub dont_warn_2 { return ($a and $b); }
+sub dont_warn_3 { return ($a xor $b); }
+
+sub dont_warn_4 { MISSING_FEATURE and return or dont_warn_3(); }
+sub dont_warn_5 { FEATURE || return and dont_warn_3(); }
+sub dont_warn_6 { not FEATURE and return or dont_warn_3(); }
+sub dont_warn_7 { !MISSING_FEATURE || return and dont_warn_3(); }
+
+# These are weird, but at least not ambiguous.
+sub dont_warn_8 { (return $a) or $b; }
+sub dont_warn_9 { (return $a) and $b; }
+sub dont_warn_10 { (return $a) xor $b; }
+EXPECT
+Possible precedence issue with control flow operator at - line 3.
+Possible precedence issue with control flow operator at - line 4.
+Possible precedence issue with control flow operator at - line 5.
+Possible precedence issue with control flow operator at - line 6.
+Possible precedence issue with control flow operator at - line 7.
+Possible precedence issue with control flow operator at - line 8.
+Possible precedence issue with control flow operator at - line 10.
+Possible precedence issue with control flow operator at - line 11.
Please sign in to comment.
Something went wrong with that request. Please try again.