Repository navigation
Forget narrowed properties after a possibly impure method call when its values are not remembered - #7073
Conversation
…ts values are not remembered
With `rememberPossiblyImpureFunctionValues: false`, the return value of a method that only maybe has side effects is no longer remembered, but what was narrowed about the properties of the object it was called on still was. So a method that changes a property it was not declared impure for left its callers with a stale type, and a comparison of that property after the call was reported as always true or always false:
$this->state = self::STATE_READY;
if (!$this->parseNextTag()) {
return $this->state === self::STATE_COMPLETE; // reported as always false
}
When the option is off, a method call and a static call within the declaring class now invalidate the object's properties, and a static call its static properties, unless the method is known to have no side effects, as they already did for a method that has them. With the option on, which is the default, nothing changes.
The native MethodCallHandler and StaticCallHandler of the turbo extension get the same condition.
The tests cover calls on `$this`, on another object, and through `?->`, `self::`, `static::`, and `parent::`, and that a pure method and a local variable are left alone. A counterpart fixture holds the default behavior, and a rule test shows the comparison above is no longer reported.
Co-Authored-By: Claude Opus 5.5 (medium effort) <noreply@anthropic.com>
Claude-Code-Session-Id: dfb24569-a61a-4404-8588-60a387d2cccb
|
🤖 Comment by Claude Opus 5.5 (medium effort) The PMMP Tests failure is caused by this change: PocketMine-MP sets DetailsJob: https://github.com/phpstan/phpstan-src/actions/runs/43651082812/job/132623385937 (PocketMine-MP 5.44.3, whose Two representative cases:
All 16 errors:
Namespaces are shortened. Each could be addressed in PocketMine-MP by marking the intervening method The other failures are not caused by this PR:
|
… the issue's reproducer verbatim A possibly impure method declared in a built-in class, such as `ArrayObject::getIterator()` called on a subclass, invalidated the object's narrowed properties when `rememberPossiblyImpureFunctionValues` is false. The invalidation of volatile expressions after the same call already exempts built-in methods, since they do not change userland properties. The new condition now exempts them too, in the native `MethodCallHandler` and `StaticCallHandler` of the turbo extension as well. `hasSideEffects()` is no longer evaluated twice in the same condition; it is not memoized. In `StaticCallHandler`, the condition is nested once so the value can be held in a variable. The rule test's fixture is now `bug-15734.php`, the reproducer from the issue copied verbatim, and it expects no errors. The comparison after a pure call that the old fixture also covered is already covered by the type inference fixture. Both type inference fixtures gain cases for a method that asserts a property of `$this` with `@phpstan-assert-if-true`, for a static call made from a static method, and for calls to built-in methods. Co-Authored-By: Claude Opus 5.5 (medium effort) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tnky3JtcvQrtFGa2fwY9Ay Claude-Code-Session-Id: dfb24569-a61a-4404-8588-60a387d2cccb
|
This pull request has been marked as ready for review. |
Closes phpstan/phpstan#15734
With
rememberPossiblyImpureFunctionValues: false, the return value of a method that only maybe has side effects is no longer remembered, but what was narrowed about the properties of the object it was called on still is. This makes a call to such a method also invalidate those properties, as a call to a method that has side effects already does:$obj->foo(),$obj?->foo()) invalidates$obj.self::,static::,parent::) invalidates$this, or only the static properties when the method is static.A method known to have no side effects, such as one marked
@phpstan-pure, still leaves everything narrowed. So does a method declared in a built-in class, as it already does for the invalidation of volatile expressions after the call. With the option on, which is the default, nothing changes.The change adds one condition to the invalidation in
MethodCallHandlerandStaticCallHandler:The turbo extension's
MethodCallHandler.cppandStaticCallHandler.cppget the same condition, reading therememberPossiblyImpureFunctionValuesslot they already have.Scope
Only the object a method is called on is invalidated, mirroring what already happens for a method that has side effects. These keep their narrowing after a possibly impure call:
$walker->visit($node)followed by a check of$node->state.StaticCallHandleronly invalidates when the scope's class is the declaring class.FuncCallHandler.Aliasing is not considered either: after
$other->advance(), the properties of$thisstay narrowed, though$othermay be$this.Performance
With the option off, nearly every non-void
$this->method()call not known to be pure now invalidates$this. Invalidating$thiscannot use the key prefilter inScopeOps, so each such call scans all narrowed expressions and conditional expressions. The benchmarks all run with the option on, so they do not measure this.hasSideEffects(), which is not memoized, is read once for the whole condition.Tests
do-not-remember-possibly-impure-function-values-properties.php, added toDoNotRememberPossiblyImpureFunctionValuesTest, covers:$this, on another object, and through?->,self::,static::, andparent::;@phpstan-assert-if-truestays narrowed.nsrt/remember-possibly-impure-function-values-properties.phpis the same fixture under the default configuration, where every narrowing is kept.StrictComparisonOfDifferentTypesDoNotRememberPossiblyImpureValuesRuleTestruns the rule with the option off onbug-15734.php, the reproducer from the issue, and expects no errors.Without the change, the expectations it affects fail, and the reproducer reports both comparisons from the issue. With the PHP change reverted and only the native one in place, the tests pass with the extension loaded, so both implementations are covered. Analysing the fixtures with the option off gives identical output with and without the extension.
The full test suite passes with the turbo extension loaded, as do
side-by-side.php,signature-parity.php, andsmoke.php.Open questions
rememberPossiblyImpureFunctionValues, whose name and documentation refer to return values. Projects that already set it to false get new errors, as the PMMP integration test shows (see the comment below). It could instead be put behind bleeding edge or given its own option.FuncCallHandlercould do the same for global state after a possibly impure function call. That is not part of this PR.🤖 Generated with Claude Code using Claude Opus 5.5 (medium effort)