From 16d0d80f33f4d2e70d29f6ac821047398a888b7e Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Mon, 14 Oct 2024 18:12:39 +0200 Subject: [PATCH 1/7] Support `for` endless loops --- src/Analyser/NodeScopeResolver.php | 13 +++++++--- .../PHPStan/Analyser/StatementResultTest.php | 12 +++++++++ ...rictComparisonOfDifferentTypesRuleTest.php | 10 +++---- .../Comparison/data/strict-comparison.php | 14 +++++----- .../Rules/Missing/MissingReturnRuleTest.php | 6 +++++ tests/PHPStan/Rules/Missing/data/bug-8463.php | 26 +++++++++++++++++++ 6 files changed, 65 insertions(+), 16 deletions(-) create mode 100644 tests/PHPStan/Rules/Missing/data/bug-8463.php diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index dda335f566..cbbdc40880 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -1194,10 +1194,8 @@ private function processStmtNode( if ($alwaysIterates) { $isAlwaysTerminating = count($finalScopeResult->getExitPointsByType(Break_::class)) === 0; - } elseif ($isIterableAtLeastOnce) { - $isAlwaysTerminating = $finalScopeResult->isAlwaysTerminating(); } else { - $isAlwaysTerminating = false; + $isAlwaysTerminating = $isIterableAtLeastOnce && $finalScopeResult->isAlwaysTerminating(); } $condScope = $condResult->getFalseyScope(); if (!$isIterableAtLeastOnce) { @@ -1314,6 +1312,7 @@ private function processStmtNode( } $bodyScope = $initScope; + $alwaysIterates = $stmt->cond === [] && $context->isTopLevel(); $isIterableAtLeastOnce = TrinaryLogic::createYes(); $lastCondExpr = $stmt->cond[count($stmt->cond) - 1] ?? null; foreach ($stmt->cond as $condExpr) { @@ -1411,10 +1410,16 @@ private function processStmtNode( } } + if ($alwaysIterates) { + $isAlwaysTerminating = count($finalScopeResult->getExitPointsByType(Break_::class)) === 0; + } else { + $isAlwaysTerminating = false; // $finalScopeResult->isAlwaysTerminating() && $isAlwaysIterable + } + return new StatementResult( $finalScope, $finalScopeResult->hasYield() || $hasYield, - false/* $finalScopeResult->isAlwaysTerminating() && $isAlwaysIterable*/, + $isAlwaysTerminating, $finalScopeResult->getExitPointsForOuterLoop(), array_merge($throwPoints, $finalScopeResult->getThrowPoints()), array_merge($impurePoints, $finalScopeResult->getImpurePoints()), diff --git a/tests/PHPStan/Analyser/StatementResultTest.php b/tests/PHPStan/Analyser/StatementResultTest.php index fb3a9dd5b1..80742551e5 100644 --- a/tests/PHPStan/Analyser/StatementResultTest.php +++ b/tests/PHPStan/Analyser/StatementResultTest.php @@ -173,6 +173,18 @@ public function dataIsAlwaysTerminating(): array 'while (true) { break; }', false, ], + [ + 'for (;;) { }', + true, + ], + [ + 'for (;;) { return; }', + true, + ], + [ + 'for (;;) { break; }', + false, + ], [ 'do { } while (doFoo());', false, diff --git a/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php b/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php index 01157d82e1..48de5a95b4 100644 --- a/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesRuleTest.php @@ -109,12 +109,12 @@ public function testStrictComparison(): void 140, ], [ - 'Strict comparison using !== between StrictComparison\Foo|null and 1 will always evaluate to true.', - 154, + 'Strict comparison using === between non-empty-array and null will always evaluate to false.', + 150, ], [ - 'Strict comparison using === between non-empty-array and null will always evaluate to false.', - 164, + 'Strict comparison using !== between StrictComparison\Foo|null and 1 will always evaluate to true.', + 161, ], [ 'Strict comparison using !== between StrictComparison\Node|null and false will always evaluate to true.', @@ -352,7 +352,7 @@ public function testStrictComparisonWithoutAlwaysTrue(): void ], [ 'Strict comparison using === between non-empty-array and null will always evaluate to false.', - 164, + 150, ], [ 'Strict comparison using === between 1 and 2 will always evaluate to false.', diff --git a/tests/PHPStan/Rules/Comparison/data/strict-comparison.php b/tests/PHPStan/Rules/Comparison/data/strict-comparison.php index 70882c4ca4..f98e7e11aa 100644 --- a/tests/PHPStan/Rules/Comparison/data/strict-comparison.php +++ b/tests/PHPStan/Rules/Comparison/data/strict-comparison.php @@ -146,6 +146,13 @@ public function whileWithTypeChange() public function forWithTypeChange() { + for (; $val = $this->returnArray();) { + if ($val === null) { + + } + $val = null; + } + $foo = null; for (;;) { if ($foo !== null) { @@ -159,13 +166,6 @@ public function forWithTypeChange() $foo = new self(); } } - - for (; $val = $this->returnArray();) { - if ($val === null) { - - } - $val = null; - } } private function returnArray(): array diff --git a/tests/PHPStan/Rules/Missing/MissingReturnRuleTest.php b/tests/PHPStan/Rules/Missing/MissingReturnRuleTest.php index ad9fc94be5..f93486e9e8 100644 --- a/tests/PHPStan/Rules/Missing/MissingReturnRuleTest.php +++ b/tests/PHPStan/Rules/Missing/MissingReturnRuleTest.php @@ -325,4 +325,10 @@ public function testBug9309(): void $this->analyse([__DIR__ . '/data/bug-9309.php'], []); } + public function testBug8463(): void + { + $this->checkExplicitMixedMissingReturn = true; + $this->analyse([__DIR__ . '/data/bug-8463.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Missing/data/bug-8463.php b/tests/PHPStan/Rules/Missing/data/bug-8463.php new file mode 100644 index 0000000000..c27592782f --- /dev/null +++ b/tests/PHPStan/Rules/Missing/data/bug-8463.php @@ -0,0 +1,26 @@ + Date: Mon, 14 Oct 2024 20:59:29 +0200 Subject: [PATCH 2/7] Revert unnecessary change --- src/Analyser/NodeScopeResolver.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index cbbdc40880..92623f00a2 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -1194,8 +1194,10 @@ private function processStmtNode( if ($alwaysIterates) { $isAlwaysTerminating = count($finalScopeResult->getExitPointsByType(Break_::class)) === 0; + } elseif ($isIterableAtLeastOnce) { + $isAlwaysTerminating = $finalScopeResult->isAlwaysTerminating(); } else { - $isAlwaysTerminating = $isIterableAtLeastOnce && $finalScopeResult->isAlwaysTerminating(); + $isAlwaysTerminating = false; } $condScope = $condResult->getFalseyScope(); if (!$isIterableAtLeastOnce) { From ad2f7d1a4f9375e21f3a6e710794e0c50191e329 Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Tue, 15 Oct 2024 09:39:27 +0200 Subject: [PATCH 3/7] Use isAlwaysTerminating of finalScopeResult and update test --- src/Analyser/NodeScopeResolver.php | 4 +++- tests/PHPStan/Analyser/StatementResultTest.php | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 92623f00a2..199c84f9b0 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -1414,8 +1414,10 @@ private function processStmtNode( if ($alwaysIterates) { $isAlwaysTerminating = count($finalScopeResult->getExitPointsByType(Break_::class)) === 0; + } elseif ($isIterableAtLeastOnce->yes()) { + $isAlwaysTerminating = $finalScopeResult->isAlwaysTerminating(); } else { - $isAlwaysTerminating = false; // $finalScopeResult->isAlwaysTerminating() && $isAlwaysIterable + $isAlwaysTerminating = false; } return new StatementResult( diff --git a/tests/PHPStan/Analyser/StatementResultTest.php b/tests/PHPStan/Analyser/StatementResultTest.php index 80742551e5..98437ea18c 100644 --- a/tests/PHPStan/Analyser/StatementResultTest.php +++ b/tests/PHPStan/Analyser/StatementResultTest.php @@ -243,7 +243,7 @@ public function dataIsAlwaysTerminating(): array ], [ 'for ($i = 0; $i < 10; $i++) { return; }', - false, // will be true with range types + true, ], [ 'for ($i = 0; $i < 0; $i++) { return; }', From 7fa723984b02eed41ef416ae7fbe1be4aeb53a64 Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Tue, 15 Oct 2024 10:02:12 +0200 Subject: [PATCH 4/7] Update e2e baseline --- tests/e2e/baseline.neon | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/e2e/baseline.neon b/tests/e2e/baseline.neon index aad958b3b5..71c60e0f32 100644 --- a/tests/e2e/baseline.neon +++ b/tests/e2e/baseline.neon @@ -155,6 +155,11 @@ parameters: count: 1 path: PHP-Parser/lib/PhpParser/ParserAbstract.php + - + message: "#^Unreachable statement \\- code above always terminates\\.$#" + count: 1 + path: PHP-Parser/lib/PhpParser/ParserAbstract.php + - message: "#^Variable \\$action might not be defined\\.$#" count: 1 From dbbb3f88461ba494fc2c09bc551e9c37bb4f61a8 Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Tue, 15 Oct 2024 10:14:56 +0200 Subject: [PATCH 5/7] Add 2 more regression tests --- .../Rules/Missing/MissingReturnRuleTest.php | 12 ++++++++++++ tests/PHPStan/Rules/Missing/data/bug-6807.php | 16 ++++++++++++++++ tests/PHPStan/Rules/Missing/data/bug-9374.php | 8 ++++++++ 3 files changed, 36 insertions(+) create mode 100644 tests/PHPStan/Rules/Missing/data/bug-6807.php create mode 100644 tests/PHPStan/Rules/Missing/data/bug-9374.php diff --git a/tests/PHPStan/Rules/Missing/MissingReturnRuleTest.php b/tests/PHPStan/Rules/Missing/MissingReturnRuleTest.php index f93486e9e8..f99d9352d9 100644 --- a/tests/PHPStan/Rules/Missing/MissingReturnRuleTest.php +++ b/tests/PHPStan/Rules/Missing/MissingReturnRuleTest.php @@ -325,10 +325,22 @@ public function testBug9309(): void $this->analyse([__DIR__ . '/data/bug-9309.php'], []); } + public function testBug6807(): void + { + $this->checkExplicitMixedMissingReturn = true; + $this->analyse([__DIR__ . '/data/bug-6807.php'], []); + } + public function testBug8463(): void { $this->checkExplicitMixedMissingReturn = true; $this->analyse([__DIR__ . '/data/bug-8463.php'], []); } + public function testBug9374(): void + { + $this->checkExplicitMixedMissingReturn = true; + $this->analyse([__DIR__ . '/data/bug-9374.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Missing/data/bug-6807.php b/tests/PHPStan/Rules/Missing/data/bug-6807.php new file mode 100644 index 0000000000..d7ffdde9dd --- /dev/null +++ b/tests/PHPStan/Rules/Missing/data/bug-6807.php @@ -0,0 +1,16 @@ + 5) + throw new Exception(); + + if (rand() == 1) + return 5; + } +} diff --git a/tests/PHPStan/Rules/Missing/data/bug-9374.php b/tests/PHPStan/Rules/Missing/data/bug-9374.php new file mode 100644 index 0000000000..275b7df356 --- /dev/null +++ b/tests/PHPStan/Rules/Missing/data/bug-9374.php @@ -0,0 +1,8 @@ + Date: Tue, 15 Oct 2024 15:04:32 +0200 Subject: [PATCH 6/7] Add moar tests --- .../PHPStan/Analyser/StatementResultTest.php | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/tests/PHPStan/Analyser/StatementResultTest.php b/tests/PHPStan/Analyser/StatementResultTest.php index 98437ea18c..f4deba2c09 100644 --- a/tests/PHPStan/Analyser/StatementResultTest.php +++ b/tests/PHPStan/Analyser/StatementResultTest.php @@ -173,6 +173,50 @@ public function dataIsAlwaysTerminating(): array 'while (true) { break; }', false, ], + [ + 'while (true) { exit; }', + true, + ], + [ + 'while (true) { while (true) { } }', + true, + ], + [ + 'while (true) { while (true) { return; } }', + true, + ], + [ + 'while (true) { while (true) { break; } }', + true, + ], + [ + 'while (true) { while (true) { exit; } }', + true, + ], + [ + 'while (true) { while (true) { break 2; } }', + false, + ], + [ + 'while (true) { while ($x) { } }', + true, + ], + [ + 'while (true) { while ($x) { return; } }', + true, + ], + [ + 'while (true) { while ($x) { break; } }', + true, + ], + [ + 'while (true) { while ($x) { exit; } }', + true, + ], + [ + 'while (true) { while ($x) { break 2; } }', + false, + ], [ 'for (;;) { }', true, @@ -185,6 +229,50 @@ public function dataIsAlwaysTerminating(): array 'for (;;) { break; }', false, ], + [ + 'for (;;) { exit; }', + true, + ], + [ + 'for (;;) { for (;;) { } }', + true, + ], + [ + 'for (;;) { for (;;) { return; } }', + true, + ], + [ + 'for (;;) { for (;;) { break; } }', + true, + ], + [ + 'for (;;) { for (;;) { exit; } }', + true, + ], + [ + 'for (;;) { for (;;) { break 2; } }', + false, + ], + [ + 'for (;;) { for ($i = 0; $i< 5; $i++) { } }', + true, + ], + [ + 'for (;;) { for ($i = 0; $i< 5; $i++) { return; } }', + true, + ], + [ + 'for (;;) { for ($i = 0; $i< 5; $i++) { break; } }', + true, + ], + [ + 'for (;;) { for ($i = 0; $i< 5; $i++) { exit; } }', + true, + ], + [ + 'for (;;) { for ($i = 0; $i< 5; $i++) { break 2; } }', + false, + ], [ 'do { } while (doFoo());', false, From 6779b9820d5f47f814b5ee2ecf4a8e6d39770f9b Mon Sep 17 00:00:00 2001 From: Martin Herndl Date: Wed, 6 Nov 2024 20:07:32 +0100 Subject: [PATCH 7/7] Try to support all the for end loop scenarios :) --- src/Analyser/NodeScopeResolver.php | 6 +++-- .../PHPStan/Analyser/StatementResultTest.php | 24 +++++++++++++++++++ ...fined-variables-anonymous-function-use.php | 2 +- .../Variables/data/defined-variables.php | 6 ++--- 4 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 199c84f9b0..1a6a069d28 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -1314,7 +1314,6 @@ private function processStmtNode( } $bodyScope = $initScope; - $alwaysIterates = $stmt->cond === [] && $context->isTopLevel(); $isIterableAtLeastOnce = TrinaryLogic::createYes(); $lastCondExpr = $stmt->cond[count($stmt->cond) - 1] ?? null; foreach ($stmt->cond as $condExpr) { @@ -1385,7 +1384,10 @@ private function processStmtNode( $loopScope = $this->processExprNode($stmt, $loopExpr, $loopScope, $nodeCallback, ExpressionContext::createTopLevel())->getScope(); } $finalScope = $finalScope->generalizeWith($loopScope); + + $alwaysIterates = TrinaryLogic::createFromBoolean($context->isTopLevel()); if ($lastCondExpr !== null) { + $alwaysIterates = $alwaysIterates->and($finalScope->getType($lastCondExpr)->toBoolean()->isTrue()); $finalScope = $finalScope->filterByFalseyValue($lastCondExpr); } @@ -1412,7 +1414,7 @@ private function processStmtNode( } } - if ($alwaysIterates) { + if ($alwaysIterates->yes()) { $isAlwaysTerminating = count($finalScopeResult->getExitPointsByType(Break_::class)) === 0; } elseif ($isIterableAtLeastOnce->yes()) { $isAlwaysTerminating = $finalScopeResult->isAlwaysTerminating(); diff --git a/tests/PHPStan/Analyser/StatementResultTest.php b/tests/PHPStan/Analyser/StatementResultTest.php index f4deba2c09..b86de8617e 100644 --- a/tests/PHPStan/Analyser/StatementResultTest.php +++ b/tests/PHPStan/Analyser/StatementResultTest.php @@ -273,6 +273,30 @@ public function dataIsAlwaysTerminating(): array 'for (;;) { for ($i = 0; $i< 5; $i++) { break 2; } }', false, ], + [ + 'for ($i = 0; $i < 5;) { }', + true, + ], + [ + 'for ($i = 0; $i < 5; $i--) { }', + true, + ], + [ + 'for (; 0, 1;) { }', + true, + ], + [ + 'for (; 1, 0;) { }', + false, + ], + [ + 'for (; "", "a";) { }', + true, + ], + [ + 'for (; "a", "";) { }', + false, + ], [ 'do { } while (doFoo());', false, diff --git a/tests/PHPStan/Rules/Variables/data/defined-variables-anonymous-function-use.php b/tests/PHPStan/Rules/Variables/data/defined-variables-anonymous-function-use.php index eb2599f89e..e02deb647a 100644 --- a/tests/PHPStan/Rules/Variables/data/defined-variables-anonymous-function-use.php +++ b/tests/PHPStan/Rules/Variables/data/defined-variables-anonymous-function-use.php @@ -14,7 +14,7 @@ function () use (&$errorHandler) { $onlyInIf = 1; } -for ($forI = 0; $forI < 10, $anotherVariableFromForCond = 1; $forI++, $forJ = $forI) { +for ($forI = 0; $anotherVariableFromForCond = 1, $forI < 10; $forI++, $forJ = $forI) { } diff --git a/tests/PHPStan/Rules/Variables/data/defined-variables.php b/tests/PHPStan/Rules/Variables/data/defined-variables.php index e0f62a0e4c..4b2ec40f04 100644 --- a/tests/PHPStan/Rules/Variables/data/defined-variables.php +++ b/tests/PHPStan/Rules/Variables/data/defined-variables.php @@ -243,7 +243,7 @@ function () { } - for ($forI = 0; $forI < 10, $forK = 5; $forI++, $forK++, $forJ = $forI) { + for ($forI = 0; $forK = 5, $forI < 10; $forI++, $forK++, $forJ = $forI) { echo $forI; } @@ -322,7 +322,7 @@ function () { include($fileB='includeB.php'); echo $fileB; - for ($forLoopVariableInit = 0; $forLoopVariableInit < 5; $forLoopVariableInit = $forLoopVariable, $anotherForLoopVariable = 1) { + for ($forLoopVariableInit = 0; $forLoopVariableInit < 5 && rand(0, 1); $forLoopVariableInit = $forLoopVariable, $anotherForLoopVariable = 1) { $forLoopVariable = 2; } echo $anotherForLoopVariable; @@ -357,7 +357,7 @@ function () { } - for (; $forVariableUsedAndThenDefined && $forVariableUsedAndThenDefined = 1;) { + for (; $forVariableUsedAndThenDefined && $forVariableUsedAndThenDefined = 1 && rand(0, 1);) { }