diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 53ac5a06f71..cb2efd64185 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -65,6 +65,7 @@ use PHPStan\File\FileHelper; use PHPStan\File\FileReader; use PHPStan\Node\BreaklessWhileLoopNode; +use PHPStan\Node\CatchWithThrownExceptionInTraitNode; use PHPStan\Node\CatchWithUnthrownExceptionNode; use PHPStan\Node\ClassConstantsNode; use PHPStan\Node\ClassMethodsNode; @@ -2372,6 +2373,13 @@ public function processStmtNode( // 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()) { + $this->callNodeCallback($nodeCallback, new CatchWithThrownExceptionInTraitNode($catchNode, $originalCatchTypes[$catchTypeIndex]), $scope, $storage); + } + continue; } $this->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(); + } + +}