Do not remember a void equality assertion statement as true when its arguments contain an impure call - #6625
Conversation
…arguments contain an impure call - ExpressionHandler stores the result of a void `@phpstan-assert =Type` call statement as `true` so a duplicate assertion is reported as always-true. It did so even when an argument contained an impure call (`assertSame($key, $counter->next())`, `assertSame($key, random_int(1, 10))`), so the repeated assertion was wrongly reported as always true. - The statement is now remembered only when none of its impure points other than the call's own (a void call is always impure) come from a call - the same gate DefaultNarrowingHelper::isSubjectValueRemembered() applies, honouring rememberPossiblyImpureFunctionValues for possibly-impure calls. - Ported the gate to the native ExpressionHandler mirror in turbo-ext. - Covered function, instance method (`$this->assertSame()`) and static method (`self::assertSame()`) assertions, impure calls in both the asserted and the expected argument, and kept the duplicate report for pure arguments. - Probed the `if (isSame($key, random_int(1, 10)))` counterpart (`@phpstan-assert-if-true =Type`): the nested duplicate is still reported, but that comes from the long-standing default narrowing of any call in a condition, which gates only on the called function's own purity, not on its arguments - a separate, pre-existing behaviour left unchanged here.
|
//cc @SanderMuller |
|
This seems ok, but should target 2.2.x to fix the 2.2.15 release IMHO @staabm. |
|
@VincentLanglet in https://github.com/phpstan/phpstan-src/pull/6628/files there is a 2.2.x variant of this. I am not sure we should merge it though, as its a fix in classes which largely changed in 2.3.x which means this will get hard to merge (and 2.3.0 will ship likely this week) |
Sure I'm ok waiting for 2.3.0 if we/ondrej doesn't think it's a big enough bug. The fix in https://github.com/phpstan/phpstan-src/pull/6628/changes seems simpler than this one, but maybe it's different because classes changed (?) |
|
merging per PM with ondrej. |
yes, its using a method which was removed in 2.3.x |
3 participants
Summary
Since 2.2.15, calling a void assertion annotated with
@phpstan-assert =ExpectedType $actual(e.g. PHPUnit'sassertSame()) twice with an impure call as an argument reported the second call asalreadyNarrowedType:The statement handler remembered the whole assertion call as
true, even though the impure argument gives a new value on every call. Now it only does that when the call's arguments contain no impure call.Changes
src/Analyser/StmtHandler/ExpressionHandler.php: store the statement'strueresult for an equality assertion only whenisCallRememberedDespiteItsOwnImpurity()holds. This means no impure point other than the call's own (a void call is always impure) comes from a function, method, static or nullsafe method call. Possibly-impure calls are skipped whenrememberPossiblyImpureFunctionValuesis enabled, which matchesDefaultNarrowingHelper::isSubjectValueRemembered(). The parameter is autowired into the constructor.turbo-ext/src/ExpressionHandler.cppandturbo-ext/src/generated/ExpressionHandler.h: the same gate and the new constructor parameter in the native mirror.Root cause
ExpressionHandlerassignedtrueto the assertion call expression when the statement's narrowing was an equality narrowing. It never checked whether the call could be repeated with the same outcome. The firstassertSame($key, random_int(1, 10))storedassertSame($key, random_int(1, 10))astrue. Nothing invalidated that entry, becauserandom_int()touches no variable. SoImpossibleCheckTypeHelperread the storedtruefor the second call. With$counter->next()an intervening$counter->next();statement invalidated the entry, which matches the observations in the issue.Test
tests/PHPStan/Rules/Comparison/data/bug-15328.php(ImpossibleCheckTypeFunctionCallRuleTest::testBug15328) contains the reproducer from the issue: an impure method and an impure function as$actual. It adds an impure call as$expected(the narrowed$actualmust not make the repeat look redundant) and a pure duplicate, which is still reported.tests/PHPStan/Rules/Comparison/data/bug-15328-method.php(ImpossibleCheckTypeMethodCallRuleTestandImpossibleCheckTypeStaticMethodCallRuleTest::testBug15328) covers the PHPUnit-style analogues$this->assertSame()andself::assertSame(), with impure and pure arguments.All the new expectations failed before the fix.
I also probed the
ifcounterpart,@phpstan-assert-if-true =Typeinside nestedifs withrandom_int(). The inner duplicate is still reported. That comes from the default narrowing of any call used as a condition:if (isFive(random_int(1, 10)))also remembers the call astrue. That narrowing checks only the called function's own purity, not its arguments. The behaviour predates this regression, so this PR leaves it unchanged.Fixes phpstan/phpstan#15328
🤖 Generated with Claude Code