-   Notifications  You must be signed in to change notification settings 
- Fork 545
 Support for endless loops #3573 
 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
Conversation
b43e2e3 to 824abe6   Compare   There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not change anything about while. Change only for. After that is merged, you can send the while change as another PR and then we can think about it.
| 
 It shouldn't change anything, just merges the elseif/else , so only refactor. But I might be overlooking something and can revert it again 😊 | 
| adapted. the failing test cases seem to be OK, since in https://github.com/nikic/PHP-Parser/blob/v3.1.5/lib/PhpParser/ParserAbstract.php#L175-L344 there are 2 endless loops and the outer one does not seem to have a break statement. Replacing them with a  | 
| Saw this fixes some more issues, will add regression tests.. | 
f0bd0f9 to 9d19c22   Compare   | realized that I can make this better.. To do so I'd need #3578 first though :) | 
9d19c22 to 61037e4   Compare   61037e4 to 6779b98   Compare   | This pull request has been marked as ready for review. | 
| Thx to the pre-condition PR that was merged, I could make the endless loop detection more generic and better 🎉 Had to adapt a bit of test code in  I like the following one in particular where the conditions were switched and by mistake this resulted in an endless loop basically: -for ($forI = 0; $forI < 10, $anotherVariableFromForCond = 1; $forI++, $forJ = $forI) { +for ($forI = 0; $anotherVariableFromForCond = 1, $forI < 10; $forI++, $forJ = $forI) { | 
| } | ||
| $finalScope = $finalScope->generalizeWith($loopScope); | ||
|  | ||
| $alwaysIterates = TrinaryLogic::createFromBoolean($context->isTopLevel()); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would something fail if you said $alwaysIterates = TrinaryLogic::createYes() here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, apparently not. this comes basically from the other loops which have code like the following
$alwaysIterates = $condBooleanType->isTrue()->yes() && $context->isTopLevel();but tbh, I didn't fully understand what the level here changes or does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, makes sense to be consistent.
This condition asks about whether it's the "analysis iteration", or one of the "let's find out if types stabilize in the loop before we generalize them" that's done for all loops in a do-while like this:
phpstan-src/src/Analyser/NodeScopeResolver.php
Lines 1330 to 1360 in c3bb055
| do { | |
| $prevScope = $bodyScope; | |
| $bodyScope = $bodyScope->mergeWith($initScope); | |
| if ($lastCondExpr !== null) { | |
| $bodyScope = $this->processExprNode($stmt, $lastCondExpr, $bodyScope, static function (): void { | |
| }, ExpressionContext::createDeep())->getTruthyScope(); | |
| } | |
| $bodyScopeResult = $this->processStmtNodes($stmt, $stmt->stmts, $bodyScope, static function (): void { | |
| }, $context->enterDeep())->filterOutLoopExitPoints(); | |
| $bodyScope = $bodyScopeResult->getScope(); | |
| foreach ($bodyScopeResult->getExitPointsByType(Continue_::class) as $continueExitPoint) { | |
| $bodyScope = $bodyScope->mergeWith($continueExitPoint->getScope()); | |
| } | |
| foreach ($stmt->loop as $loopExpr) { | |
| $exprResult = $this->processExprNode($stmt, $loopExpr, $bodyScope, static function (): void { | |
| }, ExpressionContext::createTopLevel()); | |
| $bodyScope = $exprResult->getScope(); | |
| $hasYield = $hasYield || $exprResult->hasYield(); | |
| $throwPoints = array_merge($throwPoints, $exprResult->getThrowPoints()); | |
| $impurePoints = array_merge($impurePoints, $exprResult->getImpurePoints()); | |
| } | |
| if ($bodyScope->equals($prevScope)) { | |
| break; | |
| } | |
| if ($count >= self::GENERALIZE_AFTER_ITERATION) { | |
| $bodyScope = $prevScope->generalizeWith($bodyScope); | |
| } | |
| $count++; | |
| } while ($count < self::LOOP_SCOPE_ITERATIONS); | 
| Thank you. | 
Closes phpstan/phpstan#6807
Closes phpstan/phpstan#8463
Closes phpstan/phpstan#9374
analogue to
while. contains also a slight simplification in the setting of$isAlwaysTerminatingforwhileloops to keep this more in sync.I had to switch the 2 test blocks in
tests/PHPStan/Rules/Comparison/data/strict-comparison.phpbecause the first one would be never exiting endless loop after this change, which would make the second one dead code and not testing anything useful any more.