Improve pure method detection and mark Redis::get explicitly impure #2795
Conversation
|
You've opened the pull request against the latest branch 1.11.x. If your code is relevant on 1.10.x and you want it to be released sooner, please rebase your pull request and change its target to 1.10.x. |
| @@ -429,6 +431,9 @@ public function hasSideEffects(): TrinaryLogic | |||
| if ($this->isPure !== null) { | |||
| return TrinaryLogic::createFromBoolean(!$this->isPure); | |||
| } | |||
| if (count($this->getParameters()) === 0 && preg_match('#^(get|is|has|can)[A-Z0-9_]#', $this->getName()) === 1) { | |||
There was a problem hiding this comment.
I'm sorry, I can't accept this part, it'd be a big BC break.
There was a problem hiding this comment.
I don't think it's a BC break? Let's assume someone out there created a function that has no parameters and is named like a getter, and gave it side effects. With this change, it would be considered "Pure" instead of "Maybe Pure", which doesn't change anything with default settings (rememberPossiblyImpureFunctionValues: true).
If rememberPossiblyImpureFunctionValues is disabled, the change would always result in the evaluated return value being narrower than intended, never wider. In most cases this would not be noticed, in some rare cases it might give an error which wasn't there before, which would then need to be fixed with @phpstan-impure on the offending method, which is precisely what you should do if you create a function on purpose that looks and feels like a getter.
The intent would be to, in time, change the default value for rememberPossiblyImpureFunctionValues, as this test will likely mark 99.9+% of all getters correctly as having no side effects. This would fix the BC breaking surprises we ran into with 1.10.41 at phpstan/phpstan#10215, which are likely to occur more often.
So I see it as accepting a very minor and unlikely to occur BC break to avoid accidentally breaking valid code more often as what happened with 1.10.41.
There was a problem hiding this comment.
It's a BC break because such code: https://phpstan.org/r/de31d2eb-e52a-440c-afcc-86746172cb79
would now be reported with: Call to method HelloWorld::getName() on a separate line has no effect.
Also I don't want to impose any coding style on any PHPStan user. Having a different behaviour for methods that start with get etc. is a coding style choice. People might have "getters" in the style of public function name().
There was a problem hiding this comment.
would now be reported with:
Call to method HelloWorld::getName() on a separate line has no effect.
And it would be correct 😄 I'd classify this as an improvement in dead code detection actually, and a likely bug in the evaluated code in that the result of the getter was not assigned or used.
I'll happily agree to disagree here, however I do feel the choice to default rememberPossiblyImpureFunctionValues to true is an unfortunate one, and one I would recommend reconsidering for 2.0, coupled with improved logic. Marking any method with parameters as impure by default, instead of "maybe", would also prevent a lot of issues, and makes logical sense.
| @@ -153,6 +153,8 @@ | |||
| 'DateTimeImmutable::getTimestamp' => ['hasSideEffects' => false], | |||
| 'DateTimeImmutable::getTimezone' => ['hasSideEffects' => false], | |||
|
|
|||
| 'Redis::get' => ['hasSideEffects' => true], | |||
There was a problem hiding this comment.
- Are there other Redis methods that behave the same way? I'd say yes, please provide a complete list. Alternatively you can also modify code in src/ to achieve the same result, along with some tests.
- This change isn't sufficient, you need to run a script in bin/ to regenerate resources/functionMetadata.php
Thanks.
|
I'm closing this PR as I don't feel it's the right way forward without the improved getter detection. We're now fixing symptoms, not problems. We should more fundamentally look into the 2nd and 4th bullet points at phpstan/phpstan#10215 (comment) |
Related to the discussion at phpstan/phpstan#10215
Redis::getexplicitly as impure (ie. non-repeatable)PhpMethodReflection::hasSideEffectsby using similar logic to Symfony PropertyInfo: if a function name starts with get/is/has/can and has no parameters it is a pure getter, unless explicitly marked@phpstan-impure.