Apply class purity tags to annotated methods - #6656
truongbo17 wants to merge 4 commits into
Conversation
4dfeee8 to
6b7a389
Compare
| * @phpstan-all-methods-pure | ||
| * @method int magic() |
There was a problem hiding this comment.
my gut feeling is, that magic behaviour and purity kind of contradict each other.
phpstan cannot give guarantes for a method it will not see/know and I think people will shoot into their own foot by doing impure stuff in such magic context without seeing it
There was a problem hiding this comment.
Now you point it out, it's indeed annoying that
- PHPStan won't report impurity in magic
- People cannot annotate a magic method as impure even when all are annotated as pure (which is doable with a well defined method).
VincentLanglet
left a comment
There was a problem hiding this comment.
With your change, do we still report the following example
https://phpstan.org/r/fb9d4b25-5cbf-4335-b566-11fdc26a1f0d
?
|
Thanks for the counterexample. The previous revision suppressed that diagnostic. I updated the annotated method reflection so a native The regression test covers both instance and static dispatchers, while the original issue case (no impure dispatcher) remains pure. The focused purity and annotation tests pass locally. |
| $isPure = null; | ||
| if ($classResolvedPhpDoc !== null && $classResolvedPhpDoc->areAllMethodsPure()) { | ||
| $isPure = true; | ||
| } elseif ($classResolvedPhpDoc !== null && $classResolvedPhpDoc->areAllMethodsImpure()) { | ||
| $isPure = false; | ||
| } | ||
| if ($isPure === true && $nativeCallMethod !== null && $nativeCallMethod->isPure()->no()) { | ||
| $isPure = false; | ||
| } |
There was a problem hiding this comment.
I'm not sure about the order of those condition.
As soon as nativeCallMethod is not null shouldn't we use the purity from it ?
There was a problem hiding this comment.
Agreed. I changed the precedence in 70fe6a2: when a native __call() or __callStatic() exists, the annotated method now takes its yes/no/maybe purity from that dispatcher. The class-wide tag is used only when there is no native dispatcher. I added a regression case where an explicitly pure __call() overrides @phpstan-all-methods-impure, alongside the impure dispatcher cases. The focused tests and PHPStan analysis pass locally.
Summary
@methoddeclarations currently have unknown purity even when their class is marked@phpstan-all-methods-pureor@phpstan-all-methods-impure. This makes a call to a pure annotated method report a possibly impure call from pure code.Propagate the class-level purity tag when creating an annotation method reflection and use it for both
isPure()andhasSideEffects(). Void-returning methods keep their existing side-effect behavior, matching native methods.Closes phpstan/phpstan#15322
Tests
php vendor/bin/phpunit tests/PHPStan/Reflection/Annotations/AnnotationsMethodsClassReflectionExtensionTest.phpphp vendor/bin/phpunit tests/PHPStan/Rules/Pure/PureMethodRuleTest.phpphp -d memory_limit=450M bin/phpstan analyse src/Reflection/Annotations/AnnotationMethodReflection.php src/Reflection/Annotations/AnnotationsMethodsClassReflectionExtension.php tests/PHPStan/Reflection/Annotations/AnnotationsMethodsClassReflectionExtensionTest.php tests/PHPStan/Rules/Pure/PureMethodRuleTest.php --no-progress --debugphp vendor/bin/parallel-linton the six changed PHP files