Incorrect behavior of LogicalToBooleanRector: missing parentheses around ternary, Elvis, composed assignments, 'print', 'yield' or 'yield from'

[glop-glop]

Bug Report

Subject	Details
Rector version	both last dev-main (rectorphp/rector-src@269af160) and dev-fix-logical-to-boolean-assign-parens (from rectorphp/rector-src#8152)

Hello,

I'm really sorry for these three bug reports in a row. I report them when I stumble upon them, but here is another one, linked to #9797 and #9799, still related to missing parentheses after applying LogicalToBooleanRector, this time mixing a logical operator with a ternary operator, an Elvis (short ternary) operator, a composed assignment operator, or a print, yield or yield from construct. (I've tried looking for similar issues to try and group them into a single bug report.)

Minimal PHP Code Causing Issue

See https://getrector.com/demo/56bb01d0-6dba-425d-bfc6-dec330d5d35f

This example regroups all the bugs I found. I've also added the value of the PHP expressions as a comment after each line:

<?php

# Ternary operator, left.
(true ? 42 : 0) and true; # -> true

# Ternary operator, right.
true and (true ? 42 : 0); # -> true

# Elvis operator, left.
$x = 42;
($x ?: 0) and true; # -> true

# Elvis operator, right.
$x = 0;
true and ($x ?: 42); # -> true

# Combined assignment operator, left.
$x = 0;
($x += 42) and true; # -> true, and $x is 42

# Combined assignment operator, middle.
$x = 0;
true and ($x += 42) and true; # -> true, and $x is 42

# 'print', left.
(print "foo") and (print "bar"); # -> true, prints "foobar"

# 'print', middle.
true and (print "foo") or (print "bar"); # -> true, prints "foo"

# 'yield', left.
(yield "foo") or (yield "bar"); # -> false, yields "foo" then "bar"

# 'yield', middle.
true and (yield "foo") or (yield "bar"); # -> false, yields "foo" then "bar"

# 'yield from', left.
(yield from ["foo"]) or (yield from ["bar"]); # -> false, yields "foo" then "bar"

# 'yield from', middle.
true and (yield from ["foo"]) or (yield from ["bar"]); # -> false, yields "foo" then "bar"

(In all the examples above, the parentheses are optional, as and has lower precedence than the parenthesized operators. In any case, with or without the parentheses in the input code, Rector behaves in the same way and produces exactly the same output, without any parentheses. Also, I have used += as an example, but all other combined assignment operators have the same issue.)

Except for the comments (which I've adapted to show the different values), Rector will indeed rewrite the above code as:

<?php

# Ternary operator, left.
true ? 42 : 0 && true; # -> 42, instead of true

# Ternary operator, right.
true && true ? 42 : 0; # -> 42, instead of true

# Elvis operator, left.
$x = 42;
$x ?: 0 && true; # -> 42, instead of true

# Elvis operator, right.
$x = 0;
true && $x ?: 42; # -> 42, instead of true

# Combined assignment operator, left.
$x = 0;
$x += 42 && true; # -> true, but $x is 1 instead of 42

# Combined assignment operator, middle.
$x = 0;
true && $x += 42 && true; # -> true, but $x is 1 instead of 42

# 'print', left.
print "foo" && print "bar"; # -> 1 instead of true, and prints "bar1" instead of "foobar"

# 'print', middle.
true && print "foo" || print "bar"; # -> true, but prints "1" instead of "foo"

# 'yield', left.
yield "foo" || yield "bar"; # -> null instead of false, and yields true instead of "foo" then "bar"

# 'yield', middle.
true && yield "foo" || yield "bar"; # -> false, but yields true instead of "foo" then "bar"

# 'yield from', left.
yield from ["foo"] || yield from ["bar"]; # PHP Fatal error:  Uncaught Error: Can use "yield from" only with arrays and Traversables

# 'yield from', middle.
true && yield from ["foo"] || yield from ["bar"]; # PHP Fatal error:  Uncaught Error: Can use "yield from" only with arrays and Traversables

Responsible rule: LogicalToBooleanRector

Expected Behaviour

The correct code should keep (or add) parentheses around expressions whose operator has a lower precedence than the boolean ones. That is, something like:

<?php

# Ternary operator, left.
(true ? 42 : 0) && true; # -> true (OK)

# Ternary operator, right.
true && (true ? 42 : 0); # -> true (OK)

# Elvis operator, left.
$x = 42;
($x ?: 0) && true; # -> true (OK)

# Elvis operator, right.
$x = 0;
true && ($x ?: 42); # -> true (OK)

# Combined assignment operator, left.
$x = 0;
($x += 42) && true; # -> true, and $x is 42 (OK)

# Combined assignment operator, middle.
$x = 0;
true && ($x += 42) && true; # -> true, and $x is 42 (OK)

# 'print', left.
(print "foo") && print "bar"; # -> true, prints "foobar" (OK)

# 'print', middle.
true && (print "foo") || print "bar"; # -> true, prints "foo" (OK)

# 'yield', left.
(yield "foo") || yield "bar"; # -> false, yields "foo" then "bar" (OK)

# 'yield', middle.
true && (yield "foo") || yield "bar"; # -> false, yields "foo" then "bar" (OK)

# 'yield from', left.
(yield from ["foo"]) || yield from ["bar"]; # -> false, yields "foo" then "bar" (OK)

# 'yield from', middle.
true && (yield from ["foo"]) || yield from ["bar"]; # -> false, yields "foo" then "bar" (OK)

(Note that the right-most expressions in the print, yield and yield from examples may also be parenthesized: it's not mandatory, but it may improve the code readability.)

Once again, I'm not sure, but looking at lines 475 and 481 of BetterStandardPrinter.php, maybe the tests should compare the left- and right-hand nodes with other types than just Assign, such as AssignOp, Ternary, Print_, Yield_ and YieldFrom?

(But then again, caution should be exercised so that parentheses on the left-hand side are not removed if they were already present, such as was the case in #9799. Also, from what I've tried, Yield_ seems to behave slightly differently, and testing if the left- of right-hand nodes are instances of Yield_ didn't solve the issue.)

Note: in order to try to be as exhaustive as possible in this bug report, I've looked at all PHP operators whose precedence is higher than and (resp. or) but lower than && (resp. ||): https://www.php.net/manual/en/language.operators.precedence.php

I haven't investigated this, but strangely enough, it turns out that two operators in this case are not affected by this bug and are parenthesized correctly by Rector: xor and null-coalescing (??). See for instance: https://getrector.com/demo/e741b34b-001a-4842-95b1-12c7614223f0

Well, I hope this will help closing this LogicalToBooleanRector series of bugs for good! :)

Thank you very much, in any case!

Cheers,
Glop

[TomasVotruba]

No worries, keep on testing :).
We don't want to cause any bugs with this rule, so the more testing the better 👍

I've shipped another PR, give it a go

[glop-glop]

Wow, great, thank you so much! Once again, I've never seen issues being solved at such a rate, that's amazing! :)

I've tested your patch from rectorphp/rector-src#8155, and everything seems perfectly fine for the above test cases, fantastic! 🎉

However, while wondering if there was still any reason to keep lines 481-489, I realized there was another rule which changed operator precedence, PowToExpRector, and that it was missing some cases as well. I'll open another issue on the matter.

Thank you!

Cheers,
Glop