diff --git a/src/Analyser/StmtHandler/TryCatchHandler.php b/src/Analyser/StmtHandler/TryCatchHandler.php index c41c2168982..fc5085b0d9d 100644 --- a/src/Analyser/StmtHandler/TryCatchHandler.php +++ b/src/Analyser/StmtHandler/TryCatchHandler.php @@ -21,6 +21,7 @@ use PHPStan\Analyser\VariableFlowBuilder; use PHPStan\Dependency\Dependencies; use PHPStan\DependencyInjection\AutowiredService; +use PHPStan\Node\CatchWithThrownExceptionInTraitNode; use PHPStan\Node\CatchWithUnthrownExceptionNode; use PHPStan\Node\Expr\TypeExpr; use PHPStan\Node\FinallyExitPointsNode; @@ -208,6 +209,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..4ae05f68331 --- /dev/null +++ b/src/Node/CatchWithThrownExceptionInTraitNode.php @@ -0,0 +1,53 @@ +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 1052fa1991a..d64747bcb77 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 @@ -134,4 +148,26 @@ public static function getAdditionalConfigFiles(): array ); } + 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, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleConfigPhpTest.php b/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleConfigPhpTest.php index 71bea413312..ed67520c165 100644 --- a/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleConfigPhpTest.php +++ b/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleConfigPhpTest.php @@ -2,6 +2,7 @@ namespace PHPStan\Rules\Exceptions; +use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper; use PHPStan\Rules\Rule; use PHPStan\Testing\RuleTestCase; use const PHP_VERSION_ID; @@ -20,7 +21,7 @@ protected function getRule(): Rule [], [], [], - ), true); + ), true, self::getContainer()->getByType(ConstantConditionInTraitHelper::class)); } public function testThrowTypesInPhpVersionRange(): void diff --git a/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleGetClassConfigPhpTest.php b/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleGetClassConfigPhpTest.php index af9dbc277b8..af79ff5043d 100644 --- a/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleGetClassConfigPhpTest.php +++ b/tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleGetClassConfigPhpTest.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 testGetClassThrowTypeInPhpVersionRange(): void 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 43633d859c5..0dc63dacf47 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 { @@ -26,13 +29,24 @@ public static function getComposerAutoloaderProjectPaths(): array 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 @@ -1500,4 +1514,14 @@ public function testUnserializeThrowTypeBeforePhp8(): 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'], []); + } + } 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(); + } + +} diff --git a/turbo-ext/src/TryCatchHandler.cpp b/turbo-ext/src/TryCatchHandler.cpp index 78819f6485a..c449267340b 100644 --- a/turbo-ext/src/TryCatchHandler.cpp +++ b/turbo-ext/src/TryCatchHandler.cpp @@ -427,7 +427,22 @@ class TryCatchHandler // emit error for (uint32_t catchTypeIndex = 0; catchTypeIndex < catchTypeCount; catchTypeIndex++) { - if (matchingCatchTypes[catchTypeIndex]) continue; + if (matchingCatchTypes[catchTypeIndex]) { + // 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. + bool inTrait; + if (UNEXPECTED(!pt_mutating_scope_is_in_trait(Z_OBJ_P(scope), inTrait))) return zv::Val(); + if (inTrait) { + zval *originalCaughtType = zend_hash_index_find(originalCatchTypes.table(), catchTypeIndex); + zv::Args nodeArgv{catchNode, originalCaughtType}; + zv::Val thrownNode = pt_type_new(PT_CLASS_CATCH_WITH_THROWN_EXCEPTION_IN_TRAIT_NODE, 2, nodeArgv); + if (UNEXPECTED(thrownNode.isUndef())) return zv::Val(); + if (UNEXPECTED(!pt_node_scope_resolver_call_node_callback(nodeScopeResolver, nodeCallback, thrownNode.raw(), scope, storage))) return zv::Val(); + } + + continue; + } zval *caughtType = zend_hash_index_find(catchTypes.table(), catchTypeIndex); zval *originalCaughtType = zend_hash_index_find(originalCatchTypes.table(), catchTypeIndex); zv::Args nodeArgv{catchNode, caughtType, originalCaughtType}; diff --git a/turbo-ext/src/support.cpp b/turbo-ext/src/support.cpp index e34c9b87288..148a7f1d63e 100644 --- a/turbo-ext/src/support.cpp +++ b/turbo-ext/src/support.cpp @@ -343,6 +343,7 @@ static const pt_class_template pt_class_templates[PT_CLASS_COUNT] = { /* PT_CLASS_SWITCH_CONDITION_ARM */ {"switchConditionArm", "PHPStan\\Node\\SwitchConditionArm"}, /* PT_CLASS_SWITCH_CONDITION_NODE */ {"switchConditionNode", "PHPStan\\Node\\SwitchConditionNode"}, /* PT_CLASS_CATCH_WITH_UNTHROWN_EXCEPTION_NODE */ {"catchWithUnthrownExceptionNode", "PHPStan\\Node\\CatchWithUnthrownExceptionNode"}, + /* PT_CLASS_CATCH_WITH_THROWN_EXCEPTION_IN_TRAIT_NODE */ {"catchWithThrownExceptionInTraitNode", "PHPStan\\Node\\CatchWithThrownExceptionInTraitNode"}, /* PT_CLASS_FINALLY_EXIT_POINTS_NODE */ {"finallyExitPointsNode", "PHPStan\\Node\\FinallyExitPointsNode"}, /* PT_CLASS_IN_FOREACH_NODE */ {"inForeachNode", "PHPStan\\Node\\InForeachNode"}, /* PT_CLASS_FOREACH_VALUE_BY_REF_EXPR */ {"foreachValueByRefExpr", "PHPStan\\Node\\Expr\\ForeachValueByRefExpr"}, diff --git a/turbo-ext/src/support.h b/turbo-ext/src/support.h index 6c0e26f9c3a..2dcdf03a752 100644 --- a/turbo-ext/src/support.h +++ b/turbo-ext/src/support.h @@ -438,6 +438,7 @@ enum { PT_CLASS_SWITCH_CONDITION_ARM, PT_CLASS_SWITCH_CONDITION_NODE, PT_CLASS_CATCH_WITH_UNTHROWN_EXCEPTION_NODE, + PT_CLASS_CATCH_WITH_THROWN_EXCEPTION_IN_TRAIT_NODE, PT_CLASS_FINALLY_EXIT_POINTS_NODE, /* ForeachHandler.cpp */ PT_CLASS_IN_FOREACH_NODE,