From f8c5ebcbce8155fe5093191f18bb0fdd10cd202d Mon Sep 17 00:00:00 2001 From: Peter Potrowl Date: Mon, 24 Aug 2026 15:13:44 +0200 Subject: [PATCH] Avoid dead catch false positive for inconsistently overridden trait methods A trait's try/catch can be dead in the context of one class using the trait and alive in another, e.g. when it depends on whether an abstract method gets overridden without throwing. Apply the same ConstantConditionInTraitHelper mechanism already used for isset/empty/?? to CatchWithUnthrownExceptionRule, so disagreeing verdicts across classes using the trait suppress the error instead of reporting it. Closes phpstan/phpstan#10315 --- src/Analyser/StmtHandler/TryCatchHandler.php | 8 ++ .../CatchWithThrownExceptionInTraitNode.php | 51 ++++++++++ .../ConstantConditionInTraitHelper.php | 43 +++++++- .../CatchWithThrownExceptionInTraitRule.php | 45 +++++++++ .../CatchWithUnthrownExceptionRule.php | 69 ++++++++----- src/Rules/Exceptions/DeadCatchInTraitKey.php | 26 +++++ .../AbilityToDisableImplicitThrowsTest.php | 52 ++++++++-- ...atchWithUnthrownExceptionRuleStubsTest.php | 3 +- .../CatchWithUnthrownExceptionRuleTest.php | 40 ++++++-- .../Rules/Exceptions/data/bug-10315.php | 54 +++++++++++ .../Exceptions/data/dead-catch-in-trait.php | 97 +++++++++++++++++++ 11 files changed, 441 insertions(+), 47 deletions(-) create mode 100644 src/Node/CatchWithThrownExceptionInTraitNode.php create mode 100644 src/Rules/Exceptions/CatchWithThrownExceptionInTraitRule.php create mode 100644 src/Rules/Exceptions/DeadCatchInTraitKey.php create mode 100644 tests/PHPStan/Rules/Exceptions/data/bug-10315.php create mode 100644 tests/PHPStan/Rules/Exceptions/data/dead-catch-in-trait.php diff --git a/src/Analyser/StmtHandler/TryCatchHandler.php b/src/Analyser/StmtHandler/TryCatchHandler.php index ac4612abc84..951e119fa11 100644 --- a/src/Analyser/StmtHandler/TryCatchHandler.php +++ b/src/Analyser/StmtHandler/TryCatchHandler.php @@ -13,6 +13,7 @@ use PHPStan\Analyser\StatementContext; use PHPStan\Analyser\StmtHandler; use PHPStan\DependencyInjection\AutowiredService; +use PHPStan\Node\CatchWithThrownExceptionInTraitNode; use PHPStan\Node\CatchWithUnthrownExceptionNode; use PHPStan\Node\Expr\TypeExpr; use PHPStan\Node\FinallyExitPointsNode; @@ -169,6 +170,13 @@ public function processStmt( // emit error foreach ($matchingCatchTypes as $catchTypeIndex => $matched) { if ($matched) { + // A trait's catch can be dead in the context of one class using the + // trait and alive in the context of another, so the alive ones have + // to be reported there as well for the disagreement to be noticed. + if ($scope->isInTrait()) { + $nodeScopeResolver->callNodeCallback($nodeCallback, new CatchWithThrownExceptionInTraitNode($catchNode, $originalCatchTypes[$catchTypeIndex]), $scope, $storage); + } + continue; } $nodeScopeResolver->callNodeCallback($nodeCallback, new CatchWithUnthrownExceptionNode($catchNode, $catchTypes[$catchTypeIndex], $originalCatchTypes[$catchTypeIndex]), $scope, $storage); diff --git a/src/Node/CatchWithThrownExceptionInTraitNode.php b/src/Node/CatchWithThrownExceptionInTraitNode.php new file mode 100644 index 00000000000..3723fab4ee1 --- /dev/null +++ b/src/Node/CatchWithThrownExceptionInTraitNode.php @@ -0,0 +1,51 @@ +getAttributes()); + } + + public function getOriginalNode(): Catch_ + { + return $this->originalNode; + } + + public function getOriginalCaughtType(): Type + { + return $this->originalCaughtType; + } + + #[Override] + public function getType(): string + { + return 'PHPStan_Node_CatchWithThrownExceptionInTraitNode'; + } + + /** + * @return string[] + */ + #[Override] + public function getSubNodeNames(): array + { + return []; + } + +} diff --git a/src/Rules/Comparison/ConstantConditionInTraitHelper.php b/src/Rules/Comparison/ConstantConditionInTraitHelper.php index 145c48919c1..f8e05530489 100644 --- a/src/Rules/Comparison/ConstantConditionInTraitHelper.php +++ b/src/Rules/Comparison/ConstantConditionInTraitHelper.php @@ -40,6 +40,35 @@ public function emitNoError( Scope&NodeCallbackInvoker&CollectedDataEmitter $scope, Expr $expr, ): void + { + $this->emitNoErrorForKey($ruleName, $scope, $this->exprString($expr)); + } + + /** + * @param class-string> $ruleName + */ + public function emitError( + string $ruleName, + Scope&NodeCallbackInvoker&CollectedDataEmitter $scope, + Expr $expr, + bool $value, + RuleError $ruleError, + ): void + { + $this->emitErrorForKey($ruleName, $scope, $expr, $this->exprString($expr), $value, $ruleError); + } + + /** + * Like emitNoError(), but for callers that cannot key their check by a single Expr + * (e.g. one Rule node covering several distinct checks at the same location). + * + * @param class-string> $ruleName + */ + public function emitNoErrorForKey( + string $ruleName, + Scope&NodeCallbackInvoker&CollectedDataEmitter $scope, + string $key, + ): void { if (!$scope->isInTrait()) { return; @@ -48,18 +77,22 @@ public function emitNoError( $scope->emitCollectedData(ConstantConditionInTraitCollector::class, [ $ruleName, $scope->getTraitReflection()->getName(), - $this->exprString($expr), + $key, null, ]); } /** + * Like emitError(), but for callers that cannot key their check by a single Expr + * (e.g. one Rule node covering several distinct checks at the same location). + * * @param class-string> $ruleName */ - public function emitError( + public function emitErrorForKey( string $ruleName, Scope&NodeCallbackInvoker&CollectedDataEmitter $scope, - Expr $expr, + Node $node, + string $key, bool $value, RuleError $ruleError, ): void @@ -75,9 +108,9 @@ public function emitError( $scope->emitCollectedData(ConstantConditionInTraitCollector::class, [ $ruleName, $scope->getTraitReflection()->getName(), - $this->exprString($expr), + $key, $value, - $this->ruleErrorTransformer->transform($ruleError, $scope, [], $expr), + $this->ruleErrorTransformer->transform($ruleError, $scope, [], $node), ]); } diff --git a/src/Rules/Exceptions/CatchWithThrownExceptionInTraitRule.php b/src/Rules/Exceptions/CatchWithThrownExceptionInTraitRule.php new file mode 100644 index 00000000000..21f983d73ba --- /dev/null +++ b/src/Rules/Exceptions/CatchWithThrownExceptionInTraitRule.php @@ -0,0 +1,45 @@ + + */ +#[RegisteredRule(level: 4)] +final class CatchWithThrownExceptionInTraitRule implements Rule +{ + + public function __construct(private ConstantConditionInTraitHelper $constantConditionInTraitHelper) + { + } + + public function getNodeType(): string + { + return CatchWithThrownExceptionInTraitNode::class; + } + + public function processNode(Node $node, Scope&NodeCallbackInvoker&CollectedDataEmitter $scope): array + { + $this->constantConditionInTraitHelper->emitNoErrorForKey( + CatchWithUnthrownExceptionRule::class, + $scope, + DeadCatchInTraitKey::create($node->getOriginalNode(), $node->getOriginalCaughtType()), + ); + + return []; + } + +} diff --git a/src/Rules/Exceptions/CatchWithUnthrownExceptionRule.php b/src/Rules/Exceptions/CatchWithUnthrownExceptionRule.php index f4e2479a2ad..4bd0dc9e53f 100644 --- a/src/Rules/Exceptions/CatchWithUnthrownExceptionRule.php +++ b/src/Rules/Exceptions/CatchWithUnthrownExceptionRule.php @@ -3,10 +3,13 @@ namespace PHPStan\Rules\Exceptions; use PhpParser\Node; +use PHPStan\Analyser\CollectedDataEmitter; +use PHPStan\Analyser\NodeCallbackInvoker; use PHPStan\Analyser\Scope; use PHPStan\DependencyInjection\AutowiredParameter; use PHPStan\DependencyInjection\RegisteredRule; use PHPStan\Node\CatchWithUnthrownExceptionNode; +use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\NeverType; @@ -25,6 +28,7 @@ public function __construct( private ExceptionTypeResolver $exceptionTypeResolver, #[AutowiredParameter(ref: '%exceptions.reportUncheckedExceptionDeadCatch%')] private bool $reportUncheckedExceptionDeadCatch, + private ConstantConditionInTraitHelper $constantConditionInTraitHelper, ) { } @@ -34,41 +38,56 @@ public function getNodeType(): string return CatchWithUnthrownExceptionNode::class; } - public function processNode(Node $node, Scope $scope): array + public function processNode(Node $node, Scope&NodeCallbackInvoker&CollectedDataEmitter $scope): array { if ($node->getCaughtType() instanceof NeverType) { - return [ - RuleErrorBuilder::message( - sprintf('Dead catch - %s is already caught above.', $node->getOriginalCaughtType()->describe(VerbosityLevel::typeOnly())), - ) - ->line($node->getStartLine()) - ->identifier('catch.alreadyCaught') - ->build(), - ]; - } + $error = RuleErrorBuilder::message( + sprintf('Dead catch - %s is already caught above.', $node->getOriginalCaughtType()->describe(VerbosityLevel::typeOnly())), + ) + ->line($node->getStartLine()) + ->identifier('catch.alreadyCaught') + ->build(); + } else { + if (!$this->reportUncheckedExceptionDeadCatch) { + $isCheckedException = false; + foreach ($node->getCaughtType()->getObjectClassNames() as $objectClassName) { + if ($this->exceptionTypeResolver->isCheckedException($objectClassName, $scope)) { + $isCheckedException = true; + break; + } + } - if (!$this->reportUncheckedExceptionDeadCatch) { - $isCheckedException = false; - foreach ($node->getCaughtType()->getObjectClassNames() as $objectClassName) { - if ($this->exceptionTypeResolver->isCheckedException($objectClassName, $scope)) { - $isCheckedException = true; - break; + if (!$isCheckedException) { + return []; } } - if (!$isCheckedException) { - return []; - } - } - - return [ - RuleErrorBuilder::message( + $error = RuleErrorBuilder::message( sprintf('Dead catch - %s is never thrown in the try block.', $node->getCaughtType()->describe(VerbosityLevel::typeOnly())), ) ->line($node->getStartLine()) ->identifier('catch.neverThrown') - ->build(), - ]; + ->build(); + } + + if ($scope->isInTrait()) { + // A trait's catch can be dead in the context of one class using the trait and + // alive in the context of another, e.g. when it depends on whether an abstract + // method gets overridden. Let the collector compare the verdicts of all the + // classes using the trait instead of reporting right away; the alive ones are + // recorded by CatchWithThrownExceptionInTraitRule under the same key. + $this->constantConditionInTraitHelper->emitErrorForKey( + self::class, + $scope, + $node->getOriginalNode(), + DeadCatchInTraitKey::create($node->getOriginalNode(), $node->getOriginalCaughtType()), + true, + $error, + ); + return []; + } + + return [$error]; } } diff --git a/src/Rules/Exceptions/DeadCatchInTraitKey.php b/src/Rules/Exceptions/DeadCatchInTraitKey.php new file mode 100644 index 00000000000..c61f2bd7fb2 --- /dev/null +++ b/src/Rules/Exceptions/DeadCatchInTraitKey.php @@ -0,0 +1,26 @@ +getObjectClassNames()), $catchNode->getStartLine()); + } + +} diff --git a/tests/PHPStan/Rules/Exceptions/AbilityToDisableImplicitThrowsTest.php b/tests/PHPStan/Rules/Exceptions/AbilityToDisableImplicitThrowsTest.php index 7c7bda75143..c9e082c7578 100644 --- a/tests/PHPStan/Rules/Exceptions/AbilityToDisableImplicitThrowsTest.php +++ b/tests/PHPStan/Rules/Exceptions/AbilityToDisableImplicitThrowsTest.php @@ -2,26 +2,40 @@ namespace PHPStan\Rules\Exceptions; +use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper; +use PHPStan\Rules\Comparison\ConstantConditionInTraitRule; use PHPStan\Rules\Rule; +use PHPStan\Testing\CompositeRule; use PHPStan\Testing\RuleTestCase; use PHPUnit\Framework\Attributes\RequiresPhp; use function array_merge; /** - * @extends RuleTestCase + * @extends RuleTestCase */ class AbilityToDisableImplicitThrowsTest extends RuleTestCase { protected function getRule(): Rule { - return new CatchWithUnthrownExceptionRule(new DefaultExceptionTypeResolver( - self::createReflectionProvider(), - [], - [], - [], - [], - ), true); + // @phpstan-ignore argument.type + return new CompositeRule([ + new CatchWithUnthrownExceptionRule( + new DefaultExceptionTypeResolver( + self::createReflectionProvider(), + [], + [], + [], + [], + ), + true, + self::getContainer()->getByType(ConstantConditionInTraitHelper::class), + ), + new CatchWithThrownExceptionInTraitRule( + self::getContainer()->getByType(ConstantConditionInTraitHelper::class), + ), + new ConstantConditionInTraitRule(), + ]); } public function testRule(): void @@ -97,6 +111,28 @@ public function testBug7799(): void ]); } + public function testBug10315(): void + { + $this->analyse([__DIR__ . '/data/bug-10315.php'], []); + } + + public function testDeadCatchInTrait(): void + { + $this->analyse([__DIR__ . '/data/dead-catch-in-trait.php'], [ + [ + // dead in both FirstUser and SecondUser: reported once, on the trait + 'Dead catch - DeadCatchInTrait\AlphaException is never thrown in the try block.', + 36, + ], + [ + // same catch as AlphaException on line 67, which is dead in ThrowsNeither + // but alive in ThrowsAlphaOnly and therefore not reported + 'Dead catch - DeadCatchInTrait\BetaException is never thrown in the try block.', + 67, + ], + ]); + } + public static function getAdditionalConfigFiles(): array { return array_merge( diff --git a/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleStubsTest.php b/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleStubsTest.php index 421fdb719d0..ea670a3dd0c 100644 --- a/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleStubsTest.php +++ b/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleStubsTest.php @@ -2,6 +2,7 @@ namespace PHPStan\Rules\Exceptions; +use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; @@ -19,7 +20,7 @@ protected function getRule(): Rule [], [], [], - ), true); + ), true, self::getContainer()->getByType(ConstantConditionInTraitHelper::class)); } public function testRule(): void diff --git a/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleTest.php b/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleTest.php index d77785142b5..ae2d0042633 100644 --- a/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleTest.php +++ b/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleTest.php @@ -4,12 +4,15 @@ use Error; use InvalidArgumentException; +use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper; +use PHPStan\Rules\Comparison\ConstantConditionInTraitRule; use PHPStan\Rules\Rule; +use PHPStan\Testing\CompositeRule; use PHPStan\Testing\RuleTestCase; use PHPUnit\Framework\Attributes\RequiresPhp; /** - * @extends RuleTestCase + * @extends RuleTestCase */ class CatchWithUnthrownExceptionRuleTest extends RuleTestCase { @@ -21,13 +24,24 @@ class CatchWithUnthrownExceptionRuleTest extends RuleTestCase protected function getRule(): Rule { - return new CatchWithUnthrownExceptionRule(new DefaultExceptionTypeResolver( - self::createReflectionProvider(), - [], - $this->uncheckedExceptionClasses, - [], - [], - ), $this->reportUncheckedExceptionDeadCatch); + // @phpstan-ignore argument.type + return new CompositeRule([ + new CatchWithUnthrownExceptionRule( + new DefaultExceptionTypeResolver( + self::createReflectionProvider(), + [], + $this->uncheckedExceptionClasses, + [], + [], + ), + $this->reportUncheckedExceptionDeadCatch, + self::getContainer()->getByType(ConstantConditionInTraitHelper::class), + ), + new CatchWithThrownExceptionInTraitRule( + self::getContainer()->getByType(ConstantConditionInTraitHelper::class), + ), + new ConstantConditionInTraitRule(), + ]); } public function testRule(): void @@ -842,4 +856,14 @@ public function testBug9826(): void $this->analyse([__DIR__ . '/data/bug-9826.php'], []); } + public function testBug10315(): void + { + $this->analyse([__DIR__ . '/data/bug-10315.php'], []); + } + + public function testDeadCatchInTrait(): void + { + $this->analyse([__DIR__ . '/data/dead-catch-in-trait.php'], []); + } + } diff --git a/tests/PHPStan/Rules/Exceptions/data/bug-10315.php b/tests/PHPStan/Rules/Exceptions/data/bug-10315.php new file mode 100644 index 00000000000..d29c0382ac7 --- /dev/null +++ b/tests/PHPStan/Rules/Exceptions/data/bug-10315.php @@ -0,0 +1,54 @@ +driverReadMultiple(); + } catch (PhpfastcacheUnsupportedMethodException $e) { + return []; + } + } + +} + +class Redis +{ + + use DriverPoolAbstractTrait, CacheItemPoolTrait; + + protected function driverReadMultiple(): array + { + return []; + } + +} + +class Memcached +{ + + use DriverPoolAbstractTrait, CacheItemPoolTrait; + +} diff --git a/tests/PHPStan/Rules/Exceptions/data/dead-catch-in-trait.php b/tests/PHPStan/Rules/Exceptions/data/dead-catch-in-trait.php new file mode 100644 index 00000000000..ccf72201279 --- /dev/null +++ b/tests/PHPStan/Rules/Exceptions/data/dead-catch-in-trait.php @@ -0,0 +1,97 @@ +nothingThrown(); + } catch (AlphaException $e) { + } + } + + protected function nothingThrown(): void + { + } + +} + +class FirstUser +{ + + use DeadInEveryUsingClassTrait; + +} + +class SecondUser +{ + + use DeadInEveryUsingClassTrait; + +} + +trait UnionCatchTrait +{ + + public function run(): void + { + try { + $this->work(); + } catch (AlphaException | BetaException $e) { + } + } + +} + +class ThrowsNeither +{ + + use ThrowingTrait, UnionCatchTrait; + + protected function work(): void + { + } + +} + +class ThrowsAlphaOnly +{ + + use ThrowingTrait, UnionCatchTrait; + + /** + * @throws AlphaException + */ + protected function work(): void + { + throw new AlphaException(); + } + +}