Fix phpstan/phpstan#6670: Return in finally does not always overwrite return in try-catch

Author: phpstan-bot

src/Analyser/NodeScopeResolver.php

@@ -2027,7 +2027,7 @@ public function processStmtNode(
 				$impurePoints = array_merge($impurePoints, $finallyResult->getImpurePoints());
 				$finallyScope = $finallyResult->getScope();
 				$finalScope = $finallyResult->isAlwaysTerminating() ? $finalScope : $finalScope->processFinallyScope($finallyScope, $originalFinallyScope);
-				if (count($finallyResult->getExitPoints()) > 0) {
+				if (count($finallyResult->getExitPoints()) > 0 && $finallyResult->isAlwaysTerminating()) {
 					$this->callNodeCallback($nodeCallback, new FinallyExitPointsNode(
 						$finallyResult->toPublic()->getExitPoints(),
 						$finallyExitPoints,

tests/PHPStan/Rules/Exceptions/OverwrittenExitPointByFinallyRuleTest.php

@@ -38,6 +38,26 @@ public function testRule(): void
 		]);
 	}
 
+	public function testBug6670(): void
+	{
+		$this->analyse([__DIR__ . '/data/bug-6670.php'], []);
+	}
+
+	public function testBug12215(): void
+	{
+		$this->analyse([__DIR__ . '/data/bug-12215.php'], []);
+	}
+
+	public function testBug11906(): void
+	{
+		$this->analyse([__DIR__ . '/data/bug-11906.php'], []);
+	}
+
+	public function testBug7665(): void
+	{
+		$this->analyse([__DIR__ . '/data/bug-7665.php'], []);
+	}
+
 	public function testBug5627(): void
 	{
 		$this->analyse([__DIR__ . '/data/bug-5627.php'], [

tests/PHPStan/Rules/Exceptions/data/bug-11906.php

@@ -0,0 +1,15 @@
+<?php declare(strict_types = 1);
+
+namespace Bug11906;
+
+function func(): void {
+	try {
+		throw new \LogicException('test');
+	} catch (\LogicException $e) {
+		// This catch-block should cause line 7 to not be treated as an exit point
+	} finally {
+		if (getenv('FOO')) {
+			return;
+		}
+	}
+}

tests/PHPStan/Rules/Exceptions/data/bug-12215.php

@@ -0,0 +1,53 @@
+<?php declare(strict_types = 1);
+
+namespace Bug12215;
+
+abstract class Spinlock
+{
+	private float $expireTimeout;
+
+	private ?float $acquireTs = null;
+
+	private ?string $token = null;
+
+	public function __construct(float $expireTimeout = \PHP_INT_MAX)
+	{
+		$this->expireTimeout = $expireTimeout;
+
+		// acquire lock
+		$this->acquireTs = microtime(true);
+		$this->token = random_bytes(64);
+	}
+
+	protected function release(string $key): bool
+	{
+		try {
+			return $this->releaseWithToken($key, $this->token);
+		} finally {
+			$this->token = null;
+
+			$elapsedTime = microtime(true) - $this->acquireTs;
+			if ($elapsedTime >= $this->expireTimeout) {
+				throw new \Exception('Execution outside lock exception');
+			}
+		}
+	}
+
+	protected function release2(string $key): bool
+	{
+		try {
+			return $this->releaseWithToken($key, $this->token);
+		} finally {
+			try {
+				$elapsedTime = microtime(true) - $this->acquireTs;
+				if ($elapsedTime >= $this->expireTimeout) {
+					throw new \Exception('Execution outside lock exception');
+				}
+			} finally {
+				$this->token = null;
+			}
+		}
+	}
+
+	abstract protected function releaseWithToken(string $key, string $token): bool;
+}

tests/PHPStan/Rules/Exceptions/data/bug-6670.php

@@ -0,0 +1,24 @@
+<?php declare(strict_types = 1);
+
+namespace Bug6670;
+
+class HelloWorld
+{
+	public function sayHello(): string
+	{
+		try {
+			return 'string';
+		} finally {
+			try {
+				$this->clearCache();
+			} catch(\Exception $e) {
+				return 'same string';
+			}
+		}
+	}
+
+	private function clearCache(): void
+	{
+		// do...
+	}
+}

tests/PHPStan/Rules/Exceptions/data/bug-7665.php

@@ -0,0 +1,28 @@
+<?php declare(strict_types = 1);
+
+namespace Bug7665;
+
+class HelloWorld
+{
+	public function doStuff(): string
+	{
+		try {
+			if(rand(0,1) === 1) {
+				throw new \RuntimeException('Bad luck');
+			}
+
+
+			return 'yay';
+			// other stuff
+		} catch(\Throwable $e) {
+			if (rand(0,1) === 1) {
+				exit(1);
+			}
+			// do some stuff to reset
+		} finally {
+			if(rand(0,1) === 1) {
+				exit(1);
+			}
+		}
+	}
+}