Skip to content

Commit

Permalink
elide right operand if left operand transfers control
Browse files Browse the repository at this point in the history 
This commit contains the following changes:

- The "possible precedence issue" warning now mentions (in parentheses)
  the name of the control flow operator that triggered the warning:

      $ ./perl -cwe 'return $a or $b'
      Possible precedence issue with control flow operator (return) at -e line 1.

- CORE::dump now counts as a control flow operator:

      $ ./perl -cwe 'CORE::dump or f()'
      Possible precedence issue with control flow operator (dump) at -e line 1.

- Any binary operator whose left operand is a control flow operator is
  optimized out along with any right operand it may have, even if no
  warning is triggered:

      $ ./perl -Ilib -MO=Deparse -we '(die $a) + $b'
      BEGIN { $^W = 1; }
      die $a;
      -e syntax OK

  (No warnings because explicit parentheses are used in the preceding
  example.)

      $ ./perl -Ilib -MO=Deparse -we 'exit $a ? 0 : 1'
      Possible precedence issue with control flow operator (exit) at -e line 1.
      BEGIN { $^W = 1; }
      exit $a;
      -e syntax OK
  • Loading branch information
@mauke
mauke committed Nov 22, 2023
1 parent c66c3b9 commit b2bbe7d
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 48 deletions.
35 changes: 34 additions & 1 deletion lib/B/Deparse.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1857,7 +1857,7 @@ package foo;
CORE::do({});
CORE::do({});
####
# [perl #77096] functions that do not follow the llafr
# [perl #77096] functions that do not follow the looks-like-a-function rule
() = (return 1) + time;
() = (return ($1 + $2) * $3) + time;
() = (return ($a xor $b)) + time;
Expand All @@ -1877,6 +1877,26 @@ CORE::do({});
() = (-r $_) + 3;
() = (-w $_) + 3;
() = (-x $_) + 3;
>>>>
() = (return 1);
() = (return ($1 + $2) * $3);
() = (return ($a xor $b));
() = (do 'file') + time;
() = (do ($1 + $2) * $3) + time;
() = (do ($1 xor $2)) + time;
() = (goto 1);
() = (require 'foo') + 3;
() = (require foo) + 3;
() = (CORE::dump 1);
() = (last 1);
() = (next 1);
() = (redo 1);
() = (-R $_) + 3;
() = (-W $_) + 3;
() = (-X $_) + 3;
() = (-r $_) + 3;
() = (-w $_) + 3;
() = (-x $_) + 3;
####
# require(foo()) and do(foo())
require (foo());
Expand All @@ -1900,13 +1920,26 @@ $_ = ($a xor not +($1 || 2) ** 2);
() = warn;
() = warn() + 1;
() = setpgrp() + 1;
>>>>
() = (eof) + 1;
() = (return);
() = (return, 1);
() = warn;
() = warn() + 1;
() = setpgrp() + 1;
####
# loopexes have assignment prec
() = (CORE::dump a) | 'b';
() = (goto a) | 'b';
() = (last a) | 'b';
() = (next a) | 'b';
() = (redo a) | 'b';
>>>>
() = (CORE::dump a);
() = (goto a);
() = (last a);
() = (next a);
() = (redo a);
####
# [perl #63558] open local(*FH)
open local *FH;
Expand Down
46 changes: 27 additions & 19 deletions op.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4302,8 +4302,8 @@ Perl_invert(pTHX_ OP *o)
/* Warn about possible precedence issues if op is a control flow operator that
does not terminate normally (return, exit, next, etc).
*/
static void
S_check_terminates(pTHX_ OP *op)
static bool
S_is_control_transfer(pTHX_ OP *op)
{
assert(op != NULL);

Expand All @@ -4312,15 +4312,10 @@ S_check_terminates(pTHX_ OP *op)
$b)".
*/
switch (op->op_type) {
case OP_DUMP:
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_EXIT:
case OP_RETURN:
case OP_DIE:
Expand All @@ -4337,12 +4332,12 @@ S_check_terminates(pTHX_ OP *op)
*/
if (!op->op_folded && !(op->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;
"Possible precedence issue with control flow operator (%s)", OP_DESC(op));

return true;
}

return false;
}

OP *
Expand All @@ -4354,7 +4349,7 @@ Perl_cmpchain_start(pTHX_ I32 type, OP *left, OP *right)
if (!left)
left = newOP(OP_NULL, 0);
else
S_check_terminates(aTHX_ left);
(void)S_is_control_transfer(aTHX_ left);
if (!right)
right = newOP(OP_NULL, 0);
scalar(left);
Expand Down Expand Up @@ -5962,8 +5957,11 @@ Perl_newBINOP(pTHX_ I32 type, I32 flags, OP *first, OP *last)

if (!first)
first = newOP(OP_NULL, 0);
else
S_check_terminates(aTHX_ first);
else if (S_is_control_transfer(aTHX_ first)) {
op_free(last);
first->op_folded = 1;
return first;
}

