Skip to content

Fix missing detection of dead code in expressions #4090

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ondrejmirtes merged 28 commits into from
Jul 21, 2025
Merged

Fix missing detection of dead code in expressions #4090

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
bda643b
Fix missing detection of dead code in expressions
staabm Jul 11, 2025
a3a6a49
Support method calls / static calls
staabm Jul 11, 2025
c2b3e7b
fix assignments
staabm Jul 11, 2025
a430885
fix regression
staabm Jul 11, 2025
074877a
Fix NullsafeMethodCall
staabm Jul 11, 2025
1707b28
fix build
staabm Jul 11, 2025
6023250
test nullsafe
staabm Jul 11, 2025
5bf7349
more tests
staabm Jul 11, 2025
82c93d4
fix PHP 7.4
staabm Jul 11, 2025
0cc9483
better names
staabm Jul 11, 2025
af69075
fix string interpolation
staabm Jul 12, 2025
0610269
fix func args
staabm Jul 12, 2025
0b29958
fix callables
staabm Jul 12, 2025
0b642df
added regression test
staabm Jul 12, 2025
e7d00d1
fix arrays
staabm Jul 12, 2025
dfe77ba
fix method calls
staabm Jul 12, 2025
95bb39d
ExpressionResult isAlwaysTerminating is consistent with StatementResult
staabm Jul 17, 2025
4e0c9c0
Added ExpressionResultTest
staabm Jul 17, 2025
89d5780
Add $isAlwaysTerminating to every node-type if-branch
staabm Jul 17, 2025
f18867b
fix build
staabm Jul 17, 2025
4e00904
fix arrow-fn and suppress
staabm Jul 17, 2025
d0d52d9
implement more cases
staabm Jul 17, 2025
cc65f3b
Update NodeScopeResolver.php
staabm Jul 17, 2025
70d67c2
Update NodeScopeResolver.php
staabm Jul 17, 2025
24ff8fd
fix nullsafe
staabm Jul 17, 2025
ba52625
fix expectations
staabm Jul 21, 2025
8b2806a
simplify ExpressionResultTest
staabm Jul 21, 2025
15f2b01
add more tests
staabm Jul 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add $isAlwaysTerminating to every node-type if-branch
  • Loading branch information
@staabm
staabm committed Jul 21, 2025
commit 89d5780e0eb890600da4dfadb2395319b0243cb8
49 changes: 47 additions & 2 deletions src/Analyser/NodeScopeResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2415,13 +2415,13 @@ public function processExprNode(Node\Stmt $stmt, Expr $expr, MutatingScope $scop
return $this->processExprNode($stmt, $newExpr, $scope, $nodeCallback, $context);
}

$isAlwaysTerminating = false;
$this->callNodeCallbackWithExpression($nodeCallback, $expr, $scope, $context);

