Skip to content

Commit 8396546

Browse files
Androoha1Androoha1
authored
[Issue 411] - Fix and refactor ThrowIfRector rule (#415)
* Refactor the rule ThrowIfRector class. * Adjust the rule logic to skip cases with unsafe nodes. * Rector fixes. * Duster fixes. * Rename fixture class. * Renaming. * Organize transformation safety validation under a single method. * Rewrite the logic for safety-validation of variable accesses, don't just skip for any variable that was assigned in the if-condition, but only that could be not initialized because of the short-circuit issue. * rector. * duster.
1 parent f468123 commit 8396546

File tree

4 files changed

+105
-43
lines changed

4 files changed

+105
-43
lines changed

‎src/Rector/If_/ThrowIfRector.php‎

Lines changed: 64 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,16 @@
55
use PhpParser\Node;
66
use PhpParser\Node\Arg;
77
use PhpParser\Node\Expr;
8+
use PhpParser\Node\Expr\ArrayDimFetch;
89
use PhpParser\Node\Expr\Assign;
10+
use PhpParser\Node\Expr\BinaryOp\BooleanAnd;
11+
use PhpParser\Node\Expr\BinaryOp\BooleanOr;
912
use PhpParser\Node\Expr\BooleanNot;
1013
use PhpParser\Node\Expr\FuncCall;
14+
use PhpParser\Node\Expr\MethodCall;
15+
use PhpParser\Node\Expr\PropertyFetch;
16+
use PhpParser\Node\Expr\StaticCall;
17+
use PhpParser\Node\Expr\StaticPropertyFetch;
1118
use PhpParser\Node\Expr\Throw_;
1219
use PhpParser\Node\Expr\Variable;
1320
use PhpParser\Node\Name;
@@ -16,6 +23,7 @@
1623
use PhpParser\Node\Stmt\If_;
1724
use PhpParser\NodeVisitor;
1825
use Rector\NodeTypeResolver\Node\AttributeKey;
26+
use Rector\PhpParser\Node\BetterNodeFinder;
1927
use RectorLaravel\AbstractRector;
2028
use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
2129
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
@@ -25,6 +33,8 @@
2533
*/
2634
class ThrowIfRector extends AbstractRector
2735
{
36+
public function __construct(private readonly BetterNodeFinder $betterNodeFinder) {}
37+
2838
public function getRuleDefinition(): RuleDefinition
2939
{
3040
return new RuleDefinition('Change if throw to throw_if', [
@@ -46,51 +56,45 @@ public function getRuleDefinition(): RuleDefinition
4656
]);
4757
}
4858

59+
/**
60+
* @return array<class-string<Node>>
61+
*/
4962
public function getNodeTypes(): array
5063
{
5164
return [If_::class];
5265
}
5366

67+
/**
68+
* @param If_ $node
69+
*/
5470
public function refactor(Node $node): ?Node
5571
{
56-
if (! $node instanceof If_) {
57-
return null;
58-
}
59-
72+
/**
73+
* This is too conservative, can work with cases with elseif and else execution branches.
74+
* Skip for now for simplicity.
75+
*/
6076
if ($node->else instanceof Else_ || $node->elseifs !== []) {
6177
return null;
6278
}
6379

6480
$ifStmts = $node->stmts;
65-
66-
// Check if there's a single throw statement inside the if
67-
if (count($ifStmts) !== 1 || ! $ifStmts[0] instanceof Expression || ! $ifStmts[0]->expr instanceof Throw_) {
81+
// Check if there's a single throw expression inside the if-statement
82+
if (! (count($ifStmts) === 1 && $ifStmts[0] instanceof Expression && $ifStmts[0]->expr instanceof Throw_)) {
6883
return null;
6984
}
7085

71-
$condition = $node->cond;
86+
$ifCondition = $node->cond;
7287
$throwExpr = $ifStmts[0]->expr;
7388

74-
if ($this->exceptionUsesVariablesAssignedByCondition($throwExpr, $condition)) {
89+
if (! $this->isSafeToTransform($throwExpr, $ifCondition)) {
7590
return null;
7691
}
7792

78-
// Check if the condition is a negation
79-
if ($condition instanceof BooleanNot) {
80-
// Create a new throw_unless function call
81-
$funcCall = new FuncCall(new Name('throw_unless'), [
82-
new Arg($condition->expr),
83-
new Arg($throwExpr->expr),
84-
]);
85-
} else {
86-
// Create a new throw_if function call
87-
$funcCall = new FuncCall(new Name('throw_if'), [
88-
new Arg($condition),
89-
new Arg($throwExpr->expr),
90-
]);
91-
}
92-
93-
$expression = new Expression($funcCall);
93+
$expression = new Expression(
94+
$ifCondition instanceof BooleanNot
95+
? new FuncCall(new Name('throw_unless'), [new Arg($ifCondition->expr), new Arg($throwExpr->expr)])
96+
: new FuncCall(new Name('throw_if'), [new Arg($ifCondition), new Arg($throwExpr->expr)])
97+
);
9498

9599
$comments = array_merge($node->getComments(), $ifStmts[0]->getComments());
96100
if ($comments !== []) {
@@ -100,33 +104,50 @@ public function refactor(Node $node): ?Node
100104
return $expression;
101105
}
102106

103-
/**
104-
* Make sure the exception doesn't use variables assigned by the condition or this
105-
* will cause broken code to be generated
106-
*/
107-
private function exceptionUsesVariablesAssignedByCondition(Expr $throwExpr, Expr $condition): bool
107+
private function isSafeToTransform(Throw_ $throw, Expr $expr): bool
108108
{
109-
$conditionVariables = [];
110-
$returnValue = false;
109+
$shouldTransform = true;
110+
$bannedNodeTypes = [MethodCall::class, StaticCall::class, FuncCall::class, ArrayDimFetch::class, PropertyFetch::class, StaticPropertyFetch::class];
111+
$this->traverseNodesWithCallable($throw->expr, function (Node $node) use (&$shouldTransform, $bannedNodeTypes, $expr): ?int {
112+
if (
113+
in_array($node::class, $bannedNodeTypes, true)
114+
|| $node instanceof Variable && ! $this->isSafeToTransformWithVariableAccess($node, $expr)
115+
) {
116+
$shouldTransform = false;
111117

112-
$this->traverseNodesWithCallable($condition, function (Node $node) use (&$conditionVariables): null {
113-
if ($node instanceof Assign) {
114-
$conditionVariables[] = $this->getName($node->var);
118+
return NodeVisitor::STOP_TRAVERSAL;
115119
}
116120

117121
return null;
118122
});
119123

120-
$this->traverseNodesWithCallable($throwExpr, function (Node $node) use ($conditionVariables, &$returnValue): ?int {
121-
if ($node instanceof Variable && in_array($this->getName($node), $conditionVariables, true)) {
122-
$returnValue = true;
124+
return $shouldTransform;
125+
}
123126

124-
return NodeVisitor::STOP_TRAVERSAL;
125-
}
127+
/**
128+
* Not safe to transform when throw expression contains variables that are assigned in the if-condition because of the short-circuit logical operators issue.
129+
* This method checks if the variable was assigned on the right side of a short-circuit logical operator (conjunction and disjunction).
130+
* Note: The check is a little too strict, because such a variable may be initialized before the if-statement, and in such case it doesn't matter if it was assigned somewhere in the condition.
131+
*/
132+
private function isSafeToTransformWithVariableAccess(Variable $variable, Expr $expr): bool
133+
{
134+
$firstShortCircuitOperator = $this->betterNodeFinder->findFirst(
135+
$expr,
136+
fn (Node $node): bool => $node instanceof BooleanAnd || $node instanceof BooleanOr
137+
);
138+
if (! $firstShortCircuitOperator instanceof Node) {
139+
return true;
140+
}
141+
assert($firstShortCircuitOperator instanceof BooleanAnd || $firstShortCircuitOperator instanceof BooleanOr);
126142

127-
return null;
128-
});
143+
$varName = $this->getName($variable);
144+
$hasUnsafeAssignment = $this->betterNodeFinder->findFirst(
145+
$firstShortCircuitOperator->right, // only here short-circuit problem can happen
146+
fn (Node $node): bool => $node instanceof Assign
147+
&& $node->var instanceof Variable
148+
&& $this->getName($node->var) === $varName
149+
);
129150

130-
return $returnValue;
151+
return ! $hasUnsafeAssignment instanceof Node;
131152
}
132153
}

‎tests/Rector/If_/ThrowIfRector/Fixture/skip_unsafe_cases.php.inc‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<?php
2+
3+
namespace RectorLaravel\Tests\Rector\If_\ThrowIfRector\Fixture;
4+
5+
class SomeClass
6+
{
7+
public function handle($condition)
8+
{
9+
if ($condition) {
10+
throw new Exception('Some message: ' . foo());
11+
}
12+
if ($condition) {
13+
throw new Exception('Some message: ' . $object->someMethod());
14+
}
15+
if ($condition) {
16+
throw new Exception('Some message: ' . SomeClass::someMethod());
17+
}
18+
if ($condition) {
19+
throw new Exception('Some message: ' . $array[$someOffset]);
20+
}
21+
}
22+
}
23+
24+
?>

‎tests/Rector/If_/ThrowIfRector/Fixture/skip_where_condition_contains_assigns_var_for_exception.php.inc‎ renamed to ‎tests/Rector/If_/ThrowIfRector/Fixture/skip_unsafe_variable_assignment_in_if_condition.php.inc‎

File renamed without changes.

‎tests/Rector/If_/ThrowIfRector/Fixture/with_variable_assignment_in_if_condition.php.inc‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<?php
2+
3+
namespace RectorLaravel\Tests\Rector\If_\ThrowIfRector\Fixture;
4+
5+
if ($var = 5) {
6+
throw new \Exception($var);
7+
}
8+
9+
?>
10+
-----
11+
<?php
12+
13+
namespace RectorLaravel\Tests\Rector\If_\ThrowIfRector\Fixture;
14+
15+
throw_if($var = 5, new \Exception($var));
16+
17+
?>

0 commit comments

Comments
 (0)