Improve for loop initial statement scope pollution
#3617
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ondrejmirtes
requested changes
Nov 10, 2024
Added more tests that should cover all the new code and also duplicated them to see how it behaves with the default when With my change reverted, tests fail like this |
|
Thank you! |
|
ah I just noticed a new line too much after the namespace of the test files :P also wanted to add the diff between scope pollution on and off: --- tests/PHPStan/Analyser/nsrt/for-loop.php 2024-11-10 13:55:39.808980414 +0100
+++ tests/PHPStan/Analyser/data/for-loop-no-scope-pollution.php 2024-11-10 13:55:39.685646935 +0100
@@ -1,6 +1,6 @@
<?php declare(strict_types=1);
-namespace ForLoop;
+namespace ForLoopNoScopePollution;
use PHPStan\TrinaryLogic;
use function PHPStan\Testing\assertNativeType;
@@ -22,7 +22,7 @@
assertType('int<10, max>', $i);
assertNativeType('int<10, max>', $i);
- assertVariableCertainty(TrinaryLogic::createYes(), $i);
+ assertVariableCertainty(TrinaryLogic::createMaybe(), $i);
assertType('int<min, 10>', $j);
assertNativeType('int<min, 10>', $j);
@@ -53,7 +53,7 @@
assertType('int<0, max>', $i);
assertNativeType('int<0, max>', $i);
- assertVariableCertainty(TrinaryLogic::createYes(), $i);
+ assertVariableCertainty(TrinaryLogic::createMaybe(), $i);
assertType('int<min, 10>', $j);
assertNativeType('int<min, 10>', $j);
@@ -82,12 +82,12 @@
$c = rand(0, 1);
}
- assertType('0', $i);
- assertNativeType('0', $i);
- assertVariableCertainty(TrinaryLogic::createYes(), $i);
+ assertType('*ERROR*', $i);
+ assertNativeType('*ERROR*', $i);
+ assertVariableCertainty(TrinaryLogic::createNo(), $i);
- assertType('10', $j);
- assertNativeType('10', $j);
+ assertType('0', $j);
+ assertNativeType('0', $j);
assertVariableCertainty(TrinaryLogic::createYes(), $j);
assertType('int', $a);
|
|
fyi, got the equivalent improvement for |
|
Keep them coming :) |
|
|
||
| assertType('int<0, 1>', $c); | ||
| assertNativeType('int', $c); | ||
| assertVariableCertainty(TrinaryLogic::createYes(), $c); |
There was a problem hiding this comment.
Yeah, this should be maybe :(
|
Reverted for now. |
|
As reverted, can you reopen phpstan/phpstan#9550? |
|
I realized the issue - right now this behaviour is connected to polluteScopeWithLoopInitialAssignments but it might be a better idea to have its own |
|
Sounds like a good idea I guess. So you mean this could be brought back but only under that new flag? And if so - what about foreach? The behaviour change of phpstan/phpstan#8270 confused me too a bit hmm. |
|
Yeah, I guess so, let's try it :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added a similar method on
Mutatingscopeasforeachhandling has it already. This should keep expressions in the scope we can safely keep while still removing the initial expressions whenpolluteScopeWithLoopInitialAssignmentsis set tofalse.Before, it was merging
$finalScopewith$scopeto get rid of initial expressions which has the downside of also getting rid of expressions that only exist in$finalScopebecause e.g. the loop is iterating at least once and assigns a variable inside.Closes phpstan/phpstan#9550
Btw a similar thing seems to be going on with foreach and
polluteScopeWithAlwaysIterableForeachset tofalse.