if ($expr instanceof Variable) {
$hasYield = false;
$throwPoints = [];
$impurePoints = [];
$isAlwaysTerminating = false;
if ($expr->name instanceof Expr) {
return $this->processExprNode($stmt, $expr->name, $scope, $nodeCallback, $context->enterDeep());
} elseif (in_array($expr->name, Scope::SUPERGLOBAL_VARIABLES, true)) {
Expand Down Expand Up @@ -2539,6 +2539,7 @@ function (MutatingScope $scope) use ($stmt, $expr, $nodeCallback, $context): Exp
$functionReflection = null;
$throwPoints = [];
$impurePoints = [];
$isAlwaysTerminating = false;
Comment thread
staabm marked this conversation as resolved.
Resolved
if ($expr->name instanceof Expr) {
$nameType = $scope->getType($expr->name);
if (!$nameType->isCallable()->no()) {
Expand Down Expand Up @@ -2943,6 +2944,7 @@ static function (): void {
$hasYield = false;
$throwPoints = [];
$impurePoints = [];
$isAlwaysTerminating = false;
Comment thread
staabm marked this conversation as resolved.
Resolved
if ($expr->class instanceof Expr) {
$objectClasses = $scope->getType($expr->class)->getObjectClassNames();
if (count($objectClasses) !== 1) {
Expand Down Expand Up @@ -3106,6 +3108,7 @@ static function (): void {
$hasYield = $result->hasYield();
$throwPoints = $result->getThrowPoints();
$impurePoints = $result->getImpurePoints();
$isAlwaysTerminating = false;
Comment thread
staabm marked this conversation as resolved.
Resolved
$scope = $result->getScope();
if ($expr->name instanceof Expr) {
$result = $this->processExprNode($stmt, $expr->name, $scope, $nodeCallback, $context->enterDeep());
Expand Down Expand Up @@ -3154,6 +3157,7 @@ static function (): void {
true,
),
];
$isAlwaysTerminating = false;
if ($expr->class instanceof Expr) {
$result = $this->processExprNode($stmt, $expr->class, $scope, $nodeCallback, $context->enterDeep());
$hasYield = $result->hasYield();
Expand Down Expand Up @@ -3192,6 +3196,7 @@ static function (): void {
$hasYield = $result->hasYield();
$throwPoints = $result->getThrowPoints();
$impurePoints = $result->getImpurePoints();
$isAlwaysTerminating = false;
$scope = $result->getScope();
} elseif ($expr instanceof Exit_) {
$hasYield = false;
Expand All @@ -3213,6 +3218,7 @@ static function (): void {
$hasYield = false;
$throwPoints = [];
$impurePoints = [];
$isAlwaysTerminating = false;
foreach ($expr->parts as $part) {
if (!$part instanceof Expr) {
continue;
Expand All @@ -3228,6 +3234,7 @@ static function (): void {
$hasYield = false;
$throwPoints = [];
$impurePoints = [];
$isAlwaysTerminating = false;
Comment thread
staabm marked this conversation as resolved.
Resolved
if ($expr->dim !== null) {
$result = $this->processExprNode($stmt, $expr->dim, $scope, $nodeCallback, $context->enterDeep());
$hasYield = $result->hasYield();
Expand All @@ -3246,6 +3253,7 @@ static function (): void {
$hasYield = false;
$throwPoints = [];
$impurePoints = [];
$isAlwaysTerminating = false;
foreach ($expr->items as $arrayItem) {
$itemNodes[] = new LiteralArrayItem($scope, $arrayItem);
$nodeCallback($arrayItem, $scope);
Expand Down Expand Up @@ -3327,6 +3335,7 @@ static function (): void {
$hasYield = $condResult->hasYield() || $rightResult->hasYield();
$throwPoints = array_merge($condResult->getThrowPoints(), $rightResult->getThrowPoints());
$impurePoints = array_merge($condResult->getImpurePoints(), $rightResult->getImpurePoints());
$isAlwaysTerminating = false;
} elseif ($expr instanceof BinaryOp) {
$result = $this->processExprNode($stmt, $expr->left, $scope, $nodeCallback, $context->enterDeep());
$scope = $result->getScope();
Expand Down Expand Up @@ -3359,11 +3368,13 @@ static function (): void {
true,
);
$hasYield = $result->hasYield();
$isAlwaysTerminating = false;
$scope = $result->getScope()->afterExtractCall();
} elseif ($expr instanceof Expr\Print_) {
$result = $this->processExprNode($stmt, $expr->expr, $scope, $nodeCallback, $context->enterDeep());
$throwPoints = $result->getThrowPoints();
$impurePoints = $result->getImpurePoints();
$isAlwaysTerminating = $result->isAlwaysTerminating();
$impurePoints[] = new ImpurePoint($scope, $expr, 'print', 'print', true);
$hasYield = $result->hasYield();

Expand All @@ -3372,6 +3383,7 @@ static function (): void {
$result = $this->processExprNode($stmt, $expr->expr, $scope, $nodeCallback, $context->enterDeep());
$throwPoints = $result->getThrowPoints();
$impurePoints = $result->getImpurePoints();
$isAlwaysTerminating = true;
$hasYield = $result->hasYield();

$exprType = $scope->getType($expr->expr);
Expand Down Expand Up @@ -3399,6 +3411,7 @@ static function (): void {
$result = $this->processExprNode($stmt, $expr->expr, $scope, $nodeCallback, $context->enterDeep());
$throwPoints = $result->getThrowPoints();
$impurePoints = $result->getImpurePoints();
$isAlwaysTerminating = $result->isAlwaysTerminating();
$hasYield = $result->hasYield();

$scope = $result->getScope();
Expand All @@ -3409,6 +3422,7 @@ static function (): void {
$impurePoints = $result->getImpurePoints();
$impurePoints[] = new ImpurePoint($scope, $expr, 'eval', 'eval', true);
$hasYield = $result->hasYield();
$isAlwaysTerminating = $result->isAlwaysTerminating();

$scope = $result->getScope();
} elseif ($expr instanceof Expr\YieldFrom) {
Expand All @@ -3424,6 +3438,7 @@ static function (): void {
true,
);
$hasYield = true;
$isAlwaysTerminating = $result->isAlwaysTerminating();

$scope = $result->getScope();
} elseif ($expr instanceof BooleanNot) {
Expand All @@ -3432,7 +3447,10 @@ static function (): void {
$hasYield = $result->hasYield();
$throwPoints = $result->getThrowPoints();
$impurePoints = $result->getImpurePoints();
$isAlwaysTerminating = $result->isAlwaysTerminating();
} elseif ($expr instanceof Expr\ClassConstFetch) {
$isAlwaysTerminating = false;

if ($expr->class instanceof Expr) {
$result = $this->processExprNode($stmt, $expr->class, $scope, $nodeCallback, $context->enterDeep());
$scope = $result->getScope();
Expand Down Expand Up @@ -3463,13 +3481,15 @@ static function (): void {
$hasYield = $result->hasYield();
$throwPoints = $result->getThrowPoints();
$impurePoints = $result->getImpurePoints();
$isAlwaysTerminating = $result->isAlwaysTerminating();
$scope = $this->revertNonNullability($scope, $nonNullabilityResult->getSpecifiedExpressions());
$scope = $this->lookForUnsetAllowedUndefinedExpressions($scope, $expr->expr);
} elseif ($expr instanceof Expr\Isset_) {
$hasYield = false;
$throwPoints = [];
$impurePoints = [];
$nonNullabilityResults = [];
$isAlwaysTerminating = false;
foreach ($expr->vars as $var) {
$nonNullabilityResult = $this->ensureNonNullability($scope, $var);
$scope = $this->lookForSetAllowedUndefinedExpressions($nonNullabilityResult->getScope(), $var);
Expand All @@ -3478,6 +3498,7 @@ static function (): void {
$hasYield = $hasYield || $result->hasYield();
$throwPoints = array_merge($throwPoints, $result->getThrowPoints());
$impurePoints = array_merge($impurePoints, $result->getImpurePoints());
$isAlwaysTerminating = $isAlwaysTerminating || $result->isAlwaysTerminating();
$nonNullabilityResults[] = $nonNullabilityResult;
}
foreach (array_reverse($expr->vars) as $var) {
Expand All @@ -3492,6 +3513,7 @@ static function (): void {
$hasYield = $result->hasYield();
$throwPoints = $result->getThrowPoints();
$impurePoints = $result->getImpurePoints();
$isAlwaysTerminating = $result->isAlwaysTerminating();
if ($expr->class instanceof Expr) {
$result = $this->processExprNode($stmt, $expr->class, $scope, $nodeCallback, $context->enterDeep());
$scope = $result->getScope();
Expand All @@ -3508,6 +3530,7 @@ static function (): void {
$hasYield = false;
$throwPoints = [];
$impurePoints = [];
$isAlwaysTerminating = false;
$className = null;
if ($expr->class instanceof Expr || $expr->class instanceof Name) {
if ($expr->class instanceof Expr) {
Expand Down Expand Up @@ -3632,6 +3655,7 @@ static function (): void {
$hasYield = $result->hasYield();
$throwPoints = $result->getThrowPoints();
$impurePoints = $result->getImpurePoints();
$isAlwaysTerminating = $result->isAlwaysTerminating();

$newExpr = $expr;
if ($expr instanceof Expr\PostInc) {
Expand Down Expand Up @@ -3660,20 +3684,23 @@ static function (Node $node, Scope $scope) use ($nodeCallback): void {
$ternaryCondResult = $this->processExprNode($stmt, $expr->cond, $scope, $nodeCallback, $context->enterDeep());
$throwPoints = $ternaryCondResult->getThrowPoints();
$impurePoints = $ternaryCondResult->getImpurePoints();
$isAlwaysTerminating = $ternaryCondResult->isAlwaysTerminating();
$ifTrueScope = $ternaryCondResult->getTruthyScope();
$ifFalseScope = $ternaryCondResult->getFalseyScope();
$ifTrueType = null;
if ($expr->if !== null) {
$ifResult = $this->processExprNode($stmt, $expr->if, $ifTrueScope, $nodeCallback, $context);
$throwPoints = array_merge($throwPoints, $ifResult->getThrowPoints());
$impurePoints = array_merge($impurePoints, $ifResult->getImpurePoints());
$isAlwaysTerminating = $isAlwaysTerminating || $ifResult->isAlwaysTerminating();
$ifTrueScope = $ifResult->getScope();
$ifTrueType = $ifTrueScope->getType($expr->if);
}

$elseResult = $this->processExprNode($stmt, $expr->else, $ifFalseScope, $nodeCallback, $context);
$throwPoints = array_merge($throwPoints, $elseResult->getThrowPoints());
$impurePoints = array_merge($impurePoints, $elseResult->getImpurePoints());
$isAlwaysTerminating = $isAlwaysTerminating || $elseResult->isAlwaysTerminating();
$ifFalseScope = $elseResult->getScope();

$condType = $scope->getType($expr->cond);
Expand All @@ -3698,7 +3725,7 @@ static function (Node $node, Scope $scope) use ($nodeCallback): void {
return new ExpressionResult(
$finalScope,
$ternaryCondResult->hasYield(),
false,
$isAlwaysTerminating,
$throwPoints,
$impurePoints,
static fn (): MutatingScope => $finalScope->filterByTruthyValue($expr),
Expand All @@ -3718,17 +3745,20 @@ static function (Node $node, Scope $scope) use ($nodeCallback): void {
true,
),
];
$isAlwaysTerminating = false;
if ($expr->key !== null) {
$keyResult = $this->processExprNode($stmt, $expr->key, $scope, $nodeCallback, $context->enterDeep());
$scope = $keyResult->getScope();
$throwPoints = $keyResult->getThrowPoints();
$impurePoints = array_merge($impurePoints, $keyResult->getImpurePoints());
$isAlwaysTerminating = $isAlwaysTerminating || $keyResult->isAlwaysTerminating();
}
if ($expr->value !== null) {
$valueResult = $this->processExprNode($stmt, $expr->value, $scope, $nodeCallback, $context->enterDeep());
$scope = $valueResult->getScope();
$throwPoints = array_merge($throwPoints, $valueResult->getThrowPoints());
$impurePoints = array_merge($impurePoints, $valueResult->getImpurePoints());
$isAlwaysTerminating = $isAlwaysTerminating || $valueResult->isAlwaysTerminating();
}
$hasYield = true;
} elseif ($expr instanceof Expr\Match_) {
Expand All @@ -3739,6 +3769,7 @@ static function (Node $node, Scope $scope) use ($nodeCallback): void {
$hasYield = $condResult->hasYield();
$throwPoints = $condResult->getThrowPoints();
$impurePoints = $condResult->getImpurePoints();
$isAlwaysTerminating = $condResult->isAlwaysTerminating();
$matchScope = $scope->enterMatch($expr);
$armNodes = [];
$hasDefaultCond = false;
Expand Down Expand Up @@ -3950,79 +3981,93 @@ static function (Node $node, Scope $scope) use ($nodeCallback): void {
$hasYield = $result->hasYield();
$throwPoints = $result->getThrowPoints();
$impurePoints = $result->getImpurePoints();
$isAlwaysTerminating = $result->isAlwaysTerminating();
$scope = $result->getScope();
} elseif ($expr instanceof Expr\Throw_) {
$hasYield = false;
$result = $this->processExprNode($stmt, $expr->expr, $scope, $nodeCallback, ExpressionContext::createDeep());
$throwPoints = $result->getThrowPoints();
$impurePoints = $result->getImpurePoints();
$isAlwaysTerminating = $result->isAlwaysTerminating();
$throwPoints[] = ThrowPoint::createExplicit($scope, $scope->getType($expr->expr), $expr, false);
} elseif ($expr instanceof FunctionCallableNode) {
$throwPoints = [];
$impurePoints = [];
$hasYield = false;
$isAlwaysTerminating = false;
if ($expr->getName() instanceof Expr) {
$result = $this->processExprNode($stmt, $expr->getName(), $scope, $nodeCallback, ExpressionContext::createDeep());
$scope = $result->getScope();
$hasYield = $result->hasYield();
$throwPoints = $result->getThrowPoints();
$impurePoints = $result->getImpurePoints();
$isAlwaysTerminating = $result->isAlwaysTerminating();
}
} elseif ($expr instanceof MethodCallableNode) {
$result = $this->processExprNode($stmt, $expr->getVar(), $scope, $nodeCallback, ExpressionContext::createDeep());
$scope = $result->getScope();
$hasYield = $result->hasYield();
$throwPoints = $result->getThrowPoints();
$impurePoints = $result->getImpurePoints();
$isAlwaysTerminating = false;
if ($expr->getName() instanceof Expr) {
$nameResult = $this->processExprNode($stmt, $expr->getVar(), $scope, $nodeCallback, ExpressionContext::createDeep());
$scope = $nameResult->getScope();
$hasYield = $hasYield || $nameResult->hasYield();
$throwPoints = array_merge($throwPoints, $nameResult->getThrowPoints());
$impurePoints = array_merge($impurePoints, $nameResult->getImpurePoints());
$isAlwaysTerminating = $nameResult->isAlwaysTerminating();
}
} elseif ($expr instanceof StaticMethodCallableNode) {
$throwPoints = [];
$impurePoints = [];
$hasYield = false;
$isAlwaysTerminating = false;
if ($expr->getClass() instanceof Expr) {
$classResult = $this->processExprNode($stmt, $expr->getClass(), $scope, $nodeCallback, ExpressionContext::createDeep());
$scope = $classResult->getScope();
$hasYield = $classResult->hasYield();
$throwPoints = $classResult->getThrowPoints();
$impurePoints = $classResult->getImpurePoints();
$isAlwaysTerminating = $isAlwaysTerminating || $classResult->isAlwaysTerminating();
}
if ($expr->getName() instanceof Expr) {
$nameResult = $this->processExprNode($stmt, $expr->getName(), $scope, $nodeCallback, ExpressionContext::createDeep());
$scope = $nameResult->getScope();
$hasYield = $hasYield || $nameResult->hasYield();
$throwPoints = array_merge($throwPoints, $nameResult->getThrowPoints());
$impurePoints = array_merge($impurePoints, $nameResult->getImpurePoints());
$isAlwaysTerminating = $isAlwaysTerminating || $nameResult->isAlwaysTerminating();
}
} elseif ($expr instanceof InstantiationCallableNode) {
$throwPoints = [];
$impurePoints = [];
$hasYield = false;
$isAlwaysTerminating = false;
if ($expr->getClass() instanceof Expr) {
$classResult = $this->processExprNode($stmt, $expr->getClass(), $scope, $nodeCallback, ExpressionContext::createDeep());
$scope = $classResult->getScope();
$hasYield = $classResult->hasYield();
$throwPoints = $classResult->getThrowPoints();
$impurePoints = $classResult->getImpurePoints();
$isAlwaysTerminating = $isAlwaysTerminating || $classResult->isAlwaysTerminating();
}
} elseif ($expr instanceof Node\Scalar) {
$hasYield = false;
$throwPoints = [];
$impurePoints = [];
$isAlwaysTerminating = false;
} elseif ($expr instanceof ConstFetch) {
$hasYield = false;
$throwPoints = [];
$impurePoints = [];
$isAlwaysTerminating = false;
$nodeCallback($expr->name, $scope);
} else {
$hasYield = false;
$throwPoints = [];
$impurePoints = [];
$isAlwaysTerminating = false;
}

return new ExpressionResult(
Expand Down
40 changes: 40 additions & 0 deletions tests/PHPStan/Analyser/ExpressionResultTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,50 @@ public static function dataIsAlwaysTerminating(): array
'sprintf("hello %s", "abc");',
false,
],
[
'isset($x);',
false,
],
[
'$x ? "def" : "abc";',
false,
],
[
'sprintf("hello %s", exit());',
true,
],
[
'(string) exit();',
true,
],
[
'!exit();',
true,
],
[
'eval(exit());',
true,
],
[
'empty(exit());',
true,
],
[
'isset(exit());',
true,
],
[
'$x ? "abc" : exit();',
true,
Comment thread
staabm marked this conversation as resolved.
Resolved
],
[
'$x ? exit() : "abc";',
true,
],
[
'fn() => yield (exit());',
true,
],
];
}

Expand Down
Loading