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

Author: peter17

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);

src/Node/CatchWithThrownExceptionInTraitNode.php

@@ -0,0 +1,51 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Node;
+
+use Override;
+use PhpParser\Node\Stmt\Catch_;
+use PhpParser\NodeAbstract;
+use PHPStan\Type\Type;
+
+/**
+ * A catch clause inside a trait whose caught type _is_ thrown in the try block.
+ *
+ * Emitted only in traits. A trait's catch can be dead in the context of one class using
+ * the trait and alive in the context of another, so CatchWithUnthrownExceptionRule has to
+ * learn about the alive ones as well to notice the disagreement. Dead catches keep being
+ * reported through CatchWithUnthrownExceptionNode.
+ */
+final class CatchWithThrownExceptionInTraitNode extends NodeAbstract implements VirtualNode
+{
+
+	public function __construct(private Catch_ $originalNode, private Type $originalCaughtType)
+	{
+		parent::__construct($originalNode->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 [];
+	}
+
+}

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<Rule<covariant Node>> $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<Rule<covariant Node>> $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<Rule<covariant Node>> $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),
 		]);
 	}
 

src/Rules/Exceptions/CatchWithThrownExceptionInTraitRule.php

@@ -0,0 +1,45 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Rules\Exceptions;
+
+use PhpParser\Node;
+use PHPStan\Analyser\CollectedDataEmitter;
+use PHPStan\Analyser\NodeCallbackInvoker;
+use PHPStan\Analyser\Scope;
+use PHPStan\DependencyInjection\RegisteredRule;
+use PHPStan\Node\CatchWithThrownExceptionInTraitNode;
+use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper;
+use PHPStan\Rules\Rule;
+
+/**
+ * Records that a catch clause in a trait is alive in the context of the current class,
+ * so that CatchWithUnthrownExceptionRule does not report it as dead based only on the
+ * classes using the trait where it happens to be unreachable.
+ *
+ * @implements Rule<CatchWithThrownExceptionInTraitNode>
+ */
+#[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 [];
+	}
+
+}

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];
 	}
 
 }

src/Rules/Exceptions/DeadCatchInTraitKey.php

@@ -0,0 +1,26 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Rules\Exceptions;
+
+use PhpParser\Node\Stmt\Catch_;
+use PHPStan\Type\Type;
+use function implode;
+use function sprintf;
+
+/**
+ * Identifies one caught type of one catch clause in a trait, so that the dead-catch
+ * verdicts collected from every class using the trait can be compared.
+ *
+ * The trait is parsed from the same file for every using class, so the caught type
+ * together with the line pins down the occurrence: `catch (A|B)` yields two keys,
+ * one per caught type.
+ */
+final class DeadCatchInTraitKey
+{
+
+	public static function create(Catch_ $catchNode, Type $originalCaughtType): string
+	{
+		return sprintf('%s:%d', implode('|', $originalCaughtType->getObjectClassNames()), $catchNode->getStartLine());
+	}
+
+}