phpstan-src icon indicating copy to clipboard operation
phpstan-src copied to clipboard

Support `for` endless loops

Open herndlm opened this issue 1 year ago • 4 comments

Closes https://github.com/phpstan/phpstan/issues/6807 Closes https://github.com/phpstan/phpstan/issues/8463 Closes https://github.com/phpstan/phpstan/issues/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 avatar Oct 14 '24 16:10 herndlm

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 avatar Oct 14 '24 17:10 herndlm

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 avatar Oct 14 '24 19:10 herndlm

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

herndlm avatar Oct 15 '24 08:10 herndlm

realized that I can make this better..

To do so I'd need https://github.com/phpstan/phpstan-src/pull/3578 first though :)

herndlm avatar Oct 18 '24 20:10 herndlm

This pull request has been marked as ready for review.

phpstan-bot avatar Nov 06 '24 19:11 phpstan-bot

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

herndlm avatar Nov 06 '24 19:11 herndlm

Thank you.

ondrejmirtes avatar Nov 06 '24 19:11 ondrejmirtes