Skip to content

Commit 1ccbb46

Browse files
authored
[CodeQuality] Preserve parentheses around low-precedence operands in LogicalToBooleanRector (#8155)
Fixes rectorphp/rector#9800 Boolean operators (&& ||) bind tighter than logical ones (and or). When LogicalToBooleanRector swaps them, operands whose top operator has lower precedence (ternary, elvis, +=/??=, print, yield, yield from) lost their parentheses on reprint, changing semantics. Reprint those operands so the pretty-printer re-adds the parentheses; yield <value> additionally needs them forced, as the format-preserving printer drops them.
1 parent ca5155c commit 1ccbb46

6 files changed

Lines changed: 104 additions & 0 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<?php
2+
3+
$x = 0;
4+
($x += 42) and true;
5+
6+
?>
7+
-----
8+
<?php
9+
10+
$x = 0;
11+
($x += 42) && true;
12+
13+
?>
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<?php
2+
3+
$z = 0;
4+
($z ?: 7) and true;
5+
6+
?>
7+
-----
8+
<?php
9+
10+
$z = 0;
11+
($z ?: 7) && true;
12+
13+
?>
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<?php
2+
3+
(print "foo") and (print "bar");
4+
5+
?>
6+
-----
7+
<?php
8+
9+
(print "foo") && print "bar";
10+
11+
?>
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
<?php
2+
3+
(true ? 42 : 0) and true;
4+
5+
?>
6+
-----
7+
<?php
8+
9+
(true ? 42 : 0) && true;
10+
11+
?>
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
<?php
2+
3+
function run()
4+
{
5+
(yield 1) and true;
6+
(yield from [1, 2]) and true;
7+
}
8+
9+
?>
10+
-----
11+
<?php
12+
13+
function run()
14+
{
15+
(yield 1) && true;
16+
(yield from [1, 2]) && true;
17+
}
18+
19+
?>

src/PhpParser/Printer/BetterStandardPrinter.php

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,19 @@
1414
use PhpParser\Node\Expr\Array_;
1515
use PhpParser\Node\Expr\ArrowFunction;
1616
use PhpParser\Node\Expr\Assign;
17+
use PhpParser\Node\Expr\AssignOp;
18+
use PhpParser\Node\Expr\AssignRef;
1719
use PhpParser\Node\Expr\BinaryOp;
20+
use PhpParser\Node\Expr\BinaryOp\BooleanAnd;
21+
use PhpParser\Node\Expr\BinaryOp\BooleanOr;
1822
use PhpParser\Node\Expr\BinaryOp\Pipe;
1923
use PhpParser\Node\Expr\CallLike;
2024
use PhpParser\Node\Expr\Instanceof_;
2125
use PhpParser\Node\Expr\MethodCall;
26+
use PhpParser\Node\Expr\Print_;
2227
use PhpParser\Node\Expr\Ternary;
2328
use PhpParser\Node\Expr\Yield_;
29+
use PhpParser\Node\Expr\YieldFrom;
2430
use PhpParser\Node\InterpolatedStringPart;
2531
use PhpParser\Node\Scalar\InterpolatedString;
2632
use PhpParser\Node\Scalar\String_;
@@ -482,6 +488,14 @@ private function wrapBinaryOpWithBrackets(Node $node): void
482488
$node->right->setAttribute(AttributeKey::ORIGINAL_NODE, null);
483489
}
484490

491+
// boolean operators (&&, ||) bind tighter than the logical ones (and, or); when
492+
// LogicalToBooleanRector swaps them, operands with lower precedence must be reprinted
493+
// so the pretty-printer re-adds the parentheses that keep the original semantics
494+
if ($node instanceof BooleanAnd || $node instanceof BooleanOr) {
495+
$this->reprintLowerPrecedenceOperand($node->left);
496+
$this->reprintLowerPrecedenceOperand($node->right);
497+
}
498+
485499
if ($node->left instanceof BinaryOp &&
486500
$node->left->getAttribute(AttributeKey::ORIGINAL_NODE) instanceof Node) {
487501
$node->left->setAttribute(AttributeKey::ORIGINAL_NODE, null);
@@ -493,6 +507,29 @@ private function wrapBinaryOpWithBrackets(Node $node): void
493507
}
494508
}
495509

510+
/**
511+
* @see https://github.com/rectorphp/rector/issues/9800
512+
*/
513+
private function reprintLowerPrecedenceOperand(Node $node): void
514+
{
515+
if (! $node instanceof AssignRef
516+
&& ! $node instanceof AssignOp
517+
&& ! $node instanceof Ternary
518+
&& ! $node instanceof Print_
519+
&& ! $node instanceof Yield_
520+
&& ! $node instanceof YieldFrom) {
521+
return;
522+
}
523+
524+
$node->setAttribute(AttributeKey::ORIGINAL_NODE, null);
525+
526+
// the format-preserving printer keeps yield's precedence parentheses only for the
527+
// "yield from" and empty "yield" forms; "yield <value>" needs them added explicitly
528+
if ($node instanceof Yield_ && $node->value instanceof Expr) {
529+
$node->setAttribute(AttributeKey::WRAPPED_IN_PARENTHESES, true);
530+
}
531+
}
532+
496533
/**
497534
* ensure left side is assign and right side is just created
498535
*

0 commit comments

Comments
 (0)