Skip to content
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

Support for endless loops #3573

Merged
merged 7 commits into from
Nov 6, 2024
Merged

Conversation

herndlm
Copy link
Contributor

@herndlm herndlm commented Oct 14, 2024

Closes phpstan/phpstan#6807
Closes phpstan/phpstan#8463
Closes phpstan/phpstan#9374

analogue to while. contains also a slight simplification in the setting of $isAlwaysTerminating for while loops to keep this more in sync.

I had to switch the 2 test blocks in tests/PHPStan/Rules/Comparison/data/strict-comparison.php because 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.

@herndlm herndlm force-pushed the for-endless-loop branch 2 times, most recently from b43e2e3 to 824abe6 Compare October 14, 2024 16:24
Copy link
Member

@ondrejmirtes ondrejmirtes left a 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.

@herndlm
Copy link
Contributor Author

herndlm commented Oct 14, 2024

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 😊

@herndlm
Copy link
Contributor Author

herndlm commented Oct 14, 2024

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 while (true) makes it fail the same way.

@herndlm herndlm requested a review from ondrejmirtes October 15, 2024 08:03
@herndlm
Copy link
Contributor Author

herndlm commented Oct 15, 2024

Saw this fixes some more issues, will add regression tests..
UPDATE: added, those were basically duplicates

@herndlm
Copy link
Contributor Author

herndlm commented Oct 18, 2024

realized that I can make this better..

To do so I'd need #3578 first though :)

@herndlm herndlm marked this pull request as draft October 18, 2024 20:08
@herndlm herndlm marked this pull request as ready for review November 6, 2024 19:18
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@herndlm
Copy link
Contributor Author

herndlm commented Nov 6, 2024

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 DefinedVariableRuleTest since it correctly figured out now that after those endless loops variables might not be defined.

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) {

@@ -1384,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());
Copy link
Member

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?

Copy link
Contributor Author

@herndlm herndlm Nov 6, 2024

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

Copy link
Member

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:

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);

@ondrejmirtes ondrejmirtes merged commit 3dd5a01 into phpstan:1.12.x Nov 6, 2024
453 checks passed
@ondrejmirtes
Copy link
Member

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants