Bleeding edge - report "missing return" error closer to where the return is missing

ondrejmirtes · ondrejmirtes · commit 04f8636e6577 · 2024-07-20T16:17:59.000+02:00
This also fixes false positives about `@param-out`
diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon
@@ -57,5 +57,6 @@ parameters:
 		narrowPregMatches: true
 		uselessReturnValue: true
 		printfArrayParameters: true
+		preciseMissingReturn: true
 	stubFiles:
 		- ../stubs/bleedingEdge/Rule.stub
diff --git a/conf/config.neon b/conf/config.neon
@@ -92,6 +92,7 @@ parameters:
 		narrowPregMatches: false
 		uselessReturnValue: false
 		printfArrayParameters: false
+		preciseMissingReturn: false
 	fileExtensions:
 		- php
 	checkAdvancedIsset: false
@@ -535,6 +536,7 @@ services:
 			detectDeadTypeInMultiCatch: %featureToggles.detectDeadTypeInMultiCatch%
 			universalObjectCratesClasses: %universalObjectCratesClasses%
 			paramOutType: %featureToggles.paramOutType%
+			preciseMissingReturn: %featureToggles.preciseMissingReturn%
 
 	-
 		class: PHPStan\Analyser\ConstantResolver
diff --git a/conf/parametersSchema.neon b/conf/parametersSchema.neon
@@ -87,6 +87,7 @@ parametersSchema:
 		narrowPregMatches: bool()
 		uselessReturnValue: bool()
 		printfArrayParameters: bool()
+		preciseMissingReturn: bool()
 	])
 	fileExtensions: listOf(string())
 	checkAdvancedIsset: bool()
diff --git a/src/Analyser/EndStatementResult.php b/src/Analyser/EndStatementResult.php
@@ -0,0 +1,27 @@
+<?php declare(strict_types = 1);
+
+namespace PHPStan\Analyser;
+
+use PhpParser\Node\Stmt;
+
+class EndStatementResult
+{
+
+	public function __construct(
+		private Stmt $statement,
+		private StatementResult $result,
+	)
+	{
+	}
+
+	public function getStatement(): Stmt
+	{
+		return $this->statement;
+	}
+
+	public function getResult(): StatementResult
+	{
+		return $this->result;
+	}
+
+}
diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php
@@ -261,6 +261,7 @@ public function __construct(
 		private readonly bool $treatPhpDocTypesAsCertain,
 		private readonly bool $detectDeadTypeInMultiCatch,
 		private readonly bool $paramOutType,
+		private readonly bool $preciseMissingReturn,
 	)
 	{
 		$earlyTerminatingMethodNames = [];
@@ -360,18 +361,38 @@ public function processStmtNodes(
 			if ($shouldCheckLastStatement && $isLast) {
 				/** @var Node\Stmt\Function_|Node\Stmt\ClassMethod|Expr\Closure $parentNode */
 				$parentNode = $parentNode;
-				$nodeCallback(new ExecutionEndNode(
-					$stmt,
-					new StatementResult(
-						$scope,
-						$hasYield,
-						$statementResult->isAlwaysTerminating(),
-						$statementResult->getExitPoints(),
-						$statementResult->getThrowPoints(),
-						$statementResult->getImpurePoints(),
-					),
-					$parentNode->returnType !== null,
-				), $scope);
+
+				$endStatements = $statementResult->getEndStatements();
+				if ($this->preciseMissingReturn && count($endStatements) > 0) {
+					foreach ($endStatements as $endStatement) {
+						$endStatementResult = $endStatement->getResult();
+						$nodeCallback(new ExecutionEndNode(
+							$endStatement->getStatement(),
+							new StatementResult(
+								$endStatementResult->getScope(),
+								$hasYield,
+								$endStatementResult->isAlwaysTerminating(),
+								$endStatementResult->getExitPoints(),
+								$endStatementResult->getThrowPoints(),
+								$endStatementResult->getImpurePoints(),
+							),
+							$parentNode->returnType !== null,
+						), $endStatementResult->getScope());
+					}
+				} else {
+					$nodeCallback(new ExecutionEndNode(
+						$stmt,
+						new StatementResult(
+							$scope,
+							$hasYield,
+							$statementResult->isAlwaysTerminating(),
+							$statementResult->getExitPoints(),
+							$statementResult->getThrowPoints(),
+							$statementResult->getImpurePoints(),
+						),
+						$parentNode->returnType !== null,
+					), $scope);
+				}
 			}
 
 			$exitPoints = array_merge($exitPoints, $statementResult->getExitPoints());
@@ -915,6 +936,7 @@ private function processStmtNode(
 			$exitPoints = [];
 			$throwPoints = $overridingThrowPoints ?? $condResult->getThrowPoints();
 			$impurePoints = $condResult->getImpurePoints();
+			$endStatements = [];
 			$finalScope = null;
 			$alwaysTerminating = true;
 			$hasYield = $condResult->hasYield();
@@ -928,6 +950,13 @@ private function processStmtNode(
 				$branchScope = $branchScopeStatementResult->getScope();
 				$finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? null : $branchScope;
 				$alwaysTerminating = $branchScopeStatementResult->isAlwaysTerminating();
+				if (count($branchScopeStatementResult->getEndStatements()) > 0) {
+					$endStatements = array_merge($endStatements, $branchScopeStatementResult->getEndStatements());
+				} elseif (count($stmt->stmts) > 0) {
+					$endStatements[] = new EndStatementResult($stmt->stmts[count($stmt->stmts) - 1], $branchScopeStatementResult);
+				} else {
+					$endStatements[] = new EndStatementResult($stmt, $branchScopeStatementResult);
+				}
 				$hasYield = $branchScopeStatementResult->hasYield() || $hasYield;
 			}
 
@@ -960,6 +989,13 @@ private function processStmtNode(
 					$branchScope = $branchScopeStatementResult->getScope();
 					$finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope);
 					$alwaysTerminating = $alwaysTerminating && $branchScopeStatementResult->isAlwaysTerminating();
+					if (count($branchScopeStatementResult->getEndStatements()) > 0) {
+						$endStatements = array_merge($endStatements, $branchScopeStatementResult->getEndStatements());
+					} elseif (count($elseif->stmts) > 0) {
+						$endStatements[] = new EndStatementResult($elseif->stmts[count($elseif->stmts) - 1], $branchScopeStatementResult);
+					} else {
+						$endStatements[] = new EndStatementResult($elseif, $branchScopeStatementResult);
+					}
 					$hasYield = $hasYield || $branchScopeStatementResult->hasYield();
 				}
 
@@ -989,6 +1025,13 @@ private function processStmtNode(
 					$branchScope = $branchScopeStatementResult->getScope();
 					$finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope);
 					$alwaysTerminating = $alwaysTerminating && $branchScopeStatementResult->isAlwaysTerminating();
+					if (count($branchScopeStatementResult->getEndStatements()) > 0) {
+						$endStatements = array_merge($endStatements, $branchScopeStatementResult->getEndStatements());
+					} elseif (count($stmt->else->stmts) > 0) {
+						$endStatements[] = new EndStatementResult($stmt->else->stmts[count($stmt->else->stmts) - 1], $branchScopeStatementResult);
+					} else {
+						$endStatements[] = new EndStatementResult($stmt->else, $branchScopeStatementResult);
+					}
 					$hasYield = $hasYield || $branchScopeStatementResult->hasYield();
 				}
 			}
@@ -997,7 +1040,11 @@ private function processStmtNode(
 				$finalScope = $scope;
 			}
 
-			return new StatementResult($finalScope, $hasYield, $alwaysTerminating, $exitPoints, $throwPoints, $impurePoints);
+			if ($stmt->else === null && !$ifAlwaysTrue && !$lastElseIfConditionIsTrue) {
+				$endStatements[] = new EndStatementResult($stmt, new StatementResult($finalScope, $hasYield, $alwaysTerminating, $exitPoints, $throwPoints, $impurePoints));
+			}
+
+			return new StatementResult($finalScope, $hasYield, $alwaysTerminating, $exitPoints, $throwPoints, $impurePoints, $endStatements);
 		} elseif ($stmt instanceof Node\Stmt\TraitUse) {
 			$hasYield = false;
 			$throwPoints = [];
diff --git a/src/Analyser/StatementResult.php b/src/Analyser/StatementResult.php
@@ -13,6 +13,7 @@ class StatementResult
 	 * @param StatementExitPoint[] $exitPoints
 	 * @param ThrowPoint[] $throwPoints
 	 * @param ImpurePoint[] $impurePoints
+	 * @param EndStatementResult[] $endStatements
 	 */
 	public function __construct(
 		private MutatingScope $scope,
@@ -21,6 +22,7 @@ public function __construct(
 		private array $exitPoints,
 		private array $throwPoints,
 		private array $impurePoints,
+		private array $endStatements = [],
 	)
 	{
 	}
@@ -165,4 +167,25 @@ public function getImpurePoints(): array
 		return $this->impurePoints;
 	}
 
+	/**
+	 * Top-level StatementResult represents the state of the code
+	 * at the end of control flow statements like If_ or TryCatch.
+	 *
+	 * It shows how Scope etc. looks like after If_ no matter
+	 * which code branch was executed.
+	 *
+	 * For If_, "end statements" contain the state of the code
+	 * at the end of each branch - if, elseifs, else, including the last
+	 * statement node in each branch.
+	 *
+	 * For nested ifs, end statements try to contain the last non-control flow
+	 * statement like Return_ or Throw_, instead of If_, TryCatch, or Foreach_.
+	 *
+	 * @return EndStatementResult[]
+	 */
+	public function getEndStatements(): array
+	{
+		return $this->endStatements;
+	}
+
 }
diff --git a/src/Rules/Variables/ParameterOutExecutionEndTypeRule.php b/src/Rules/Variables/ParameterOutExecutionEndTypeRule.php
@@ -15,6 +15,7 @@
 use PHPStan\Rules\RuleErrorBuilder;
 use PHPStan\Rules\RuleLevelHelper;
 use PHPStan\Type\ErrorType;
+use PHPStan\Type\NeverType;
 use PHPStan\Type\Type;
 use PHPStan\Type\TypeUtils;
 use PHPStan\Type\VerbosityLevel;
@@ -48,6 +49,18 @@ public function processNode(Node $node, Scope $scope): array
 			return [];
 		}
 
+		$endNode = $node->getNode();
+		if ($endNode instanceof Node\Stmt\Expression) {
+			$endNodeExpr = $endNode->expr;
+			$endNodeExprType = $scope->getType($endNodeExpr);
+			if ($endNodeExprType instanceof NeverType && $endNodeExprType->isExplicit()) {
+				return [];
+			}
+		}
+		if ($endNode instanceof Node\Stmt\Throw_) {
+			return [];
+		}
+
 		$variant = ParametersAcceptorSelector::selectSingle($inFunction->getVariants());
 		$parameters = $variant->getParameters();
 		$errors = [];
diff --git a/src/Testing/RuleTestCase.php b/src/Testing/RuleTestCase.php
@@ -106,6 +106,7 @@ private function getAnalyser(DirectRuleRegistry $ruleRegistry): Analyser
 				$this->shouldTreatPhpDocTypesAsCertain(),
 				self::getContainer()->getParameter('featureToggles')['detectDeadTypeInMultiCatch'],
 				self::getContainer()->getParameter('featureToggles')['paramOutType'],
+				self::getContainer()->getParameter('featureToggles')['preciseMissingReturn'],
 			);
 			$fileAnalyser = new FileAnalyser(
 				$this->createScopeFactory($reflectionProvider, $typeSpecifier),
diff --git a/src/Testing/TypeInferenceTestCase.php b/src/Testing/TypeInferenceTestCase.php
@@ -86,6 +86,7 @@ public static function processFile(
 			self::getContainer()->getParameter('treatPhpDocTypesAsCertain'),
 			self::getContainer()->getParameter('featureToggles')['detectDeadTypeInMultiCatch'],
 			self::getContainer()->getParameter('featureToggles')['paramOutType'],
+			self::getContainer()->getParameter('featureToggles')['preciseMissingReturn'],
 		);
 		$resolver->setAnalysedFiles(array_map(static fn (string $file): string => $fileHelper->normalizePath($file), array_merge([$file], static::getAdditionalAnalysedFiles())));
 
diff --git a/tests/PHPStan/Analyser/AnalyserTest.php b/tests/PHPStan/Analyser/AnalyserTest.php
@@ -742,6 +742,7 @@ private function createAnalyser(bool $enableIgnoreErrorsWithinPhpDocs): Analyser
 			$this->shouldTreatPhpDocTypesAsCertain(),
 			self::getContainer()->getParameter('featureToggles')['detectDeadTypeInMultiCatch'],
 			self::getContainer()->getParameter('featureToggles')['paramOutType'],
+			self::getContainer()->getParameter('featureToggles')['preciseMissingReturn'],
 		);
 		$lexer = new Lexer(['usedAttributes' => ['comments', 'startLine', 'endLine', 'startTokenPos', 'endTokenPos']]);
 		$fileAnalyser = new FileAnalyser(
diff --git a/tests/PHPStan/Rules/Missing/MissingReturnRuleTest.php b/tests/PHPStan/Rules/Missing/MissingReturnRuleTest.php
@@ -40,7 +40,11 @@ public function testRule(): void
 			],
 			[
 				'Method MissingReturn\Foo::doLorem() should return int but return statement is missing.',
-				36,
+				39,
+			],
+			[
+				'Method MissingReturn\Foo::doLorem() should return int but return statement is missing.',
+				47,
 			],
 			[
 				'Anonymous function should return int but return statement is missing.',
@@ -106,6 +110,18 @@ public function testRule(): void
 				'Method MissingReturn\NeverReturn::doBaz2() should always throw an exception or terminate script execution but doesn\'t do that.',
 				481,
 			],
