diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index 244217b17a5..4efee95ecbb 100644 --- a/src/Analyser/NodeScopeResolver.php +++ b/src/Analyser/NodeScopeResolver.php @@ -2288,10 +2288,12 @@ public function processStmtNode( // emit error foreach ($matchingCatchTypes as $catchTypeIndex => $matched) { - if ($matched) { + // Matched (non-dead) catches only need to be reported to the rule + // when in a trait, to detect disagreement between the classes using it. + if ($matched && !$scope->isInTrait()) { continue; } - $this->callNodeCallback($nodeCallback, new CatchWithUnthrownExceptionNode($catchNode, $catchTypes[$catchTypeIndex], $originalCatchTypes[$catchTypeIndex]), $scope, $storage); + $this->callNodeCallback($nodeCallback, new CatchWithUnthrownExceptionNode($catchNode, $catchTypes[$catchTypeIndex], $originalCatchTypes[$catchTypeIndex], $matched), $scope, $storage); } if (count($matchingThrowPoints) === 0) { diff --git a/src/Node/CatchWithUnthrownExceptionNode.php b/src/Node/CatchWithUnthrownExceptionNode.php index 9288d863c1a..ca2d59536fa 100644 --- a/src/Node/CatchWithUnthrownExceptionNode.php +++ b/src/Node/CatchWithUnthrownExceptionNode.php @@ -13,7 +13,7 @@ final class CatchWithUnthrownExceptionNode extends NodeAbstract implements VirtualNode { - public function __construct(private Catch_ $originalNode, private Type $caughtType, private Type $originalCaughtType) + public function __construct(private Catch_ $originalNode, private Type $caughtType, private Type $originalCaughtType, private bool $matched) { parent::__construct($originalNode->getAttributes()); } @@ -33,6 +33,14 @@ public function getOriginalCaughtType(): Type return $this->originalCaughtType; } + /** + * Whether this caught type is actually thrown in the try block, i.e. the catch is not dead. + */ + public function isMatched(): bool + { + return $this->matched; + } + #[Override] public function getType(): string { 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/CatchWithUnthrownExceptionRule.php b/src/Rules/Exceptions/CatchWithUnthrownExceptionRule.php index f4e2479a2ad..3e4bee61105 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,55 @@ 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(), - ]; + // A trait's catch block 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. The key below identifies this specific catch + // type occurrence so its verdicts can be compared across all classes using the trait. + $key = sprintf('%s:%d', $node->getOriginalCaughtType()->describe(VerbosityLevel::typeOnly()), $node->getOriginalNode()->getStartLine()); + + if ($node->isMatched()) { + $this->constantConditionInTraitHelper->emitNoErrorForKey(self::class, $scope, $key); + return []; } - if (!$this->reportUncheckedExceptionDeadCatch) { - $isCheckedException = false; - foreach ($node->getCaughtType()->getObjectClassNames() as $objectClassName) { - if ($this->exceptionTypeResolver->isCheckedException($objectClassName, $scope)) { - $isCheckedException = true; - break; + if ($node->getCaughtType() instanceof NeverType) { + $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 (!$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()) { + $this->constantConditionInTraitHelper->emitErrorForKey(self::class, $scope, $node->getOriginalNode(), $key, true, $error); + return []; + } + + return [$error]; } } diff --git a/tests/PHPStan/Rules/Exceptions/AbilityToDisableImplicitThrowsTest.php b/tests/PHPStan/Rules/Exceptions/AbilityToDisableImplicitThrowsTest.php index 7c7bda75143..bdd78e5899b 100644 --- a/tests/PHPStan/Rules/Exceptions/AbilityToDisableImplicitThrowsTest.php +++ b/tests/PHPStan/Rules/Exceptions/AbilityToDisableImplicitThrowsTest.php @@ -2,26 +2,37 @@ 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 ConstantConditionInTraitRule(), + ]); } public function testRule(): void @@ -97,6 +108,11 @@ public function testBug7799(): void ]); } + public function testBug10315(): void + { + $this->analyse([__DIR__ . '/data/bug-10315.php'], []); + } + 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..1366759eec3 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,21 @@ 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 ConstantConditionInTraitRule(), + ]); } public function testRule(): void @@ -842,4 +853,9 @@ public function testBug9826(): void $this->analyse([__DIR__ . '/data/bug-9826.php'], []); } + public function testBug10315(): void + { + $this->analyse([__DIR__ . '/data/bug-10315.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; + +}