Bleeding edge - rule for detecting overwriting exit points in finally

Author: ondrejmirtes

conf/config.level4.neon

@@ -20,6 +20,8 @@ rules:
 conditionalTags:
 	PHPStan\Rules\Exceptions\CatchWithUnthrownExceptionRule:
 		phpstan.rules.rule: %featureToggles.preciseExceptionTracking%
+	PHPStan\Rules\Exceptions\OverwrittenExitPointByFinallyRule:
+		phpstan.rules.rule: %featureToggles.preciseExceptionTracking%
 	PHPStan\Rules\DeadCode\UnusedPrivateConstantRule:
 		phpstan.rules.rule: %featureToggles.unusedClassElements%
 	PHPStan\Rules\DeadCode\UnusedPrivateMethodRule:
@@ -152,6 +154,9 @@ services:
 	-
 		class: PHPStan\Rules\Exceptions\CatchWithUnthrownExceptionRule
 
+	-
+		class: PHPStan\Rules\Exceptions\OverwrittenExitPointByFinallyRule
+
 	-
 		class: PHPStan\Rules\TooWideTypehints\TooWideMethodReturnTypehintRule
 		arguments:

src/Analyser/NodeScopeResolver.php

@@ -63,6 +63,7 @@
 use PHPStan\Node\ClassStatementsGatherer;
 use PHPStan\Node\ClosureReturnStatementsNode;
 use PHPStan\Node\ExecutionEndNode;
+use PHPStan\Node\FinallyExitPointsNode;
 use PHPStan\Node\FunctionReturnStatementsNode;
 use PHPStan\Node\InArrowFunctionNode;
 use PHPStan\Node\InClassMethodNode;
@@ -1288,6 +1289,12 @@ private function processStmtNode(
 				$throwPointsForLater = array_merge($throwPointsForLater, $finallyResult->getThrowPoints());
 				$finallyScope = $finallyResult->getScope();
 				$finalScope = $finallyResult->isAlwaysTerminating() ? $finalScope : $finalScope->processFinallyScope($finallyScope, $originalFinallyScope);
+				if (count($finallyResult->getExitPoints()) > 0) {
+					$nodeCallback(new FinallyExitPointsNode(
+						$finallyResult->getExitPoints(),
+						$exitPoints
+					), $scope);
+				}
 				$exitPoints = array_merge($exitPoints, $finallyResult->getExitPoints());
 			}
 

src/Node/FinallyExitPointsNode.php

@@ -0,0 +1,57 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Node;
+
+use PhpParser\NodeAbstract;
+use PHPStan\Analyser\StatementExitPoint;
+
+class FinallyExitPointsNode extends NodeAbstract implements VirtualNode
+{
+
+	/** @var StatementExitPoint[] */
+	private array $finallyExitPoints;
+
+	/** @var StatementExitPoint[] */
+	private array $tryCatchExitPoints;
+
+	/**
+	 * @param StatementExitPoint[] $finallyExitPoints
+	 * @param StatementExitPoint[] $tryCatchExitPoints
+	 */
+	public function __construct(array $finallyExitPoints, array $tryCatchExitPoints)
+	{
+		parent::__construct([]);
+		$this->finallyExitPoints = $finallyExitPoints;
+		$this->tryCatchExitPoints = $tryCatchExitPoints;
+	}
+
+	/**
+	 * @return StatementExitPoint[]
+	 */
+	public function getFinallyExitPoints(): array
+	{
+		return $this->finallyExitPoints;
+	}
+
+	/**
+	 * @return StatementExitPoint[]
+	 */
+	public function getTryCatchExitPoints(): array
+	{
+		return $this->tryCatchExitPoints;
+	}
+
+	public function getType(): string
+	{
+		return 'PHPStan_Node_FinallyExitPointsNode';
+	}
+
+	/**
+	 * @return string[]
+	 */
+	public function getSubNodeNames(): array
+	{
+		return [];
+	}
+
+}

src/Rules/Exceptions/OverwrittenExitPointByFinallyRule.php

@@ -0,0 +1,61 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Rules\Exceptions;
+
+use PhpParser\Node;
+use PHPStan\Analyser\Scope;
+use PHPStan\Node\FinallyExitPointsNode;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleErrorBuilder;
+
+/**
+ * @implements Rule<FinallyExitPointsNode>
+ */
+class OverwrittenExitPointByFinallyRule implements Rule
+{
+
+	public function getNodeType(): string
+	{
+		return FinallyExitPointsNode::class;
+	}
+
+	public function processNode(Node $node, Scope $scope): array
+	{
+		if (count($node->getTryCatchExitPoints()) === 0) {
+			return [];
+		}
+
+		$errors = [];
+		foreach ($node->getTryCatchExitPoints() as $exitPoint) {
+			$errors[] = RuleErrorBuilder::message(sprintf('This %s is overwritten by a different one in the finally block below.', $this->describeExitPoint($exitPoint->getStatement())))->line($exitPoint->getStatement()->getLine())->build();
+		}
+
+		foreach ($node->getFinallyExitPoints() as $exitPoint) {
+			$errors[] = RuleErrorBuilder::message(sprintf('The overwriting %s is on this line.', $this->describeExitPoint($exitPoint->getStatement())))->line($exitPoint->getStatement()->getLine())->build();
+		}
+
+		return $errors;
+	}
+
+	private function describeExitPoint(Node\Stmt $stmt): string
+	{
+		if ($stmt instanceof Node\Stmt\Return_) {
+			return 'return';
+		}
+
+		if ($stmt instanceof Node\Stmt\Throw_) {
+			return 'throw';
+		}
+
+		if ($stmt instanceof Node\Stmt\Continue_) {
+			return 'continue';
+		}
+
+		if ($stmt instanceof Node\Stmt\Break_) {
+			return 'break';
+		}
+
+		return 'exit point';
+	}
+
+}

tests/PHPStan/Rules/Exceptions/OverwrittenExitPointByFinallyRuleTest.php

@@ -0,0 +1,37 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Rules\Exceptions;
+
+use PHPStan\Rules\Rule;
+use PHPStan\Testing\RuleTestCase;
+
+/**
+ * @extends RuleTestCase<OverwrittenExitPointByFinallyRule>
+ */
+class OverwrittenExitPointByFinallyRuleTest extends RuleTestCase
+{
+
+	protected function getRule(): Rule
+	{
+		return new OverwrittenExitPointByFinallyRule();
+	}
+
+	public function testRule(): void
+	{
+		$this->analyse([__DIR__ . '/data/overwritten-exit-point.php'], [
+			[
+				'This return is overwritten by a different one in the finally block below.',
+				11,
+			],
+			[
+				'This return is overwritten by a different one in the finally block below.',
+				13,
+			],
+			[
+				'The overwriting return is on this line.',
+				15,
+			],
+		]);
+	}
+
+}

tests/PHPStan/Rules/Exceptions/data/overwritten-exit-point.php

@@ -0,0 +1,33 @@
+<?php
+
+namespace OverwrittenExitPoint;
+
+function (): string {
+	try {
+		if (rand(0, 1)) {
+			throw new \Exception();
+		}
+
+		return 'foo';
+	} catch (\Exception $e) {
+		return 'bar';
+	} finally {
+		return 'baz';
+	}
+};
+
+function (): string {
+	try {
+
+	} finally {
+		return 'foo';
+	}
+};
+
+function (): string {
+	try {
+		return 'foo';
+	} finally {
+
+	}
+};