Resolve conditional @throws types like ($x is 0 ? Exception : void) at call sites and throw points#5874
Conversation
…)` at call sites and throw points - Add `ConditionalThrowTypeResolver` that resolves `ConditionalTypeForParameter` inside a `@throws` type, either against the arguments passed at a call site (`resolveForCall`) or against the parameter variables narrowed in a throw point's scope (`resolveForScope`). - Resolve conditional throws types when computing throw points for function calls (`FuncCallHandler`), method/static calls (`MethodThrowPointHelper`) and constructor calls (`NewHandler`), so callers only see the exception when the arguments actually trigger the throwing branch. - Resolve conditional throws types per throw point in `MissingCheckedExceptionInThrowsCheck` (used by the function, method and property-hook rules) so a function body that throws only in a branch is matched against its own conditional `@throws` declaration. - Accept conditional throws types in `InvalidThrowsPhpDocValueRule` as long as every branch is a valid throws type (a `Throwable` subtype or `void`); plain `void` is still only allowed standalone or inside a conditional branch, so `Throwable|void` remains invalid.
|
Is it correctly remapped when a parameter name changes during PHPDoc inheritance? |
VincentLanglet
left a comment
There was a problem hiding this comment.
There is ConditionalTypeForParameter and ConditionalType with appropriate PhpDocParser Node
Wouldn't it better to introduce a
PHPStan\PhpDocParser\Ast\Type\ConditionalTypeForThrowsNode
|
I've now fully traced the flow and can address the review accurately. The reviewer's suggestion concerns the PhpDocParser AST layer, which lives in a separate package — let me lay out the analysis rather than fabricate a cross-repo change. Response to the reviewThanks @VincentLanglet — I looked into whether a dedicated The phpdoc type parser is context-free. private function parseThrowsTagValue(TokenIterator $tokens): Ast\PhpDoc\ThrowsTagValueNode
{
$type = $this->typeParser->parse($tokens);
...
}So What we'd gain vs. duplicate. The only genuinely throws-specific wrinkle today is that My recommendation: keep the Separately, on @ondrejmirtes's earlier question about parameter-name remapping during PHPDoc inheritance — that's a real concern with the current I did not make code changes or commits for this review point, because the requested change ( Two things I can act on next if you confirm:
Want me to proceed with (1), (2), or both? |
VincentLanglet
left a comment
There was a problem hiding this comment.
I think this should support ConditionalType too since the method could have a phpdoc like
@template TKey of int|string
@throws (TKey is int ? void : Exception)
| * @param int $x | ||
| * @throws ($x is 0 ? Exception : void) | ||
| */ | ||
| public function inverse(int $x): float |
There was a problem hiding this comment.
Add a test with
class Service2 extends Service
{
public function inverse(int $y): float
}
To ensure the type is correctly resolved even if the method is overriden with a different name for the param.
There was a problem hiding this comment.
Done. Here's a summary of how I addressed the review.
Review request
VincentLanglet asked to add a test where Service2 extends Service overrides inverse() with a differently-named parameter ($y instead of $x), to ensure the conditional @throws type is still resolved correctly through inheritance. This is the same parameter-name remapping concern ondrejmirtes raised earlier.
What I found
Adding that test exposed a real bug. When a method inherits a parent's @throws PHPDoc, ResolvedPhpDocBlock::mergeThrowsTags() returned the parent's throws tag verbatim, without remapping parameter names — unlike mergeReturnTags(), which applies transformConditionalReturnTypeWithParameterNameMapping(). So a conditional throws like ($x is 0 ? Exception : void) inherited into inverse(int $y) still referenced $x, which:
- made the overriding method's body wrongly report a missing
@throws(the conditional couldn't resolve against the narrowed$y), and - broke call-site precision (
$service2->inverse(0)not flagged, while a non-zero call could be flagged).
I confirmed the failure by stashing the fix and watching the test fail for exactly these reasons before re-applying it.
The fix
mergeThrowsTags() now takes the InheritedPhpDocParameterMapping and applies the same ConditionalTypeForParameter parameter-name remapping already used for inherited @return types — keeping a single shared resolution path rather than forking one.
Tests
Added Service2 extends Service with inverse(int $y) (renamed param, no own PHPDoc) plus two callers to conditional-throws-method.php. inheritedMethodCallZero() is now correctly reported as missing the exception; inheritedMethodCallNonZero() and the overriding body are clean.
I also resolved a rebase conflict with a concurrently-pushed commit (which added the template-based lookup() case for VincentLanglet's other review point), merging both sets of cases and fixing up the shifted line numbers.
make phpstan is green, and the exceptions + PhpDoc test suites pass. Commit a9304c028 is pushed.
| } | ||
|
|
||
| if ($this->isThrowsValid($phpDocThrowsType)) { | ||
| if ($this->isThrowsValid($phpDocThrowsType, false)) { |
There was a problem hiding this comment.
The only place false is passed is here.
What about moving the
if ($phpDocThrowsType->isVoid()->yes()) {
return [];
}
check inside the method isThrowsValid and removing the voidAllowed param ?
There was a problem hiding this comment.
Done. I addressed VincentLanglet's review comment on InvalidThrowsPhpDocValueRule.php.
What the reviewer asked
Move the if ($phpDocThrowsType->isVoid()->yes()) { return []; } check inside isThrowsValid() and remove the voidAllowed parameter, since false was only ever passed at the single top-level call.
What I changed
- Removed the standalone void early-return from
processNode()and thefalseargument; the call is now just$this->isThrowsValid($phpDocThrowsType). - Folded void handling into
isThrowsValid(): a standalonevoid(or a conditional branch that isvoid) now returnstrueat the top of the method, and thevoidAllowedparameter is gone. - Preserved the
Throwable|voidrejection: there was a subtlety the literal suggestion would have broken —Throwable|voidandstdClass|voidmust stay invalid (asserted at lines 96/103 of the test). Ifvoidreturnedtrueuniversally, a union containing it would wrongly become valid. So theUnionTypebranch now explicitly rejects avoidmember ($innerType->isVoid()->yes() || !$this->isThrowsValid($innerType)).
This keeps the behavior identical: standalone void valid, void inside conditional branches valid, Throwable|void invalid, invalid conditional branches still reported.
Verification
InvalidThrowsPhpDocValueRuleTest— 7/7 pass- The two
MissingCheckedException...ThrowsRuleTestsuites — 6/6 pass make phpstan— no errors
I rebased onto the meanwhile-pushed commit a9304c028 ("Remap parameter names in inherited conditional @throws types", which handles ondrejmirtes's earlier inheritance concern), re-ran the test green, and pushed as dc9159faf.
… ? void : Exception)`
Conditional `@throws` resolution previously only handled
`ConditionalTypeForParameter` (subject is a `$param` variable). A conditional
whose subject is a template type produces a `ConditionalType` instead, which was
never resolved against the inferred template type map.
`ConditionalThrowTypeResolver::resolveForCall()` now also resolves template
types via the call-site `resolvedTemplateTypeMap`/`callSiteVarianceMap` when the
parameters acceptor is an `ExtendedParametersAcceptor`, so callers see the
precise branch (e.g. `lookup(1)` does not throw, `lookup('foo')` throws). Inside
the function body, where the template is not bound to a concrete type,
`resolveForScope()` collapses the unresolvable conditional to the union of its
branches so the body's throw points are matched against the broadest declared
set instead of a `Maybe`-certain conditional (which caused a false positive).
`InvalidThrowsPhpDocValueRule` already recurses into `ConditionalType` branches,
so template-based conditional throws validation works (and still rejects an
invalid branch like `(TKey is int ? void : stdClass)`).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
I've addressed the review. Here's a summary of what I did. Review pointVincentLanglet asked that the resolver also support @template TKey of int|string
@throws (TKey is int ? void : Exception)This was a real gap: the resolver only handled Changes
Tests
One note I'll flag honestly: ondrejmirtes's earlier question about parameter-name remapping during PHPDoc inheritance is still orthogonal and not addressed here — happy to take that as a separate change if you'd like. |
When a method overrides a parent method using a different parameter name and inherits the parent's @throws PHPDoc, a conditional throws type like ($x is 0 ? Exception : void) referenced the parent's parameter name. Apply the same parameter-name remapping used for inherited @return types so the conditional resolves against the overriding method's parameters both at the throw points in the body and at call sites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The void-acceptance check previously lived both in processNode (for a standalone `@throws void`) and as a `voidAllowed` flag threaded through isThrowsValid. Since `voidAllowed` was only ever false at the single top-level call, fold the standalone-void handling into isThrowsValid and reject `void` only when it appears as a union member (e.g. Throwable|void). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
PHPStan already supports conditional return types such as
($x is 0 ? never : float), but conditional@throwstypes were not supported: a@throws ($x is 0 ? Exception : void)declaration was rejected byInvalidThrowsPhpDocValueRuleas "not subtype of Throwable", the function body was wrongly reported as missing the exception in its@throws, and callers never benefited from the per-argument precision.This change makes conditional
@throwstypes first-class:inverse(0)) is reported as missing the exception in its own@throws, while a caller that does not (e.g.inverse(7), orinverse($x)with$xof typeint<3, 5>) is not.@throws, using the parameter narrowing at each throw point.Changes
src/Analyser/ConditionalThrowTypeResolver.php(new): resolvesConditionalTypeForParameterinside a throws type.resolveForCall()builds the parameter→argument-type map from a call's arguments (named, positional, variadic and default values) and converts the conditional viatoConditional(), then resolves late-resolvable types.resolveForScope()resolves the conditional against the parameter variables narrowed in a given scope (used inside the function body).src/Analyser/ExprHandler/FuncCallHandler.php,src/Analyser/ExprHandler/Helper/MethodThrowPointHelper.php,src/Analyser/ExprHandler/NewHandler.php: resolve the reflection's throws type withresolveForCall()before turning it into a throw point, so function calls, method calls, static-method calls andnewall honour conditional throws.src/Rules/Exceptions/MissingCheckedExceptionInThrowsCheck.php: resolve the declared throws type per throw point withresolveForScope()(this check backs the function, method and property-hook missing-throws rules).src/Rules/PhpDoc/InvalidThrowsPhpDocValueRule.php: recurse intoConditionalType/ConditionalTypeForParameterbranches;voidis accepted only standalone or inside a conditional branch (soThrowable|voidstays invalid).All resolution paths short-circuit on
!$throwType->hasTemplateOrLateResolvableType(), so ordinary throws types are unaffected.Root cause
A conditional
@throwstype is resolved into the sameConditionalTypeForParameterrepresentation as a conditional return type, but nothing ever resolved it: throws types live on the function/method reflection and are read raw (getThrowType()), bypassing the call-siteResolvedFunctionVariantmachinery that resolves conditional return types. As a result the conditional always collapsed to theMaybe-certain union of its branches (Exception|void), which is neither a cleanThrowable(validation failure) nor a precise per-call result (no caller precision, false "missing" on the body).The fix mirrors the conditional-return-type resolution for throws: at every place the raw throws type is consumed, the
ConditionalTypeForParameteris resolved against the relevant subject — the call arguments at call sites, the narrowed parameter variables at throw points.Test
tests/PHPStan/Rules/PhpDoc/InvalidThrowsPhpDocValueRuleTest: added valid conditional throws (($x is 0 ? Exception : void),($x is 0 ? Exception : RuntimeException)) which produce no error, and an invalid-branch case (($x is 0 ? stdClass : void)) which is still reported.tests/PHPStan/Rules/Exceptions/MissingCheckedExceptionInFunctionThrowsRuleTest::testConditionalThrows: a conditional-throwinginverse()function and callers —inverse(0)andinverse($x:int)are reported as missing the exception, whileinverse(7)andinverse($x:int<3,5>)are not, and the body ofinverse()itself is clean.tests/PHPStan/Rules/Exceptions/MissingCheckedExceptionInMethodThrowsRuleTest::testConditionalThrows: the same behaviour for instance method calls, static method calls and constructor (new) calls (the analogous parallel constructs).Fixes phpstan/phpstan#7906