diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon index 396a0acd13..3bbbeb6bc0 100644 --- 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 index 21aa21dfd3..ac8f18be39 100644 --- 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 index 8459195463..f33fc0ba5d 100644 --- 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 new file mode 100644 index 0000000000..32d0809bef --- /dev/null +++ b/src/Analyser/EndStatementResult.php @@ -0,0 +1,27 @@ +statement; + } + + public function getResult(): StatementResult + { + return $this->result; + } + +} diff --git a/src/Analyser/NodeScopeResolver.php b/src/Analyser/NodeScopeResolver.php index c1184e26d1..dbd1418a1d 100644 --- 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 index e731931225..20365299c0 100644 --- 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 index fee386168b..648781ccc5 100644 --- 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 index c01ddbdeea..3b4330a297 100644 --- 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 index 2b8de77989..c9c32db584 100644 --- 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 index c46aaa662c..b149c39ab0 100644 --- 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 index 152b97ab41..ad9fc94be5 100644 --- 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 index 5f46dcfdde..06a9463a58 100644 --- 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 index 96ff882d8c..26157f4ffb 100644 --- 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 new file mode 100644 index 0000000000..72bf9b9968 --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/bug-11363.php @@ -0,0 +1,17 @@ +