[DeadCode] Add early return check to RemoveUnusedNonEmptyArrayBeforeForeachRector #3611
Conversation
TomasVotruba
force-pushed
the
tv-extend-empty-return
branch
2 times, most recently
from
a83b74a
to
07f5070
Compare
rules/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector.php
Outdated
Show resolved
Hide resolved
TomasVotruba
force-pushed
the
tv-extend-empty-return
branch
from
0ffcb0e
to
f19c78f
Compare
...f_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/if_check_with_empty_multiple.php.inc
Outdated
Show resolved
Hide resolved
|
All checks have passed 🎉 @TomasVotruba I think it is ready. |
samsonasik
force-pushed
the
tv-extend-empty-return
branch
from
6da8fc1
to
de215f8
Compare
|
Rebased, I will add more fixture for skip multi ifs and fixtures. |
|
I added fixture for skip multi ifs and foreach empty check 833fd62 |
|
Fixed 🎉 by return early when there is next stmt after foreach. |
|
All checks have passed 🎉 @TomasVotruba I think it is ready. |
There was a problem hiding this comment.
@TomasVotruba I am thinking that the following code should be skipped as well:
if (empty($items)) {
return;
}
foreach ($items as $item) {
return 1;
}above, when empty, it doesn't return anything, when not empty, it return 1, which make it sometime return sometime not.
|
I added failing fixture for skip has return with value inside foreach af6f370 |
|
Oh, I see, when empty, it pass to after foreach, which no other stmt, so that seems ok 👍 |
|
I updated the fixture 0db3625 |
|
All checks have passed 🎉 @TomasVotruba I think it is ready. |
|
|
||
| private function refactorStmtsAware(StmtsAwareInterface $stmtsAware): ?StmtsAwareInterface | ||
| { | ||
| foreach ((array) $stmtsAware->stmts as $key => $stmt) { |
There was a problem hiding this comment.
It seems it can be optimized by only get $stmtsAware->stms[$lastKey - 1] and $stmtsAware->stms[$lastKey], that make loop is not needed
There was a problem hiding this comment.
optimized, also allow multi if -> foreach which actually ok, with only remove last if bd8b9f9
There was a problem hiding this comment.
New phpunit https://github.com/sebastianbergmann/phpunit/releases/tag/10.1.0 seems cause error on paratest, the handling is still on progress paratestphp/paratest#752
I will set phpunit to 10.0.19 temporary.
There was a problem hiding this comment.
I temporary set phpunit to 10.0.19 e23e2e3 while wait for paratest handling for it.
There was a problem hiding this comment.
paratest 7.1.3 just released https://github.com/paratestphp/paratest/releases/tag/v7.1.3, let's try 6091029
| if (empty($items)) { | ||
| return; | ||
| } | ||
|
|
||
| foreach ($items as $item) { | ||
| echo $item; | ||
| } | ||
|
|
||
| foreach ($items2 as $item2) { | ||
| echo $item2; | ||
| } |
There was a problem hiding this comment.
this only last if before last foreach that removed 👍
|
All checks have passed 🎉 @TomasVotruba I think it is ready. |
|
Thank you, this looks very good 😊 |
…oreachRector (#3611) * [DeadCode] Add early return check to RemoveUnusedNonEmptyArrayBeforeForeachRector * skip edge case * Add fixture multi if -> foreach * Add the patch by loop jump to key recursive * [ci-review] Rector Rectify * clean up skip config * Revert clean up skip config This reverts commit 22c3df8. * Revert [ci-review] Rector Rectify This reverts commit 10a8bfa. * Revert Add the patch by loop jump to key recursive This reverts commit 9375b38. * Revert Add fixture multi if -> foreach This reverts commit ceb6b33. * add failing fixture throw inside empty * Add patch ensure if only return with no expr * add fixture for multi ifs and fixtures * Fixed 🎉 * rename fixture * skip has return with value inside foreach * rename fixture * optimize, allow multi if check, but last only * temporary pin phpunit to 10.0.19 due to make error on paratest * Fix phpstan * use paratest 7.1.3 --------- Co-authored-by: Abdul Malik Ikhsan <samsonasik@gmail.com> Co-authored-by: GitHub Action <actions@github.com>
No description provided.