Skip to content

Fix condition of fall-through case not used for exhaustive checks #3900

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.

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
merged 1 commit into from
Mar 23, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 6 additions & 2 deletions src/Analyser/NodeScopeResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -1449,9 +1449,11 @@ private function processStmtNode(
$exitPointsForOuterLoop = [];
$throwPoints = $condResult->getThrowPoints();
$impurePoints = $condResult->getImpurePoints();
$fullCondExpr = null;
foreach ($stmt->cases as $caseNode) {
if ($caseNode->cond !== null) {
$condExpr = new BinaryOp\Equal($stmt->cond, $caseNode->cond);
$fullCondExpr = $fullCondExpr === null ? $condExpr : new BooleanOr($fullCondExpr, $condExpr);
$caseResult = $this->processExprNode($stmt, $caseNode->cond, $scopeForBranches, $nodeCallback, ExpressionContext::createDeep());
$scopeForBranches = $caseResult->getScope();
$hasYield = $hasYield || $caseResult->hasYield();
Expand All @@ -1460,6 +1462,7 @@ private function processStmtNode(
$branchScope = $caseResult->getTruthyScope()->filterByTruthyValue($condExpr);
} else {
$hasDefaultCase = true;
$fullCondExpr = null;
$branchScope = $scopeForBranches;
}

Expand All @@ -1481,8 +1484,9 @@ private function processStmtNode(
if ($branchScopeResult->isAlwaysTerminating()) {
$alwaysTerminating = $alwaysTerminating && $branchFinalScopeResult->isAlwaysTerminating();
$prevScope = null;
if (isset($condExpr)) {
$scopeForBranches = $scopeForBranches->filterByFalseyValue($condExpr);
if (isset($fullCondExpr)) {
$scopeForBranches = $scopeForBranches->filterByFalseyValue($fullCondExpr);
$fullCondExpr = null;
}
if (!$branchFinalScopeResult->isAlwaysTerminating()) {
$finalScope = $branchScope->mergeWith($finalScope);
Expand Down
30 changes: 30 additions & 0 deletions tests/PHPStan/Analyser/nsrt/bug-11064.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php declare(strict_types = 1);

namespace Bug11064;

use function PHPStan\Testing\assertType;

interface IClass {}
class ClassA implements IClass {}
class ClassB implements IClass {}

/**
* @param null|'nil'|IClass $val
*/
function test($val): string {
switch (true) {
case $val === null:
case $val === 'nil':
assertType("'nil'|null", $val);
return 'null';

case $val instanceof ClassA:
assertType('Bug11064\\ClassA', $val);
return 'class a';

default:
assertType('Bug11064\\IClass~Bug11064\\ClassA', $val);
throw new RuntimeException('unsupported class: ' . get_class($val));
}
}

21 changes: 21 additions & 0 deletions tests/PHPStan/Rules/Missing/MissingReturnRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -343,4 +343,25 @@ public function testBug9374(): void
$this->analyse([__DIR__ . '/data/bug-9374.php'], []);
}

public function testBug3488Two(): void
{
$this->checkExplicitMixedMissingReturn = true;
$this->analyse([__DIR__ . '/data/bug-3488-2.php'], [
[
'Method Bug3488\C::invalidCase() should return int but return statement is missing.',
30,
],
]);
}

public function testBug12722(): void
{
if (PHP_VERSION_ID < 80100) {
$this->markTestSkipped('Test requires PHP 8.1.');
}

$this->checkExplicitMixedMissingReturn = true;
$this->analyse([__DIR__ . '/data/bug-12722.php'], []);
}

}
32 changes: 32 additions & 0 deletions tests/PHPStan/Rules/Missing/data/bug-12722.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php declare(strict_types = 1); // lint >= 8.1

namespace Bug12722;

enum states {
case state1;
case statealmost1;
case state3;
}

class HelloWorld
{
public function intentional_fallthrough(states $state): int
{
switch($state) {

case states::state1: //intentional fall-trough this case...
case states::statealmost1: return 1;
case states::state3: return 3;
}
}

public function no_fallthrough(states $state): int
{
switch($state) {

case states::state1: return 1;
case states::statealmost1: return 1;
case states::state3: return 3;
}
}
}
52 changes: 52 additions & 0 deletions tests/PHPStan/Rules/Missing/data/bug-3488-2.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php declare(strict_types = 1);

namespace Bug3488;

interface I {
const THING_A = 'a';
const THING_B = 'b';
const THING_C = 'c';
}

class C
{
/**
* @param I::THING_* $thing
*/
public function allCovered(string $thing): int
{
switch ($thing) { // should not error
case I::THING_A:
case I::THING_B:
case I::THING_C:
return 0;
}
}
/**
* @param I::THING_* $thing
*/
public function invalidCase(string $thing): int
{
switch ($thing) {
case I::THING_A:
case I::THING_B:
return 0;
case 'd':
throw new Exception('The error marker should be on the line above');
}
}
/**
* @param I::THING_* $thing
*/
public function defaultUnnecessary(string $thing): int
{
switch ($thing) {
case I::THING_A:
case I::THING_B:
case I::THING_C:
return 0;
default:
throw new Exception('This should be detected as unreachable');
}
}
}
9 changes: 9 additions & 0 deletions tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1068,4 +1068,13 @@ public function testBug10228(): void
$this->analyse([__DIR__ . '/data/bug-10228.php'], []);
}

public function testBug8719(): void
{
$this->cliArgumentsVariablesRegistered = true;
$this->polluteScopeWithLoopInitialAssignments = true;
$this->checkMaybeUndefinedVariables = true;
$this->polluteScopeWithAlwaysIterableForeach = true;
$this->analyse([__DIR__ . '/data/bug-8719.php'], []);
}

}
4 changes: 3 additions & 1 deletion tests/PHPStan/Rules/Variables/IssetRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -277,10 +277,12 @@ public function testVariableCertaintyInIsset(): void
112,
],
[
'Variable $variableInFirstCase in isset() always exists and is not nullable.',
// could be Variable $variableInFirstCase in isset() always exists and is not nullable.
'Variable $variableInFirstCase in isset() is never defined.',
116,
],
[
// could be Variable $variableInSecondCase in isset() always exists and is not nullable.
'Variable $variableInSecondCase in isset() is never defined.',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this error already was incorrect. The other one is now also broken, because its condition is also used

117,
],
Expand Down
50 changes: 50 additions & 0 deletions tests/PHPStan/Rules/Variables/data/bug-8719.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
<?php declare(strict_types = 1);

namespace Bug8719;

class HelloWorld
{
private const CASE_1 = 1;
private const CASE_2 = 2;
private const CASE_3 = 3;

/**
* @return self::CASE_*
*/
private function getCase(): int
{
return random_int(1,3);
}

public function ok(): string
{
switch($this->getCase()) {
case self::CASE_1:
$foo = 'bar';
break;
case self::CASE_2:
$foo = 'baz';
break;
case self::CASE_3:
$foo = 'barbaz';
break;
}

return $foo;
}

public function not_ok(): string
{
switch($this->getCase()) {
case self::CASE_1:
$foo = 'bar';
break;
case self::CASE_2:
case self::CASE_3:
$foo = 'barbaz';
break;
}

return $foo;
}
}
Loading