Repository navigation
Preserve a declared never native return type on magic methods instead of overriding it - #6209
phpstan-bot wants to merge 4 commits into
Conversation
|
The review is addressed — the gate is on the branch and pushed ( What changed
if (!$phpVersion->supportsNeverReturnType() || !$realReturnType instanceof NeverType) {Plus One thing worth knowing about that gateWhile working on this I checked whether the branch could ever be reached with
I confirmed the new PHP 8.0 test is version-sensitive rather than vacuous: flipping its neon to The explicit check is harmless and self-documenting, so I left it in as requested — just flagging that it's belt-and-braces over a parser-level guarantee, in case you'd rather not pay the
|
…ad of overriding it * `PhpMethodFromParserNodeReflection` unconditionally replaced the declared native return type of magic methods with the type PHP mandates (`void` for `__clone`/`__destruct`/`__unset`/`__wakeup`/`__set`/`__unserialize`, `string` for `__toString`, `bool` for `__isset`, `array<int, string>` for `__sleep`, `array` for `__serialize`). PHP however always accepts `never` on a magic method, so the declared `never` is now kept. * `MethodNeverRule` no longer reports `__construct()` and `__destruct()` - PHP does not allow declaring a return type on them at all, so the suggested fix was impossible to apply. * This also fixes `ReturnTypeRule` (a `return` statement inside a magic method declared `never` was silently accepted) and `MissingReturnRule` (an empty-bodied `__toString(): never` reported "should return string but return statement is missing"). * Probed and found already correct: `__set_state()` and `__debugInfo()` (they combine via `TypeCombinator::intersect()`, which already collapses to `never`), `PhpMethodReflection` (only applies the mandated type when no native return type is declared), property hooks and plain functions (no mandated return type).
`never` is only a valid return type since PHP 8.1, so the branch that keeps a declared `never` instead of the return type PHP mandates for a magic method is now guarded by a new PhpVersion::supportsNeverReturnType(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3c0e6ca to
962b8ba
Compare
|
I think this is ready @staabm |
Summary
__clone(): neverwas reported withMethod X::__clone() always throws an exception, it should have return type "never".even though it already had that return type. PHPStan replaced the declared native return type of magic methods with the type PHP mandates for them, so theneverwritten in the code was invisible to every rule that reads the method's return type.PHP always accepts
neveras the return type of a magic method (zend_check_magic_method_return_type()returns early forMAY_BE_NEVER), so the declared type must be kept.Changes
src/Reflection/Php/PhpMethodFromParserNodeReflection.php— the whole block of magic-method return type overrides is now skipped when the declared native return type isnever. This covers__clone,__destruct,__unset,__wakeup,__toString,__isset,__sleep,__set,__unserialize,__serialize,__set_stateand__debugInfo.src/Rules/Playground/MethodNeverRule.php— skip__construct()and__destruct(). PHP does not allow declaring a return type on them at all (Method X::__construct() cannot declare a return type), so telling the user to addneverwas advice that cannot be followed.Analogous cases probed:
__set_state()and__debugInfo()were already correct — they build their type withTypeCombinator::intersect(..., $realReturnType), which collapses toneveron its own. They are now handled by the same branch for consistency.PhpMethodReflection(BetterReflection-backed methods, i.e. classes outside the analysed file) was already correct — it only applies the mandated type when no native return type is declared.get/set) and plain functions/closures/arrow functions have no mandated return type, so there is nothing to override there. Verified no false positive fromMethodNeverRule/FunctionNeverRule.ReturnTypeRule(src/Rules/Methods/ReturnTypeRule.php) andMissingReturnRule(src/Rules/Missing/MissingReturnRule.php) were silently affected by the same root cause and are fixed by the same change — no rule-side change was needed, but both got regression tests.Root cause
The pattern is "a mandated magic-method return type overwrites the declared one".
PhpMethodFromParserNodeReflection::__construct()rewrote$realReturnTypefor every magic method it knows about, without looking at what was actually declared. SincePhpMethodFromParserNodeReflectionis the reflection used for the method the analyser is currently inside, every rule that works off$scope->getFunction()->getReturnType()sawvoid/string/bool/arrayinstead ofnever:PHPStan\Rules\Playground\MethodNeverRule— false positive "always throws an exception, it should have return typenever" on 11 magic methods that already declarednever.PHPStan\Rules\Methods\ReturnTypeRule— false negative:return;,return [];,return 'foo';,return true;inside a magic method declaredneverwere all accepted.PHPStan\Rules\Missing\MissingReturnRule— wrong message: an empty-bodied__toString(): neverwas reported as "should return string but return statement is missing" instead of "should always throw an exception or terminate script execution but doesn't do that".The fix keeps the declared type whenever it is
never, which restores agreement withPhpMethodReflection, the reflection used for the same class when it is not the file being analysed.A second, independent false positive on the same rule:
__construct()/__destruct()bodies that always throw were told to addnever, which PHP rejects at compile time.MethodNeverRulenow skips those two.Test
tests/PHPStan/Rules/Playground/data/method-never.php+MethodNeverRuleTest— aMagicMethodsclass declaringneveron__clone,__toString,__isset,__set,__unset,__sleep,__wakeup,__serialize,__unserialize,__set_stateand__debugInfoplus an always-throwing__construct/__destruct(all expected to be silent), and aMagicMethodsWithoutNeverclass with__clone(): void/__toString(): stringthat still gets reported. Without the fix this produced 11 extra errors.tests/PHPStan/Rules/Methods/data/never-magic-method-return-type.php+ReturnTypeRuleTest::testNeverMagicMethodReturnType()— return statements inside magic methods declarednevermust be reported. Without the fix, zero of the five errors were reported.tests/PHPStan/Rules/Missing/data/never-magic-method.php+MissingReturnRuleTest::testNeverMagicMethod()— magic methods declaredneverwith a body that can fall through must report "should always throw an exception or terminate script execution". Without the fix, three of them reported the wrong mandated type and__clonereported nothing.Each test was confirmed to fail with the source change stashed.
make tests,make phpstanandmake cs-fixare green.Fixes phpstan/phpstan#15070