+			[
+				'Method MissingReturn\MorePreciseMissingReturnLines::doFoo() should return int but return statement is missing.',
+				514,
+			],
+			[
+				'Method MissingReturn\MorePreciseMissingReturnLines::doFoo() should return int but return statement is missing.',
+				515,
+			],
+			[
+				'Method MissingReturn\MorePreciseMissingReturnLines::doFoo2() should return int but return statement is missing.',
+				524,
+			],
 		]);
 	}
 
diff --git a/tests/PHPStan/Rules/Missing/data/missing-return.php b/tests/PHPStan/Rules/Missing/data/missing-return.php
@@ -504,3 +504,28 @@ function () {
 	}
 
 }
+
+class MorePreciseMissingReturnLines
+{
+
+	public function doFoo(): int
+	{
+		if (doFoo()) {
+			echo 1;
+		} elseif (doBar()) {
+
+		} else {
+			return 1;
+		}
+	}
+
+	public function doFoo2(): int
+	{
+		if (doFoo()) {
+			return 1;
+		} elseif (doBar()) {
+			return 2;
+		}
+	}
+
+}
diff --git a/tests/PHPStan/Rules/Variables/ParameterOutExecutionEndTypeRuleTest.php b/tests/PHPStan/Rules/Variables/ParameterOutExecutionEndTypeRuleTest.php
@@ -26,13 +26,17 @@ public function testRule(): void
 				'Parameter &$p @param-out type of method ParameterOutExecutionEnd\Foo::foo2() expects string, string|null given.',
 				21,
 			],
+			[
+				'Parameter &$p @param-out type of method ParameterOutExecutionEnd\Foo::foo2() expects string, string|null given.',
+				23,
+			],
 			[
 				'Parameter &$p @param-out type of method ParameterOutExecutionEnd\Foo::foo3() expects string, string|null given.',
 				34,
 			],
 			[
 				'Parameter &$p @param-out type of method ParameterOutExecutionEnd\Foo::foo4() expects string, string|null given.',
-				45,
+				47,
 			],
 			[
 				'Parameter &$p @param-out type of method ParameterOutExecutionEnd\Foo::foo6() expects int, string given.',
@@ -42,7 +46,16 @@ public function testRule(): void
 				'Parameter &$p @param-out type of function ParameterOutExecutionEnd\foo2() expects string, string|null given.',
 				80,
 			],
+			[
+				'Parameter &$p @param-out type of function ParameterOutExecutionEnd\foo2() expects string, string|null given.',
+				82,
+			],
 		]);
 	}
 
+	public function testBug11363(): void
+	{
+		$this->analyse([__DIR__ . '/data/bug-11363.php'], []);
+	}
+
 }
diff --git a/tests/PHPStan/Rules/Variables/data/bug-11363.php b/tests/PHPStan/Rules/Variables/data/bug-11363.php
@@ -0,0 +1,17 @@
+<?php declare(strict_types = 1);
+
+namespace Bug11363;
+
+/**
+ * @param mixed $a
+ * @param-out int $a
+ */
+function bar(&$a): void {
+	if (is_string($a)) {
+		$a = (int) $a;
+		return;
+	}
+	else {
+		throw new \Exception("err");
+	}
+}
