Skip to content

Commit

Permalink
don't optimize (or warn about) control flow ops in OP_SASSIGN
Browse files Browse the repository at this point in the history
In short, '$a = return $b' is stored "backwards": OP_SASSIGN has the RHS
of the assignment as its first operand. Warning about precedence makes
no sense here.

Logical assignment ops like //=, ||=, &&= are represented as
OP_{DOR,OR,AND}ASSIGN wrapped around a weird single-child OP_SASSIGN.
Trying to naively elide the inner OP_SASSIGN leaves a dangling outer
OP_{DOR,OR,AND}ASSIGN (which B::Deparse doesn't know what to do with) or
just makes the compiler loop indefinitely. So just don't.

Fixes #21665.
  • Loading branch information
mauke committed Nov 27, 2023
1 parent 23e1e44 commit ded9a6e
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 1 deletion.
7 changes: 7 additions & 0 deletions lib/B/Deparse.t
Expand Up @@ -3384,3 +3384,10 @@ my(@x) = (-2.0, -1.0, -0.0, 0.0, 1.0, 2.0);
# PADSV_STORE optimised state should be handled
# CONTEXT use feature "state";
() = (state $s = 1);
####
# control transfer in RHS of assignment
my $x;
$x = (return 'ok');
$x //= (return 'ok');
$x = exit 42;
$x //= exit 42;
16 changes: 15 additions & 1 deletion op.c
Expand Up @@ -5957,7 +5957,21 @@ Perl_newBINOP(pTHX_ I32 type, I32 flags, OP *first, OP *last)

if (!first)
first = newOP(OP_NULL, 0);
else if (S_is_control_transfer(aTHX_ first)) {
else if (type != OP_SASSIGN && S_is_control_transfer(aTHX_ first)) {
/* Skip OP_SASSIGN.
* '$x = return 42' is represented by (SASSIGN (RETURN 42) (GVSV *x));
* in other words, OP_SASSIGN has its operands "backwards". Skip the
* control transfer check because '$x = return $y' is not a precedence
* issue (the '$x =' part has no runtime effect no matter how you
* parenthesize it).
* Also, don't try to optimize the OP_SASSIGN case because the logical
* assignment ops like //= are represented by an OP_{AND,OR,DOR}ASSIGN
* containing an OP_SASSIGN with a single child (first == last):
* '$x //= return 42' is (DORASSIGN (GVSV *x) (SASSIGN (RETURN 42))).
* Naively eliminating the OP_ASSIGN leaves the incomplete (DORASSIGN
* (GVSV *x) (RETURN 42)), which e.g. B::Deparse doesn't handle.
*/
assert(first != last);
op_free(last);
first->op_folded = 1;
return first;
Expand Down
7 changes: 7 additions & 0 deletions t/lib/warnings/op
Expand Up @@ -1871,6 +1871,7 @@ Possible precedence issue with control flow operator (die) at - line 34.
########
# op.c
# (same as above, except these should not warn)
use warnings;
use constant FEATURE => 1;
use constant MISSING_FEATURE => 0;

Expand Down Expand Up @@ -1924,6 +1925,12 @@ sub dont_warn_43 { last ($a xor $b) while(1); }
sub dont_warn_44 { redo ($a or $b) while(1); }
sub dont_warn_45 { redo ($a and $b) while(1); }
sub dont_warn_46 { redo ($a xor $b) while(1); }

# These are not operator precedence issues (even though assignment internally
# stores its RHS as the first operand)
sub dont_warn_47 { $a = return $b; }
sub dont_warn_48 { $a //= return $b; }
sub dont_warn_49 { $a &&= exit $b; }
EXPECT
########
use feature "signatures";
Expand Down

0 comments on commit ded9a6e

Please sign in to comment.