Skip to content

Preserve a declared never native return type on magic methods instead of overriding it - #6209

Closed
phpstan-bot wants to merge 4 commits into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-thg5km0
Closed

phpstan-bot wants to merge 4 commits into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-thg5km0

Conversation

@phpstan-bot

Copy link
Copy Markdown
Collaborator

Summary

__clone(): never was reported with Method 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 the never written in the code was invisible to every rule that reads the method's return type.

PHP always accepts never as the return type of a magic method (zend_check_magic_method_return_type() returns early for MAY_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 is never. This covers __clone, __destruct, __unset, __wakeup, __toString, __isset, __sleep, __set, __unserialize, __serialize, __set_state and __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 add never was advice that cannot be followed.

Analogous cases probed:

  • __set_state() and __debugInfo() were already correct — they build their type with TypeCombinator::intersect(..., $realReturnType), which collapses to never on 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.
  • Property hooks (get/set) and plain functions/closures/arrow functions have no mandated return type, so there is nothing to override there. Verified no false positive from MethodNeverRule/FunctionNeverRule.
  • ReturnTypeRule (src/Rules/Methods/ReturnTypeRule.php) and MissingReturnRule (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 $realReturnType for every magic method it knows about, without looking at what was actually declared. Since PhpMethodFromParserNodeReflection is the reflection used for the method the analyser is currently inside, every rule that works off $scope->getFunction()->getReturnType() saw void/string/bool/array instead of never:

  • PHPStan\Rules\Playground\MethodNeverRule — false positive "always throws an exception, it should have return type never" on 11 magic methods that already declared never.
  • PHPStan\Rules\Methods\ReturnTypeRule — false negative: return;, return [];, return 'foo';, return true; inside a magic method declared never were all accepted.
  • PHPStan\Rules\Missing\MissingReturnRule — wrong message: an empty-bodied __toString(): never was 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 with PhpMethodReflection, 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 add never, which PHP rejects at compile time. MethodNeverRule now skips those two.

Test

  • tests/PHPStan/Rules/Playground/data/method-never.php + MethodNeverRuleTest — a MagicMethods class declaring never on __clone, __toString, __isset, __set, __unset, __sleep, __wakeup, __serialize, __unserialize, __set_state and __debugInfo plus an always-throwing __construct/__destruct (all expected to be silent), and a MagicMethodsWithoutNever class with __clone(): void / __toString(): string that 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 declared never must 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 declared never with 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 __clone reported nothing.

Each test was confirmed to fail with the source change stashed. make tests, make phpstan and make cs-fix are green.

Fixes phpstan/phpstan#15070

@phpstan-bot

Copy link
Copy Markdown
Collaborator Author

The review is addressed — the gate is on the branch and pushed (e139b542d), with no local changes left over.

What changed

PhpVersion::supportsNeverReturnType() (>= 80100) was added, PhpMethodFromParserNodeReflection now takes a PhpVersion (threaded from MutatingScope at both enterClassMethod() and enterPropertyHook() call sites), and the skip condition became:

if (!$phpVersion->supportsNeverReturnType() || !$realReturnType instanceof NeverType) {

Plus MethodNeverRulePhp80Test with a phpVersion: 80000 config, asserting the mandated types still win there.

One thing worth knowing about that gate

While working on this I checked whether the branch could ever be reached with never on a pre-8.1 target, and it can't — the gate is redundant in practice:

8.0: node=PhpParser\Node\Name       type=PHPStan\Type\ObjectType             isNeverType=false
8.1: node=PhpParser\Node\Identifier type=PHPStan\Type\NonAcceptingNeverType  isNeverType=true

never is a builtin type in php-parser only from 8.1 (PhpVersion.php:26, 'never' => 80100, consumed by ParserAbstract::handleBuiltinTypes()), and PHPStan builds its parser from the configured version (src/Parser/PhpParserFactory.php:20). Below 8.1 the return type arrives as a Name and ParserNodeTypeToPHPStanType resolves it to ObjectType('never'), so instanceof NeverType is already false. For a phpVersion: {min, max} range, PhpVersionFactoryFactory uses min, so the parser and supportsNeverReturnType() read the same number — they can't disagree.

I confirmed the new PHP 8.0 test is version-sensitive rather than vacuous: flipping its neon to 80100 makes it fail with should never return but return statement found instead of the mandated-type messages. So it does lock in the 8.0 behaviour either way.

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 @api constructor signature change for it.

make tests (21257 tests, 97 skipped), make phpstan and make cs-fix are all green on the pushed branch state.

phpstan-bot and others added 4 commits September 19, 2026 14:09
…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>
@VincentLanglet
VincentLanglet force-pushed the create-pull-request/patch-thg5km0 branch from 3c0e6ca to 962b8ba Compare September 19, 2026 12:09
@VincentLanglet

Copy link
Copy Markdown
Contributor

I think this is ready @staabm

@VincentLanglet VincentLanglet self-assigned this Sep 20, 2026
@staabm
staabm deleted the create-pull-request/patch-thg5km0 branch October 14, 2026 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

False positive: Always throwing and returning never

3 participants