diff --git a/src/Analyser/ExprHandler/MethodCallHandler.php b/src/Analyser/ExprHandler/MethodCallHandler.php index f13426cdd29..3e1369dc800 100644 --- a/src/Analyser/ExprHandler/MethodCallHandler.php +++ b/src/Analyser/ExprHandler/MethodCallHandler.php @@ -321,7 +321,19 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex $throwPoints[] = $methodThrowPoint; } - if ($methodReflection->getName() === '__construct' || $methodReflection->hasSideEffects()->yes()) { + $hasSideEffects = $methodReflection->hasSideEffects(); + if ( + $methodReflection->getName() === '__construct' + || $hasSideEffects->yes() + // When possibly impure values are not remembered, a possibly impure method + // may also have changed the object's properties, so forget them too. + // Built-in methods are exempt, as they are from invalidating volatile expressions below. + || ( + !$this->rememberPossiblyImpureFunctionValues + && !$hasSideEffects->no() + && !$methodReflection->getDeclaringClass()->isBuiltin() + ) + ) { $nodeScopeResolver->callNodeCallback($nodeCallback, new InvalidateExprNode($normalizedExpr->var), $scope, $storage); $scope = $scope->invalidateExpression($normalizedExpr->var, true, $methodReflection->getDeclaringClass(), $methodReflection->isStatic()); } diff --git a/src/Analyser/ExprHandler/StaticCallHandler.php b/src/Analyser/ExprHandler/StaticCallHandler.php index e510200d21c..cd2a867a6ef 100644 --- a/src/Analyser/ExprHandler/StaticCallHandler.php +++ b/src/Analyser/ExprHandler/StaticCallHandler.php @@ -373,21 +373,30 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex } } - if ( - $expr->class instanceof Name - && $methodReflection !== null - && ( + if ($expr->class instanceof Name && $methodReflection !== null) { + $hasSideEffects = $methodReflection->hasSideEffects(); + if ( ( - !$methodReflection->isStatic() - && $methodReflection->getName() === '__construct' + ( + !$methodReflection->isStatic() + && $methodReflection->getName() === '__construct' + ) + || $hasSideEffects->yes() + // When possibly impure values are not remembered, a possibly impure method + // may also have changed the object's properties, so forget them too. + // Built-in methods are exempt, as they are from invalidating volatile expressions below. + || ( + !$this->rememberPossiblyImpureFunctionValues + && !$hasSideEffects->no() + && !$methodReflection->getDeclaringClass()->isBuiltin() + ) ) - || $methodReflection->hasSideEffects()->yes() - ) - && $scope->isInClass() - && $scope->getClassReflection()->is($methodReflection->getDeclaringClass()->getName()) - ) { - // a static method never receives $this, so property fetches on it survive - $scope = $scope->invalidateExpression(new Variable('this'), true, $methodReflection->getDeclaringClass(), $methodReflection->isStatic()); + && $scope->isInClass() + && $scope->getClassReflection()->is($methodReflection->getDeclaringClass()->getName()) + ) { + // a static method never receives $this, so property fetches on it survive + $scope = $scope->invalidateExpression(new Variable('this'), true, $methodReflection->getDeclaringClass(), $methodReflection->isStatic()); + } } if ( diff --git a/tests/PHPStan/Analyser/DoNotRememberPossiblyImpureFunctionValuesTest.php b/tests/PHPStan/Analyser/DoNotRememberPossiblyImpureFunctionValuesTest.php index 75a9797ee38..c0ba1711574 100644 --- a/tests/PHPStan/Analyser/DoNotRememberPossiblyImpureFunctionValuesTest.php +++ b/tests/PHPStan/Analyser/DoNotRememberPossiblyImpureFunctionValuesTest.php @@ -11,6 +11,7 @@ class DoNotRememberPossiblyImpureFunctionValuesTest extends TypeInferenceTestCas public static function dataAsserts(): iterable { yield from self::gatherAssertTypes(__DIR__ . '/data/do-not-remember-possibly-impure-function-values.php'); + yield from self::gatherAssertTypes(__DIR__ . '/data/do-not-remember-possibly-impure-function-values-properties.php'); } /** diff --git a/tests/PHPStan/Analyser/data/do-not-remember-possibly-impure-function-values-properties.php b/tests/PHPStan/Analyser/data/do-not-remember-possibly-impure-function-values-properties.php new file mode 100644 index 00000000000..5c8686d8605 --- /dev/null +++ b/tests/PHPStan/Analyser/data/do-not-remember-possibly-impure-function-values-properties.php @@ -0,0 +1,177 @@ +state = 'done'; + return true; + } + + /** @phpstan-pure */ + public function pure(): bool + { + return true; + } + + /** @phpstan-impure */ + public function impure(): bool + { + $this->state = 'done'; + return true; + } + + public static function advanceStatic(): bool + { + self::$staticState = 'done'; + return true; + } + + /** @phpstan-pure */ + public static function pureStatic(): bool + { + return true; + } + + public function doFoo(): void + { + $this->state = 'ready'; + $this->advance(); + assertType("'done'|'ready'", $this->state); + + $this->state = 'ready'; + if ($this->advance()) { + assertType("'done'|'ready'", $this->state); + } + + $this->state = 'ready'; + $this->pure(); + assertType("'ready'", $this->state); + + $this->state = 'ready'; + $this->impure(); + assertType("'done'|'ready'", $this->state); + } + + public function doLocalVariable(): void + { + $state = 'ready'; + $this->advance(); + assertType("'ready'", $state); + } + + public function doOtherObject(self $other): void + { + $this->state = 'ready'; + $other->state = 'ready'; + $other->advance(); + assertType("'done'|'ready'", $other->state); + assertType("'ready'", $this->state); + } + + public function doNullsafe(?self $other): void + { + if ($other === null) { + return; + } + + $other->state = 'ready'; + $other?->advance(); + assertType("'done'|'ready'", $other->state); + } + + public function doStatic(): void + { + self::$staticState = 'ready'; + self::advanceStatic(); + assertType("'done'|'ready'", self::$staticState); + + self::$staticState = 'ready'; + static::advanceStatic(); + assertType("'done'|'ready'", self::$staticState); + + self::$staticState = 'ready'; + self::pureStatic(); + assertType("'ready'", self::$staticState); + } + + public function doStaticKeepsInstanceProperties(): void + { + $this->state = 'ready'; + self::advanceStatic(); + assertType("'ready'", $this->state); + } + + public static function doStaticFromStaticMethod(): void + { + self::$staticState = 'ready'; + self::advanceStatic(); + assertType("'done'|'ready'", self::$staticState); + + self::$staticState = 'ready'; + self::pureStatic(); + assertType("'ready'", self::$staticState); + } + +} + +class Holder +{ + + public ?\stdClass $foo = null; + + /** @phpstan-assert-if-true !null $this->foo */ + public function hasFoo(): bool + { + return $this->foo !== null; + } + + public function doAssertIfTrue(): void + { + if ($this->hasFoo()) { + assertType('stdClass', $this->foo); + } + } + +} + +/** @extends \ArrayObject */ +class Bag extends \ArrayObject +{ + + public ?int $n = null; + + public function doBuiltinMethod(): void + { + $this->n = 1; + $this->count(); + $this->getIterator(); + assertType('1', $this->n); + } + +} + +class ChildStateMachine extends StateMachine +{ + + public function advance(): bool + { + $this->state = 'ready'; + parent::advance(); + assertType("'done'|'ready'", $this->state); + + return true; + } + +} diff --git a/tests/PHPStan/Analyser/nsrt/remember-possibly-impure-function-values-properties.php b/tests/PHPStan/Analyser/nsrt/remember-possibly-impure-function-values-properties.php new file mode 100644 index 00000000000..bc9226c3bb6 --- /dev/null +++ b/tests/PHPStan/Analyser/nsrt/remember-possibly-impure-function-values-properties.php @@ -0,0 +1,177 @@ +state = 'done'; + return true; + } + + /** @phpstan-pure */ + public function pure(): bool + { + return true; + } + + /** @phpstan-impure */ + public function impure(): bool + { + $this->state = 'done'; + return true; + } + + public static function advanceStatic(): bool + { + self::$staticState = 'done'; + return true; + } + + /** @phpstan-pure */ + public static function pureStatic(): bool + { + return true; + } + + public function doFoo(): void + { + $this->state = 'ready'; + $this->advance(); + assertType("'ready'", $this->state); + + $this->state = 'ready'; + if ($this->advance()) { + assertType("'ready'", $this->state); + } + + $this->state = 'ready'; + $this->pure(); + assertType("'ready'", $this->state); + + $this->state = 'ready'; + $this->impure(); + assertType("'done'|'ready'", $this->state); + } + + public function doLocalVariable(): void + { + $state = 'ready'; + $this->advance(); + assertType("'ready'", $state); + } + + public function doOtherObject(self $other): void + { + $this->state = 'ready'; + $other->state = 'ready'; + $other->advance(); + assertType("'ready'", $other->state); + assertType("'ready'", $this->state); + } + + public function doNullsafe(?self $other): void + { + if ($other === null) { + return; + } + + $other->state = 'ready'; + $other?->advance(); + assertType("'ready'", $other->state); + } + + public function doStatic(): void + { + self::$staticState = 'ready'; + self::advanceStatic(); + assertType("'ready'", self::$staticState); + + self::$staticState = 'ready'; + static::advanceStatic(); + assertType("'ready'", self::$staticState); + + self::$staticState = 'ready'; + self::pureStatic(); + assertType("'ready'", self::$staticState); + } + + public function doStaticKeepsInstanceProperties(): void + { + $this->state = 'ready'; + self::advanceStatic(); + assertType("'ready'", $this->state); + } + + public static function doStaticFromStaticMethod(): void + { + self::$staticState = 'ready'; + self::advanceStatic(); + assertType("'ready'", self::$staticState); + + self::$staticState = 'ready'; + self::pureStatic(); + assertType("'ready'", self::$staticState); + } + +} + +class Holder +{ + + public ?\stdClass $foo = null; + + /** @phpstan-assert-if-true !null $this->foo */ + public function hasFoo(): bool + { + return $this->foo !== null; + } + + public function doAssertIfTrue(): void + { + if ($this->hasFoo()) { + assertType('stdClass', $this->foo); + } + } + +} + +/** @extends \ArrayObject */ +class Bag extends \ArrayObject +{ + + public ?int $n = null; + + public function doBuiltinMethod(): void + { + $this->n = 1; + $this->count(); + $this->getIterator(); + assertType('1', $this->n); + } + +} + +class ChildStateMachine extends StateMachine +{ + + public function advance(): bool + { + $this->state = 'ready'; + parent::advance(); + assertType("'ready'", $this->state); + + return true; + } + +} diff --git a/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesDoNotRememberPossiblyImpureValuesRuleTest.php b/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesDoNotRememberPossiblyImpureValuesRuleTest.php new file mode 100644 index 00000000000..9967f47f82b --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/StrictComparisonOfDifferentTypesDoNotRememberPossiblyImpureValuesRuleTest.php @@ -0,0 +1,47 @@ + + */ +class StrictComparisonOfDifferentTypesDoNotRememberPossiblyImpureValuesRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + // @phpstan-ignore argument.type + return new CompositeRule([ + new StrictComparisonOfDifferentTypesRule( + self::getContainer()->getByType(RicherScopeGetTypeHelper::class), + self::getContainer()->getByType(ConstantConditionInTraitHelper::class), + true, + false, + true, + ), + new ConstantConditionInTraitRule(), + ]); + } + + public function testBug15734(): void + { + $this->analyse([__DIR__ . '/data/bug-15734.php'], []); + } + + public static function getAdditionalConfigFiles(): array + { + return array_merge( + parent::getAdditionalConfigFiles(), + [ + __DIR__ . '/doNotRememberPossiblyImpureValues.neon', + ], + ); + } + +} diff --git a/tests/PHPStan/Rules/Comparison/data/bug-15734.php b/tests/PHPStan/Rules/Comparison/data/bug-15734.php new file mode 100644 index 00000000000..3293c4b64ce --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/bug-15734.php @@ -0,0 +1,27 @@ += 7.4 + +declare(strict_types = 1); + +namespace Bug15734; + +class Tokenizer +{ + /** @var 'ready'|'matched'|'complete' */ + private string $state = 'ready'; + + public function nextToken(): bool + { + $this->state = 'ready'; + if (!$this->parseNextTag()) { + return $this->state === 'complete'; + } + + return $this->state === 'matched'; + } + + private function parseNextTag(): bool + { + $this->state = rand(0, 1) === 1 ? 'matched' : 'complete'; + return $this->state === 'matched'; + } +} diff --git a/turbo-ext/src/MethodCallHandler.cpp b/turbo-ext/src/MethodCallHandler.cpp index 69c2bef129e..118fdff32f2 100644 --- a/turbo-ext/src/MethodCallHandler.cpp +++ b/turbo-ext/src/MethodCallHandler.cpp @@ -447,6 +447,16 @@ class MethodCallHandler zend_long hasSideEffects = reflectionTrinary(methodReflection.raw(), PT_MR_HAS_SIDE_EFFECTS); if (UNEXPECTED(hasSideEffects < 0)) return zv::Val(); invalidate = hasSideEffects == PT_TRI_YES; + // When possibly impure values are not remembered, a possibly impure method + // may also have changed the object's properties, so forget them too. + // Built-in methods are exempt, as they are from invalidating volatile expressions below. + if (!invalidate && !zend_is_true(OBJ_PROP_NUM(self, slots::rememberPossiblyImpureFunctionValues)) && hasSideEffects != PT_TRI_NO) { + zv::Val declaringClass = pt_extended_method_reflection_call(methodReflection.raw(), PT_MR_GET_DECLARING_CLASS); + if (UNEXPECTED(declaringClass.isUndef())) return zv::Val(); + bool builtin; + if (UNEXPECTED(!pt_class_reflection_is_builtin(Z_OBJ_P(declaringClass.raw()), builtin))) return zv::Val(); + invalidate = !builtin; + } } if (invalidate) { zval *normalizedVar = exprVar(normalizedExpr.raw()); diff --git a/turbo-ext/src/StaticCallHandler.cpp b/turbo-ext/src/StaticCallHandler.cpp index a18c204676d..cb31dc8e056 100644 --- a/turbo-ext/src/StaticCallHandler.cpp +++ b/turbo-ext/src/StaticCallHandler.cpp @@ -680,6 +680,16 @@ class StaticCallHandler zend_long hasSideEffects = reflectionTrinary(methodReflection.raw(), PT_MR_HAS_SIDE_EFFECTS); if (UNEXPECTED(hasSideEffects < 0)) return zv::Val(); invalidate = hasSideEffects == PT_TRI_YES; + // When possibly impure values are not remembered, a possibly impure method + // may also have changed the object's properties, so forget them too. + // Built-in methods are exempt, as they are from invalidating volatile expressions below. + if (!invalidate && !zend_is_true(OBJ_PROP_NUM(self, slots::rememberPossiblyImpureFunctionValues)) && hasSideEffects != PT_TRI_NO) { + zv::Val declaringClass = pt_extended_method_reflection_call(methodReflection.raw(), PT_MR_GET_DECLARING_CLASS); + if (UNEXPECTED(declaringClass.isUndef())) return zv::Val(); + bool builtin; + if (UNEXPECTED(!pt_class_reflection_is_builtin(Z_OBJ_P(declaringClass.raw()), builtin))) return zv::Val(); + invalidate = !builtin; + } } if (invalidate && UNEXPECTED(!scopeClassIsDeclaringClass(currentScope.raw(), methodReflection.raw(), invalidate))) return zv::Val(); if (invalidate) {