Fix forgetting static property access after impure call #3950
Conversation
|
This pull request has been marked as ready for review. |
| class HelloWorld | ||
| { | ||
| public static ?HelloWorld $instance = null; | ||
|
|
||
| public function save(): void { | ||
| self::$instance = new HelloWorld(); | ||
|
|
||
| $callback = static function(): void { | ||
| self::$instance = null; | ||
| }; | ||
|
|
||
| $callback(); | ||
|
|
||
| var_dump(self::$instance); | ||
|
|
||
| self::$instance?->save(); | ||
| } | ||
| } | ||
|
|
||
| (new HelloWorld())->save(); |
There was a problem hiding this comment.
there is a similar case where we still fail:
<?php declare(strict_types = 1);
class HelloWorld
{
public static ?HelloWorld $instance = null;
public function save(): void {
HelloWorld::$instance = new HelloWorld();
$callback = static function(): void {
HelloWorld::$instance = null;
};
$callback();
var_dump(HelloWorld::$instance);
HelloWorld::$instance?->save();
}
}
(new HelloWorld())->save();note: instead of self in this case the FQCN is used.
maybe we should normalize FQCN globally in AST to self in such cases, so we don't need to handle this case all over the place?
| @@ -2597,6 +2597,13 @@ static function (): void { | |||
| $throwPoints[] = ThrowPoint::createImplicit($scope, $expr); | |||
| } | |||
|
|
|||
| if ( | |||
| $parametersAcceptor instanceof ClosureType && $parametersAcceptor->getReturnType()->isVoid()->yes() | |||
There was a problem hiding this comment.
to be consistent with MethodCall, StaticCall we could add
$functionReflection !== null && $functionReflection->hasSideEffects()->yes()
here, but this would regress some types:
There were 3 failures:
1) PHPStan\Analyser\LegacyNodeScopeResolverTest::testForeachLoopVariables with data set #19 ('array<string, 1|2|3>', '$this->property', ''afterLoop'')
$this->property at 'afterLoop'
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'array<string, 1|2|3>'
+'array<int>'
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:9696
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:9590
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:9597
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:7092
2) PHPStan\Analyser\LegacyNodeScopeResolverTest::testForeachLoopVariables with data set #17 ('array<string, 1|2|3>', '$this->property', ''begin'')
$this->property at 'begin'
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'array<string, 1|2|3>'
+'array<int>'
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:9696
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:9590
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:9597
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:7092
3) PHPStan\Analyser\LegacyNodeScopeResolverTest::testForeachLoopVariables with data set #18 ('non-empty-array<string, 1|2|3>', '$this->property', ''end'')
$this->property at 'end'
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'non-empty-array<string, 1|2|3>'
+'non-empty-array<int>'
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:9696
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:9590
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:9597
/Users/staabm/workspace/phpstan-src/tests/PHPStan/Analyser/LegacyNodeScopeResolverTest.php:7092
FAILURES!
Tests: 12743, Assertions: 85340, Failures: 3, Skipped: 139.
wdyt?
There was a problem hiding this comment.
For closures we have already analysed the body, there's ClosureType::getInvalidateExpressions() so we know exactly what to invalidate.
There was a problem hiding this comment.
for the example in tests/PHPStan/Rules/Methods/data/bug-8523b.php ClosureType::getInvalidateExpressions() returns an empty array.
when searching for new InvalidateExprNode in the whole codebase it looks like only MethodCall and Arg->value is considered a invalidate-expression. Should I add PropertyFetch and StaticPropertyFetch and StaticCall?
There was a problem hiding this comment.
Oh, I just mixed invalidating expressions with impure points. Please check ClosureType::getImpurePoints() to see whether closure is pure or not.
| @@ -2597,6 +2597,13 @@ static function (): void { | |||
| $throwPoints[] = ThrowPoint::createImplicit($scope, $expr); | |||
| } | |||
|
|
|||
| if ( | |||
| $parametersAcceptor instanceof ClosureType && $parametersAcceptor->getReturnType()->isVoid()->yes() | |||
There was a problem hiding this comment.
For closures we have already analysed the body, there's ClosureType::getInvalidateExpressions() so we know exactly what to invalidate.
| if ( | ||
| $exprStringToInvalidate === '$this' | ||
| && $node instanceof Name | ||
| && in_array($node->toLowerString(), ['self', 'static', 'parent'], true) |
There was a problem hiding this comment.
I just realized the self here should be improved. We should use Scope::resolveName or resolveTypeByName and check that the class name matches the current scope class name. Because self is just an alias to the current class name, so whether we're using self:: or Foo:: shouldn't matter.
| @@ -2597,6 +2597,13 @@ static function (): void { | |||
| $throwPoints[] = ThrowPoint::createImplicit($scope, $expr); | |||
| } | |||
|
|
|||
| if ( | |||
| $parametersAcceptor instanceof ClosureType && $parametersAcceptor->getReturnType()->isVoid()->yes() | |||
There was a problem hiding this comment.
Oh, I just mixed invalidating expressions with impure points. Please check ClosureType::getImpurePoints() to see whether closure is pure or not.
closes phpstan/phpstan#11019
closes phpstan/phpstan#3747
closes phpstan/phpstan#8523