NewOp(1101, binop, 1, BINOP);

Expand Down Expand Up @@ -8759,13 +8757,17 @@ S_new_logop(pTHX_ I32 type, I32 flags, OP** firstp, OP** otherp)
if (type == OP_XOR) /* Not short circuit, but here by precedence. */
return newBINOP(type, flags, scalar(first), scalar(other));

S_check_terminates(aTHX_ first);

assert((PL_opargs[type] & OA_CLASS_MASK) == OA_LOGOP
|| type == OP_CUSTOM);

scalarboolean(first);

if (S_is_control_transfer(aTHX_ first)) {
op_free(other);
first->op_folded = 1;
return first;
}

/* search for a constant op that could let us fold the test */
if ((cstop = search_const(first))) {
if (cstop->op_private & OPpCONST_STRICT)
Expand Down Expand Up @@ -8931,8 +8933,14 @@ Perl_newCONDOP(pTHX_ I32 flags, OP *first, OP *trueop, OP *falseop)
if (!trueop)
return newLOGOP(OP_OR, 0, first, falseop);

S_check_terminates(aTHX_ first);
scalarboolean(first);
if (S_is_control_transfer(aTHX_ first)) {
op_free(trueop);
op_free(falseop);
first->op_folded = 1;
return first;
}

if ((cstop = search_const(first))) {
/* Left or right arm of the conditional? */
const bool left = SvTRUE(cSVOPx(cstop)->op_sv);
Expand Down
2 changes: 1 addition & 1 deletion pod/perldiag.pod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5392,7 +5392,7 @@ Perl guesses a reasonable buffer size, but puts a sentinel byte at the
end of the buffer just in case. This sentinel byte got clobbered, and
Perl assumes that memory is now corrupted. See L<perlfunc/ioctl>.

=item Possible precedence issue with control flow operator
=item Possible precedence issue with control flow operator (%s)

(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
Expand Down
54 changes: 27 additions & 27 deletions t/lib/warnings/op
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1841,33 +1841,33 @@ sub do_warn_25 { redo $a xor $b while(1); }
sub do_warn_26 { $b if return $a; }
sub do_warn_27 { $b if die $a; }
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 9.
Possible precedence issue with control flow operator at - line 10.
Possible precedence issue with control flow operator at - line 11.
Possible precedence issue with control flow operator at - line 15.
Possible precedence issue with control flow operator at - line 16.
Possible precedence issue with control flow operator at - line 17.
Possible precedence issue with control flow operator at - line 18.
Possible precedence issue with control flow operator at - line 20.
Possible precedence issue with control flow operator at - line 21.
Possible precedence issue with control flow operator at - line 22.
Possible precedence issue with control flow operator at - line 23.
Possible precedence issue with control flow operator at - line 24.
Possible precedence issue with control flow operator at - line 25.
Possible precedence issue with control flow operator at - line 26.
Possible precedence issue with control flow operator at - line 27.
Possible precedence issue with control flow operator at - line 28.
Possible precedence issue with control flow operator at - line 29.
Possible precedence issue with control flow operator at - line 30.
Possible precedence issue with control flow operator at - line 31.
Possible precedence issue with control flow operator at - line 33.
Possible precedence issue with control flow operator at - line 34.
Possible precedence issue with control flow operator (return) at - line 3.
Possible precedence issue with control flow operator (return) at - line 4.
Possible precedence issue with control flow operator (return) at - line 5.
Possible precedence issue with control flow operator (die) at - line 6.
Possible precedence issue with control flow operator (die) at - line 7.
Possible precedence issue with control flow operator (die) at - line 8.
Possible precedence issue with control flow operator (exit) at - line 9.
Possible precedence issue with control flow operator (exit) at - line 10.
Possible precedence issue with control flow operator (exit) at - line 11.
Possible precedence issue with control flow operator (exit) at - line 15.
Possible precedence issue with control flow operator (exit) at - line 16.
Possible precedence issue with control flow operator (exit) at - line 17.
Possible precedence issue with control flow operator (exit) at - line 18.
Possible precedence issue with control flow operator (goto) at - line 20.
Possible precedence issue with control flow operator (goto) at - line 21.
Possible precedence issue with control flow operator (goto) at - line 22.
Possible precedence issue with control flow operator (next) at - line 23.
Possible precedence issue with control flow operator (next) at - line 24.
Possible precedence issue with control flow operator (next) at - line 25.
Possible precedence issue with control flow operator (last) at - line 26.
Possible precedence issue with control flow operator (last) at - line 27.
Possible precedence issue with control flow operator (last) at - line 28.
Possible precedence issue with control flow operator (redo) at - line 29.
Possible precedence issue with control flow operator (redo) at - line 30.
Possible precedence issue with control flow operator (redo) at - line 31.
Possible precedence issue with control flow operator (return) at - line 33.
Possible precedence issue with control flow operator (die) at - line 34.
########
# op.c
# (same as above, except these should not warn)
Expand Down

0 comments on commit b2bbe7d

Please sign in to comment.