diff --git a/src/Analyser/StmtHandler/ExpressionHandler.php b/src/Analyser/StmtHandler/ExpressionHandler.php index 00fee299360..52faea1c57d 100644 --- a/src/Analyser/StmtHandler/ExpressionHandler.php +++ b/src/Analyser/StmtHandler/ExpressionHandler.php @@ -8,6 +8,7 @@ use PhpParser\Node\Stmt\Expression; use PHPStan\Analyser\ExpressionContext; use PHPStan\Analyser\ExpressionResultStorage; +use PHPStan\Analyser\ImpurePoint; use PHPStan\Analyser\InternalStatementExitPoint; use PHPStan\Analyser\InternalStatementResult; use PHPStan\Analyser\MutatingScope; @@ -17,6 +18,7 @@ use PHPStan\Analyser\StatementsHandler; use PHPStan\Analyser\StmtHandler; use PHPStan\Analyser\TypeSpecifierContext; +use PHPStan\DependencyInjection\AutowiredParameter; use PHPStan\DependencyInjection\AutowiredService; use PHPStan\Node\NoopExpressionNode; use PHPStan\Node\PropertyAssignNode; @@ -37,6 +39,8 @@ final class ExpressionHandler implements StmtHandler public function __construct( private StatementsHandler $statementsHandler, + #[AutowiredParameter] + private bool $rememberPossiblyImpureFunctionValues, ) { } @@ -102,7 +106,7 @@ public function processStmt( $specifiedTypes = $result->getSpecifiedTypesForScope($scope, TypeSpecifierContext::createNull()); $scope = $scope->applySpecifiedTypes($specifiedTypes); - if ($specifiedTypes->isEquality()) { + if ($specifiedTypes->isEquality() && $this->isCallRememberedDespiteItsOwnImpurity($result->getImpurePoints(), $stmt->expr)) { // Statement counterpart of ExpressionResult's equality handling: // store the call's true result so a duplicate void assertion statement is // reported as always-true. We assign directly because void calls have no @@ -127,4 +131,38 @@ public function processStmt( return new InternalStatementResult($scope, hasYield: $hasYield, isAlwaysTerminating: $isAlwaysTerminating, exitPoints: [], throwPoints: $throwPoints, impurePoints: $impurePoints, variableFlow: $result->getVariableFlow()); } + /** + * A void assertion is impure by itself, but repeating it only repeats the + * same check when nothing else in it has side effects - an impure call in + * its arguments (`assertSame($a, $counter->next())`) yields a new value + * every time. + * + * @param ImpurePoint[] $impurePoints + */ + private function isCallRememberedDespiteItsOwnImpurity(array $impurePoints, Expr $call): bool + { + foreach ($impurePoints as $impurePoint) { + $node = $impurePoint->getNode(); + if ($node === $call) { + continue; + } + if ( + !$node instanceof Expr\FuncCall + && !$node instanceof Expr\MethodCall + && !$node instanceof Expr\StaticCall + && !$node instanceof Expr\NullsafeMethodCall + ) { + continue; + } + + if (!$impurePoint->isCertain() && $this->rememberPossiblyImpureFunctionValues) { + continue; + } + + return false; + } + + return true; + } + } diff --git a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php index 91fc26544f5..64470d7ba3c 100644 --- a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeFunctionCallRuleTest.php @@ -537,6 +537,17 @@ public function testBug14705Php8(): void ]); } + #[RequiresPhp('>= 8.0.0')] + public function testBug15328(): void + { + $this->analyse([__DIR__ . '/data/bug-15328.php'], [ + [ + 'Call to function assertSame() with int and int will always evaluate to true.', + 59, + ], + ]); + } + public function testBug2755(): void { $this->analyse([__DIR__ . '/data/bug-2755.php'], []); diff --git a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php index 67424026bdf..ff111cce5e7 100644 --- a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeMethodCallRuleTest.php @@ -320,6 +320,18 @@ public function testBug10337(): void $this->analyse([__DIR__ . '/data/bug-10337.php'], []); } + #[RequiresPhp('>= 8.0.0')] + public function testBug15328(): void + { + $this->treatPhpDocTypesAsCertain = true; + $this->analyse([__DIR__ . '/data/bug-15328-method.php'], [ + [ + 'Call to method Bug15328Method\\Assert::assertSame() with int and int will always evaluate to true.', + 69, + ], + ]); + } + public function testBug14705(): void { $this->treatPhpDocTypesAsCertain = true; diff --git a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeStaticMethodCallRuleTest.php b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeStaticMethodCallRuleTest.php index fcb377a5492..75c47afe7f7 100644 --- a/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeStaticMethodCallRuleTest.php +++ b/tests/PHPStan/Rules/Comparison/ImpossibleCheckTypeStaticMethodCallRuleTest.php @@ -126,6 +126,18 @@ public function testBug15223TreatPhpDocTypesAsCertain(): void $this->analyse([__DIR__ . '/data/bug-15223-static.php'], []); } + #[RequiresPhp('>= 8.0.0')] + public function testBug15328(): void + { + $this->treatPhpDocTypesAsCertain = true; + $this->analyse([__DIR__ . '/data/bug-15328-method.php'], [ + [ + 'Call to static method Bug15328Method\\Assert::assertSame() with int and int will always evaluate to true.', + 75, + ], + ]); + } + public function testAssertUnresolvedGeneric(): void { $this->treatPhpDocTypesAsCertain = true; diff --git a/tests/PHPStan/Rules/Comparison/data/bug-15328-method.php b/tests/PHPStan/Rules/Comparison/data/bug-15328-method.php new file mode 100644 index 00000000000..a7bd3899c8b --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/bug-15328-method.php @@ -0,0 +1,78 @@ += 8.0 + +declare(strict_types = 1); + +namespace Bug15328Method; + +use Exception; + +class Assert +{ + + /** + * @template ExpectedType + * + * @param ExpectedType $expected + * + * @phpstan-assert =ExpectedType $actual + */ + final public static function assertSame(mixed $expected, mixed $actual): void + { + if ($expected !== $actual) { + throw new Exception; + } + } + +} + +final class Counter +{ + private int $count = 0; + + /** @phpstan-impure */ + public function next(): int + { + return ++$this->count; + } +} + +final class ContextTest extends Assert +{ + + public function testMethodCall(int $key): void + { + $counter = new Counter; + + $this->assertSame($key, $counter->next()); + $this->assertSame($key, $counter->next()); + } + + public function testStaticCall(int $key): void + { + $counter = new Counter; + + self::assertSame($key, $counter->next()); + self::assertSame($key, $counter->next()); + } + + public function testImpureFunction(int $key): void + { + $this->assertSame($key, random_int(1, 10)); + $this->assertSame($key, random_int(1, 10)); + self::assertSame($key, random_int(1, 10)); + self::assertSame($key, random_int(1, 10)); + } + + public function testPureMethodCall(int $key, int $actual): void + { + $this->assertSame($key, $actual); + $this->assertSame($key, $actual); + } + + public function testPureStaticCall(int $key, int $actual): void + { + self::assertSame($key, $actual); + self::assertSame($key, $actual); + } + +} diff --git a/tests/PHPStan/Rules/Comparison/data/bug-15328.php b/tests/PHPStan/Rules/Comparison/data/bug-15328.php new file mode 100644 index 00000000000..c98885952be --- /dev/null +++ b/tests/PHPStan/Rules/Comparison/data/bug-15328.php @@ -0,0 +1,60 @@ += 8.0 + +declare(strict_types = 1); + +namespace Bug15328; + +use Exception; + +/** + * Same signature and PHPDoc as PHPUnit\Framework\Assert::assertSame() + * + * @template ExpectedType + * + * @param ExpectedType $expected + * + * @phpstan-assert =ExpectedType $actual + */ +function assertSame(mixed $expected, mixed $actual): void +{ + if ($expected !== $actual) { + throw new Exception; + } +} + +final class Counter +{ + private int $count = 0; + + /** @phpstan-impure */ + public function next(): int + { + return ++$this->count; + } +} + +function impureMethod(int $key): void +{ + $counter = new Counter; + + assertSame($key, $counter->next()); + assertSame($key, $counter->next()); +} + +function impureFunction(int $key): void +{ + assertSame($key, random_int(1, 10)); + assertSame($key, random_int(1, 10)); +} + +function impureExpected(int $actual): void +{ + assertSame(random_int(1, 10), $actual); + assertSame(random_int(1, 10), $actual); +} + +function pure(int $key, int $actual): void +{ + assertSame($key, $actual); + assertSame($key, $actual); +} diff --git a/turbo-ext/src/ExpressionHandler.cpp b/turbo-ext/src/ExpressionHandler.cpp index d22d35b011a..a92053342cd 100644 --- a/turbo-ext/src/ExpressionHandler.cpp +++ b/turbo-ext/src/ExpressionHandler.cpp @@ -68,10 +68,12 @@ class ExpressionHandler public: explicit ExpressionHandler(zend_object *self) : self(self) {} - /* the constructor body: the promoted property */ - void construct(zval *statementsHandler) + /* the constructor body: the promoted properties */ + void construct(zval *statementsHandler, bool rememberPossiblyImpureFunctionValues) { - zv::ObjRef(self).propAtWrite(slots::statementsHandler, zv::Val::copyOf(zv::Ref(statementsHandler))); + zv::ObjRef object(self); + object.propAtWrite(slots::statementsHandler, zv::Val::copyOf(zv::Ref(statementsHandler))); + object.propAtWrite(slots::rememberPossiblyImpureFunctionValues, zv::Val::boolean(rememberPossiblyImpureFunctionValues)); } /* Mirrors supports(); false = pending exception */ @@ -166,13 +168,20 @@ class ExpressionHandler bool isEquality; if (UNEXPECTED(!pt_specified_types_is_equality(specifiedTypes.raw(), isEquality))) return zv::Val(); + zval *statementExpression = NULL; + bool remembered = false; if (isEquality) { + statementExpression = statementExpr(stmt); + if (UNEXPECTED(statementExpression == NULL)) return zv::Val(); + impurePoints = pt_expression_result_impure_points(resultValue, impurePointsHold); + if (UNEXPECTED(impurePoints == NULL)) return zv::Val(); + if (UNEXPECTED(!isCallRememberedDespiteItsOwnImpurity(impurePoints, statementExpression, remembered))) return zv::Val(); + } + if (remembered) { // Statement counterpart of ExpressionResult's equality handling: // store the call's true result so a duplicate void assertion statement is // reported as always-true. Assigned directly because void calls have no // return value to protect, and intersecting true with void would produce never. - zval *statementExpression = statementExpr(stmt); - if (UNEXPECTED(statementExpression == NULL)) return zv::Val(); zval trueType, trueNativeType; if (UNEXPECTED(!pt_constant_boolean_type_new(&trueType, true))) return zv::Val(); zv::Val trueTypeHold = zv::Val::adopt(trueType); @@ -223,6 +232,51 @@ class ExpressionHandler private: zend_object *self; + /* Mirrors isCallRememberedDespiteItsOwnImpurity(); false = pending exception */ + [[nodiscard]] bool isCallRememberedDespiteItsOwnImpurity(zval *impurePoints, zval *call, bool &out) const + { + if (UNEXPECTED(Z_TYPE_P(impurePoints) != IS_ARRAY)) { + zend_type_error("foreach() argument must be of type array|object, %s given", zend_zval_value_name(impurePoints)); + return false; + } + /* the slot is borrowed: keep the array alive across the calls */ + zv::Val impurePointsCopy = zv::Val::copyOf(zv::Ref(impurePoints)); + for (zv::ArrayEntry entry : zv::ArrRef(impurePointsCopy.raw())) { + zval *impurePoint = entry.value().deref().raw(); + if (UNEXPECTED(Z_TYPE_P(impurePoint) != IS_OBJECT)) { + memberCallOnNonObject("getNode", impurePoint); + return false; + } + zv::Val nodeHold; + zval *node = pt_impure_point_node(impurePoint, nodeHold); + if (UNEXPECTED(node == NULL)) return false; + if (Z_TYPE_P(node) == IS_OBJECT && Z_TYPE_P(call) == IS_OBJECT && Z_OBJ_P(node) == Z_OBJ_P(call)) { + continue; + } + bool error = false; + bool isCall = ptsh::isInstanceOf(node, PT_CLASS_FUNC_CALL, error) + || ptsh::isInstanceOf(node, PT_CLASS_METHOD_CALL, error) + || ptsh::isInstanceOf(node, PT_CLASS_STATIC_CALL, error) + || ptsh::isInstanceOf(node, PT_CLASS_NULLSAFE_METHOD_CALL, error); + if (UNEXPECTED(error)) return false; + if (!isCall) { + continue; + } + + bool certain = false; + if (UNEXPECTED(!pt_impure_point_is_certain(impurePoint, certain))) return false; + if (!certain && Z_TYPE_P(OBJ_PROP_NUM(self, slots::rememberPossiblyImpureFunctionValues)) == IS_TRUE) { + continue; + } + + out = false; + return true; + } + + out = true; + return true; + } + /* the twin's try block: the expression walked, a thrown expression's * @var-changed-type node emitted */ static zv::Val processExpression(zval *nodeScopeResolver, zval *stmt, zval *scope, zval *storage, zval *nodeCallback, zval *context, zval *statementsHandler, zval *preAnnotationScope) @@ -332,8 +386,9 @@ PT_MINIT_REGISTRATION(pt_register_expression_handler) * service by reflecting the constructor */ cls.method(sigs::__construct, [](INTERNAL_FUNCTION_PARAMETERS) { zval *statementsHandler; - if (!zp::parse(execute_data, statementsHandler)) RETURN_THROWS(); - ExpressionHandler(Z_OBJ_P(ZEND_THIS)).construct(statementsHandler); + bool rememberPossiblyImpureFunctionValues; + if (!zp::parse(execute_data, statementsHandler, rememberPossiblyImpureFunctionValues)) RETURN_THROWS(); + ExpressionHandler(Z_OBJ_P(ZEND_THIS)).construct(statementsHandler, rememberPossiblyImpureFunctionValues); }); cls.method<&ExpressionHandler::supports, zp::Obj>(sigs::supports); diff --git a/turbo-ext/src/generated/ExpressionHandler.h b/turbo-ext/src/generated/ExpressionHandler.h index 6cb1d3ad709..719a7b66636 100644 --- a/turbo-ext/src/generated/ExpressionHandler.h +++ b/turbo-ext/src/generated/ExpressionHandler.h @@ -11,6 +11,7 @@ namespace ptdecl::ExpressionHandler { /* the OBJ_PROP_NUM slots of the instance properties the class declares (the inherited ones come first) */ namespace slot { inline constexpr uint32_t statementsHandler = 0; +inline constexpr uint32_t rememberPossiblyImpureFunctionValues = 1; } // namespace slot inline void declareClass(reg::Class &cls) @@ -23,6 +24,7 @@ inline void declareClass(reg::Class &cls) inline void declareProperties(reg::Class &cls) { cls.property("statementsHandler", ZEND_ACC_PRIVATE, reg::PropertyKind::Typed, 0, "PHPStan\\Analyser\\StatementsHandler"); + cls.property("rememberPossiblyImpureFunctionValues", ZEND_ACC_PRIVATE, reg::PropertyKind::Typed, MAY_BE_BOOL); } /* the string and parameter tables the signatures below index into (see reg::Sig) */ @@ -30,42 +32,52 @@ namespace sigtab { inline constexpr char strings[] = "statementsHandler\0" /* 0 */ "PHPStan\\Analyser\\StatementsHandler\0" /* 18 */ - "__construct\0" /* 53 */ - "stmt\0" /* 65 */ - "PhpParser\\Node\\Stmt\0" /* 70 */ - "\0" /* 90 */ - "supports\0" /* 91 */ - "nodeScopeResolver\0" /* 100 */ - "PHPStan\\Analyser\\NodeScopeResolver\0" /* 118 */ - "scope\0" /* 153 */ - "PHPStan\\Analyser\\MutatingScope\0" /* 159 */ - "storage\0" /* 190 */ - "PHPStan\\Analyser\\ExpressionResultStorage\0" /* 198 */ - "nodeCallback\0" /* 239 */ - "context\0" /* 252 */ - "PHPStan\\Analyser\\StatementContext\0" /* 260 */ - "PHPStan\\Analyser\\InternalStatementResult\0" /* 294 */ - "processStmt"; /* 335 */ + "rememberPossiblyImpureFunctionValues\0" /* 53 */ + "__construct\0" /* 90 */ + "stmt\0" /* 102 */ + "PhpParser\\Node\\Stmt\0" /* 107 */ + "\0" /* 127 */ + "supports\0" /* 128 */ + "nodeScopeResolver\0" /* 137 */ + "PHPStan\\Analyser\\NodeScopeResolver\0" /* 155 */ + "scope\0" /* 190 */ + "PHPStan\\Analyser\\MutatingScope\0" /* 196 */ + "storage\0" /* 227 */ + "PHPStan\\Analyser\\ExpressionResultStorage\0" /* 235 */ + "nodeCallback\0" /* 276 */ + "context\0" /* 289 */ + "PHPStan\\Analyser\\StatementContext\0" /* 297 */ + "PHPStan\\Analyser\\InternalStatementResult\0" /* 331 */ + "processStmt\0" /* 372 */ + "impurePoints\0" /* 384 */ + "call\0" /* 397 */ + "PhpParser\\Node\\Expr\0" /* 402 */ + "isCallRememberedDespiteItsOwnImpurity"; /* 422 */ inline constexpr reg::PackedArg args[] = { reg::packed(0, 0, 18), /* __construct $statementsHandler */ - reg::packed(65, 0, 70), /* supports $stmt */ - reg::packed(90, MAY_BE_BOOL), /* supports return */ - reg::packed(100, 0, 118), /* processStmt $nodeScopeResolver */ - reg::packed(65, 0, 70), /* processStmt $stmt */ - reg::packed(153, 0, 159), /* processStmt $scope */ - reg::packed(190, 0, 198), /* processStmt $storage */ - reg::packed(239, MAY_BE_CALLABLE), /* processStmt $nodeCallback */ - reg::packed(252, 0, 260), /* processStmt $context */ - reg::packed(90, 0, 294), /* processStmt return */ + reg::packed(53, MAY_BE_BOOL), /* __construct $rememberPossiblyImpureFunctionValues */ + reg::packed(102, 0, 107), /* supports $stmt */ + reg::packed(127, MAY_BE_BOOL), /* supports return */ + reg::packed(137, 0, 155), /* processStmt $nodeScopeResolver */ + reg::packed(102, 0, 107), /* processStmt $stmt */ + reg::packed(190, 0, 196), /* processStmt $scope */ + reg::packed(227, 0, 235), /* processStmt $storage */ + reg::packed(276, MAY_BE_CALLABLE), /* processStmt $nodeCallback */ + reg::packed(289, 0, 297), /* processStmt $context */ + reg::packed(127, 0, 331), /* processStmt return */ + reg::packed(384, MAY_BE_ARRAY), /* isCallRememberedDespiteItsOwnImpurity $impurePoints */ + reg::packed(397, 0, 402), /* isCallRememberedDespiteItsOwnImpurity $call */ + reg::packed(127, MAY_BE_BOOL), /* isCallRememberedDespiteItsOwnImpurity return */ }; using Sig = reg::Sig; } // namespace sigtab /* the signatures of the methods the class declares itself (a used trait's are in the trait's header) */ namespace sig { -inline constexpr sigtab::Sig __construct = { { 53 /* __construct */, 1, 0, 1, reg::NoArg, ZEND_ACC_PUBLIC } }; -inline constexpr sigtab::Sig supports = { { 91 /* supports */, 1, 1, 1, 2, ZEND_ACC_PUBLIC } }; -inline constexpr sigtab::Sig processStmt = { { 335 /* processStmt */, 6, 3, 6, 9, ZEND_ACC_PUBLIC } }; +inline constexpr sigtab::Sig __construct = { { 90 /* __construct */, 2, 0, 2, reg::NoArg, ZEND_ACC_PUBLIC } }; +inline constexpr sigtab::Sig supports = { { 128 /* supports */, 1, 2, 1, 3, ZEND_ACC_PUBLIC } }; +inline constexpr sigtab::Sig processStmt = { { 372 /* processStmt */, 6, 4, 6, 10, ZEND_ACC_PUBLIC } }; +inline constexpr sigtab::Sig isCallRememberedDespiteItsOwnImpurity = { { 422 /* isCallRememberedDespiteItsOwnImpurity */, 2, 11, 2, 13, ZEND_ACC_PRIVATE } }; } // namespace sig } // namespace ptdecl::ExpressionHandler