-
Notifications
You must be signed in to change notification settings - Fork 568
Fix phpstan/phpstan#14446: Incorrect type-narrowing when using strict rules #5435
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
Changes from all commits
7cca523
8163acb
53b917a
62b06cc
5c825e2
1196a5e
19038be
5c33ca2
d8f21c4
f6abe3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| <?php declare(strict_types = 1); | ||
|
|
||
| namespace PHPStan\Analyser; | ||
|
|
||
| use PHPStan\Testing\TypeInferenceTestCase; | ||
| use PHPUnit\Framework\Attributes\DataProvider; | ||
|
|
||
| class Bug14446Test extends TypeInferenceTestCase | ||
| { | ||
|
|
||
| public static function dataFileAsserts(): iterable | ||
| { | ||
| yield from self::gatherAssertTypes(__DIR__ . '/data/bug-14446.php'); | ||
| } | ||
|
|
||
| /** | ||
| * @param mixed ...$args | ||
| */ | ||
| #[DataProvider('dataFileAsserts')] | ||
| public function testFileAsserts( | ||
| string $assertType, | ||
| string $file, | ||
| ...$args, | ||
| ): void | ||
| { | ||
| $this->assertFileAsserts($assertType, $file, ...$args); | ||
| } | ||
|
|
||
| public static function getAdditionalConfigFiles(): array | ||
| { | ||
| return [ | ||
| __DIR__ . '/bug-14446.neon', | ||
| ]; | ||
| } | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| parameters: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this test be in the strict-rule repo instead?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. The change removes the Both |
||
| polluteScopeWithAlwaysIterableForeach: false | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| <?php declare(strict_types = 1); | ||
|
|
||
| namespace Bug14446; | ||
|
|
||
| use function PHPStan\Testing\assertType; | ||
|
|
||
| function test(bool $initial): void { | ||
| $current = $initial; | ||
|
|
||
| while (true) { | ||
| assertType('bool', $initial); | ||
| if (!$current) { | ||
| assertType('bool', $initial); | ||
| break; | ||
| } | ||
|
|
||
| $items = [1]; | ||
| foreach ($items as $item) { | ||
| $current = false; | ||
| } | ||
| } | ||
|
|
||
| assertType('bool', $initial); | ||
| var_dump($initial === true); | ||
| } | ||
|
|
||
| function testMaybeIterable(bool $initial): void { | ||
| $current = $initial; | ||
|
|
||
| while (true) { | ||
| assertType('bool', $initial); | ||
| if (!$current) { | ||
| assertType('bool', $initial); | ||
| break; | ||
| } | ||
|
|
||
| $items = rand() > 0 ? [1] : []; | ||
| foreach ($items as $item) { | ||
| $current = false; | ||
| } | ||
| } | ||
|
|
||
| assertType('bool', $initial); | ||
| var_dump($initial === true); | ||
| } | ||
|
|
||
| function testFinally(bool $initial): void { | ||
| $current = $initial; | ||
| try { | ||
| // nothing | ||
| } finally { | ||
| $current = false; | ||
| } | ||
| assertType('false', $current); | ||
| assertType('bool', $initial); | ||
| if (!$current) { | ||
| assertType('bool', $initial); | ||
| } | ||
| } | ||
|
|
||
| function testFinallyWithCatch(bool $initial): void { | ||
| $current = $initial; | ||
| try { | ||
| doSomething(); | ||
| } catch (\Exception $e) { | ||
| // nothing | ||
| } finally { | ||
| $current = false; | ||
| } | ||
| assertType('false', $current); | ||
| if (!$current) { | ||
| assertType('bool', $initial); | ||
| } | ||
| assertType('bool', $initial); | ||
| } | ||
|
|
||
| function doSomething(): void {} | ||
|
|
||
| /** | ||
| * @param mixed $value | ||
| */ | ||
| function testForeachKeyOverwrite($value): void { | ||
| if (is_array($value) && $value !== []) { | ||
| $hasOnlyStringKey = true; | ||
| foreach (array_keys($value) as $key) { | ||
| if (is_int($key)) { | ||
| $hasOnlyStringKey = false; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| assertType('bool', $hasOnlyStringKey); | ||
|
|
||
| if ($hasOnlyStringKey) { | ||
| // $key should not be in scope here with polluteScopeWithAlwaysIterableForeach: false | ||
| // Second foreach should not report "Foreach overwrites $key with its key variable" | ||
| foreach ($value as $key => $element) { | ||
| assertType('(int|string)', $key); | ||
| } | ||
| } | ||
| } | ||
| } |
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.
are there even more places?
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.
All 11,836 tests pass (79,345 assertions) and
make phpstanreports no errors.Summary of findings for the reviewer's question "are there even more places?":
No, there are no more places that need the fix. Here's the complete analysis:
The two methods already fixed in this PR are the only ones where stale conditional expressions could cause incorrect type narrowing:
processFinallyScope(line 3735) — already fixedprocessAlwaysIterableForeachScopeWithoutPollute(line 3880) — already fixedThe remaining methods that pass
$this->conditionalExpressionswhen creating a new scope fall into three safe categories:Only modify
$this-related state (3 methods):enterClosureBind,restoreOriginalScopeAfterClosureBind,restoreThis— these only touch$this-prefixed expressions andinClosureBindScopeClasses, so variable alias conditional expressions (like$current => $initial) are unaffected.Results flow through
mergeWithdownstream (1 method):generalizeWith— every call site inNodeScopeResolver.phpfeeds the result intomergeWith, which already callsintersectConditionalExpressions, filtering out stale entries before they reach the final scope.Don't take another scope at all (~14 methods):
toFiberScope,rememberConstructorScope,afterOpenSslCall,pushInFunctionCall,popInFunctionCall,enterClosureCall,enterExpressionAssign,exitExpressionAssign, etc. — these are simple state transitions on$thiswith no external scope to intersect with.I wasn't able to post this as a comment on the PR (token lacks permission). The answer should be posted manually on #5435.