Skip to content

Commit 04f8636

Browse files
ondrejmirtesondrejmirtes
committed
Bleeding edge - report "missing return" error closer to where the return is missing
This also fixes false positives about `@param-out`
1 parent 28dff17 commit 04f8636

14 files changed

+203
-15
lines changed

Diff for: ‎conf/bleedingEdge.neon

+1
Original file line numberDiff line numberDiff line change
@@ -57,5 +57,6 @@ parameters:
5757
narrowPregMatches: true
5858
uselessReturnValue: true
5959
printfArrayParameters: true
60+
preciseMissingReturn: true
6061
stubFiles:
6162
- ../stubs/bleedingEdge/Rule.stub

Diff for: ‎conf/config.neon

+2
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ parameters:
9292
narrowPregMatches: false
9393
uselessReturnValue: false
9494
printfArrayParameters: false
95+
preciseMissingReturn: false
9596
fileExtensions:
9697
- php
9798
checkAdvancedIsset: false
@@ -535,6 +536,7 @@ services:
535536
detectDeadTypeInMultiCatch: %featureToggles.detectDeadTypeInMultiCatch%
536537
universalObjectCratesClasses: %universalObjectCratesClasses%
537538
paramOutType: %featureToggles.paramOutType%
539+
preciseMissingReturn: %featureToggles.preciseMissingReturn%
538540

539541
-
540542
class: PHPStan\Analyser\ConstantResolver

Diff for: ‎conf/parametersSchema.neon

+1
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ parametersSchema:
8787
narrowPregMatches: bool()
8888
uselessReturnValue: bool()
8989
printfArrayParameters: bool()
90+
preciseMissingReturn: bool()
9091
])
9192
fileExtensions: listOf(string())
9293
checkAdvancedIsset: bool()

Diff for: ‎src/Analyser/EndStatementResult.php

+27
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace PHPStan\Analyser;
4+
5+
use PhpParser\Node\Stmt;
6+
7+
class EndStatementResult
8+
{
9+
10+
public function __construct(
11+
private Stmt $statement,
12+
private StatementResult $result,
13+
)
14+
{
15+
}
16+
17+
public function getStatement(): Stmt
18+
{
19+
return $this->statement;
20+
}
21+
22+
public function getResult(): StatementResult
23+
{
24+
return $this->result;
25+
}
26+
27+
}

Diff for: ‎src/Analyser/NodeScopeResolver.php

+60-13
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,7 @@ public function __construct(
261261
private readonly bool $treatPhpDocTypesAsCertain,
262262
private readonly bool $detectDeadTypeInMultiCatch,
263263
private readonly bool $paramOutType,
264+
private readonly bool $preciseMissingReturn,
264265
)
265266
{
266267
$earlyTerminatingMethodNames = [];
@@ -360,18 +361,38 @@ public function processStmtNodes(
360361
if ($shouldCheckLastStatement && $isLast) {
361362
/** @var Node\Stmt\Function_|Node\Stmt\ClassMethod|Expr\Closure $parentNode */
362363
$parentNode = $parentNode;
363-
$nodeCallback(new ExecutionEndNode(
364-
$stmt,
365-
new StatementResult(
366-
$scope,
367-
$hasYield,
368-
$statementResult->isAlwaysTerminating(),
369-
$statementResult->getExitPoints(),
370-
$statementResult->getThrowPoints(),
371-
$statementResult->getImpurePoints(),
372-
),
373-
$parentNode->returnType !== null,
374-
), $scope);
364+
365+
$endStatements = $statementResult->getEndStatements();
366+
if ($this->preciseMissingReturn && count($endStatements) > 0) {
367+
foreach ($endStatements as $endStatement) {
368+
$endStatementResult = $endStatement->getResult();
369+
$nodeCallback(new ExecutionEndNode(
370+
$endStatement->getStatement(),
371+
new StatementResult(
372+
$endStatementResult->getScope(),
373+
$hasYield,
374+
$endStatementResult->isAlwaysTerminating(),
375+
$endStatementResult->getExitPoints(),
376+
$endStatementResult->getThrowPoints(),
377+
$endStatementResult->getImpurePoints(),
378+
),
379+
$parentNode->returnType !== null,
380+
), $endStatementResult->getScope());
381+
}
382+
} else {
383+
$nodeCallback(new ExecutionEndNode(
384+
$stmt,
385+
new StatementResult(
386+
$scope,
387+
$hasYield,
388+
$statementResult->isAlwaysTerminating(),
389+
$statementResult->getExitPoints(),
390+
$statementResult->getThrowPoints(),
391+
$statementResult->getImpurePoints(),
392+
),
393+
$parentNode->returnType !== null,
394+
), $scope);
395+
}
375396
}
376397

377398
$exitPoints = array_merge($exitPoints, $statementResult->getExitPoints());
@@ -915,6 +936,7 @@ private function processStmtNode(
915936
$exitPoints = [];
916937
$throwPoints = $overridingThrowPoints ?? $condResult->getThrowPoints();
917938
$impurePoints = $condResult->getImpurePoints();
939+
$endStatements = [];
918940
$finalScope = null;
919941
$alwaysTerminating = true;
920942
$hasYield = $condResult->hasYield();
@@ -928,6 +950,13 @@ private function processStmtNode(
928950
$branchScope = $branchScopeStatementResult->getScope();
929951
$finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? null : $branchScope;
930952
$alwaysTerminating = $branchScopeStatementResult->isAlwaysTerminating();
953+
if (count($branchScopeStatementResult->getEndStatements()) > 0) {
954+
$endStatements = array_merge($endStatements, $branchScopeStatementResult->getEndStatements());
955+
} elseif (count($stmt->stmts) > 0) {
956+
$endStatements[] = new EndStatementResult($stmt->stmts[count($stmt->stmts) - 1], $branchScopeStatementResult);
957+
} else {
958+
$endStatements[] = new EndStatementResult($stmt, $branchScopeStatementResult);
959+
}
931960
$hasYield = $branchScopeStatementResult->hasYield() || $hasYield;
932961
}
933962

@@ -960,6 +989,13 @@ private function processStmtNode(
960989
$branchScope = $branchScopeStatementResult->getScope();
961990
$finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope);
962991
$alwaysTerminating = $alwaysTerminating && $branchScopeStatementResult->isAlwaysTerminating();
992+
if (count($branchScopeStatementResult->getEndStatements()) > 0) {
993+
$endStatements = array_merge($endStatements, $branchScopeStatementResult->getEndStatements());
994+
} elseif (count($elseif->stmts) > 0) {
995+
$endStatements[] = new EndStatementResult($elseif->stmts[count($elseif->stmts) - 1], $branchScopeStatementResult);
996+
} else {
997+
$endStatements[] = new EndStatementResult($elseif, $branchScopeStatementResult);
998+
}
963999
$hasYield = $hasYield || $branchScopeStatementResult->hasYield();
9641000
}
9651001

@@ -989,6 +1025,13 @@ private function processStmtNode(
9891025
$branchScope = $branchScopeStatementResult->getScope();
9901026
$finalScope = $branchScopeStatementResult->isAlwaysTerminating() ? $finalScope : $branchScope->mergeWith($finalScope);
9911027
$alwaysTerminating = $alwaysTerminating && $branchScopeStatementResult->isAlwaysTerminating();
1028+
if (count($branchScopeStatementResult->getEndStatements()) > 0) {
1029+
$endStatements = array_merge($endStatements, $branchScopeStatementResult->getEndStatements());
1030+
} elseif (count($stmt->else->stmts) > 0) {
1031+
$endStatements[] = new EndStatementResult($stmt->else->stmts[count($stmt->else->stmts) - 1], $branchScopeStatementResult);
1032+
} else {
1033+
$endStatements[] = new EndStatementResult($stmt->else, $branchScopeStatementResult);
1034+
}
9921035
$hasYield = $hasYield || $branchScopeStatementResult->hasYield();
9931036
}
9941037
}
@@ -997,7 +1040,11 @@ private function processStmtNode(
9971040
$finalScope = $scope;
9981041
}
9991042

1000-
return new StatementResult($finalScope, $hasYield, $alwaysTerminating, $exitPoints, $throwPoints, $impurePoints);
1043+
if ($stmt->else === null && !$ifAlwaysTrue && !$lastElseIfConditionIsTrue) {
1044+
$endStatements[] = new EndStatementResult($stmt, new StatementResult($finalScope, $hasYield, $alwaysTerminating, $exitPoints, $throwPoints, $impurePoints));
1045+
}
1046+
1047+
return new StatementResult($finalScope, $hasYield, $alwaysTerminating, $exitPoints, $throwPoints, $impurePoints, $endStatements);
10011048
} elseif ($stmt instanceof Node\Stmt\TraitUse) {
10021049
$hasYield = false;
10031050
$throwPoints = [];

Diff for: ‎src/Analyser/StatementResult.php

+23
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ class StatementResult
1313
* @param StatementExitPoint[] $exitPoints
1414
* @param ThrowPoint[] $throwPoints
1515
* @param ImpurePoint[] $impurePoints
16+
* @param EndStatementResult[] $endStatements
1617
*/
1718
public function __construct(
1819
private MutatingScope $scope,
@@ -21,6 +22,7 @@ public function __construct(
2122
private array $exitPoints,
2223
private array $throwPoints,
2324
private array $impurePoints,
25+
private array $endStatements = [],
2426
)
2527
{
2628
}
@@ -165,4 +167,25 @@ public function getImpurePoints(): array
165167
return $this->impurePoints;
166168
}
167169

170+
/**
171+
* Top-level StatementResult represents the state of the code
172+
* at the end of control flow statements like If_ or TryCatch.
173+
*
174+
* It shows how Scope etc. looks like after If_ no matter
175+
* which code branch was executed.
176+
*
177+
* For If_, "end statements" contain the state of the code
178+
* at the end of each branch - if, elseifs, else, including the last
179+
* statement node in each branch.
180+
*
181+
* For nested ifs, end statements try to contain the last non-control flow
182+
* statement like Return_ or Throw_, instead of If_, TryCatch, or Foreach_.
183+
*
184+
* @return EndStatementResult[]
185+
*/
186+
public function getEndStatements(): array
187+
{
188+
return $this->endStatements;
189+
}
190+
168191
}

Diff for: ‎src/Rules/Variables/ParameterOutExecutionEndTypeRule.php

+13
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use PHPStan\Rules\RuleErrorBuilder;
1616
use PHPStan\Rules\RuleLevelHelper;
1717
use PHPStan\Type\ErrorType;
18+
use PHPStan\Type\NeverType;
1819
use PHPStan\Type\Type;
1920
use PHPStan\Type\TypeUtils;
2021
use PHPStan\Type\VerbosityLevel;
@@ -48,6 +49,18 @@ public function processNode(Node $node, Scope $scope): array
4849
return [];
4950
}
5051

52+
$endNode = $node->getNode();
53+
if ($endNode instanceof Node\Stmt\Expression) {
54+
$endNodeExpr = $endNode->expr;
55+
$endNodeExprType = $scope->getType($endNodeExpr);
56+
if ($endNodeExprType instanceof NeverType && $endNodeExprType->isExplicit()) {
57+
return [];
58+
}
59+
}
60+
if ($endNode instanceof Node\Stmt\Throw_) {
61+
return [];
62+
}
63+
5164
$variant = ParametersAcceptorSelector::selectSingle($inFunction->getVariants());
5265
$parameters = $variant->getParameters();
5366
$errors = [];

Diff for: ‎src/Testing/RuleTestCase.php

+1
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ private function getAnalyser(DirectRuleRegistry $ruleRegistry): Analyser
106106
$this->shouldTreatPhpDocTypesAsCertain(),
107107
self::getContainer()->getParameter('featureToggles')['detectDeadTypeInMultiCatch'],
108108
self::getContainer()->getParameter('featureToggles')['paramOutType'],
109+
self::getContainer()->getParameter('featureToggles')['preciseMissingReturn'],
109110
);
110111
$fileAnalyser = new FileAnalyser(
111112
$this->createScopeFactory($reflectionProvider, $typeSpecifier),

Diff for: ‎src/Testing/TypeInferenceTestCase.php

+1
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ public static function processFile(
8686
self::getContainer()->getParameter('treatPhpDocTypesAsCertain'),
8787
self::getContainer()->getParameter('featureToggles')['detectDeadTypeInMultiCatch'],
8888
self::getContainer()->getParameter('featureToggles')['paramOutType'],
89+
self::getContainer()->getParameter('featureToggles')['preciseMissingReturn'],
8990
);
9091
$resolver->setAnalysedFiles(array_map(static fn (string $file): string => $fileHelper->normalizePath($file), array_merge([$file], static::getAdditionalAnalysedFiles())));
9192

Diff for: ‎tests/PHPStan/Analyser/AnalyserTest.php

+1
Original file line numberDiff line numberDiff line change
@@ -742,6 +742,7 @@ private function createAnalyser(bool $enableIgnoreErrorsWithinPhpDocs): Analyser
742742
$this->shouldTreatPhpDocTypesAsCertain(),
743743
self::getContainer()->getParameter('featureToggles')['detectDeadTypeInMultiCatch'],
744744
self::getContainer()->getParameter('featureToggles')['paramOutType'],
745+
self::getContainer()->getParameter('featureToggles')['preciseMissingReturn'],
745746
);
746747
$lexer = new Lexer(['usedAttributes' => ['comments', 'startLine', 'endLine', 'startTokenPos', 'endTokenPos']]);
747748
$fileAnalyser = new FileAnalyser(

Diff for: ‎tests/PHPStan/Rules/Missing/MissingReturnRuleTest.php

+17-1
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,11 @@ public function testRule(): void
4040
],
4141
[
4242
'Method MissingReturn\Foo::doLorem() should return int but return statement is missing.',
43-
36,
43+
39,
44+
],
45+
[
46+
'Method MissingReturn\Foo::doLorem() should return int but return statement is missing.',
47+
47,
4448
],
4549
[
4650
'Anonymous function should return int but return statement is missing.',
@@ -106,6 +110,18 @@ public function testRule(): void
106110
'Method MissingReturn\NeverReturn::doBaz2() should always throw an exception or terminate script execution but doesn\'t do that.',
107111
481,
108112
],
113+
[
114+
'Method MissingReturn\MorePreciseMissingReturnLines::doFoo() should return int but return statement is missing.',
115+
514,
116+
],
117+
[
118+
'Method MissingReturn\MorePreciseMissingReturnLines::doFoo() should return int but return statement is missing.',
119+
515,
120+
],
121+
[
122+
'Method MissingReturn\MorePreciseMissingReturnLines::doFoo2() should return int but return statement is missing.',
123+
524,
124+
],
109125
]);
110126
}
111127

Diff for: ‎tests/PHPStan/Rules/Missing/data/missing-return.php

+25
Original file line numberDiff line numberDiff line change
@@ -504,3 +504,28 @@ function () {
504504
}
505505

506506
}
507+
508+
class MorePreciseMissingReturnLines
509+
{
510+
511+
public function doFoo(): int
512+
{
513+
if (doFoo()) {
514+
echo 1;
515+
} elseif (doBar()) {
516+
517+
} else {
518+
return 1;
519+
}
520+
}
521+
522+
public function doFoo2(): int
523+
{
524+
if (doFoo()) {
525+
return 1;
526+
} elseif (doBar()) {
527+
return 2;
528+
}
529+
}
530+
531+
}

Diff for: ‎tests/PHPStan/Rules/Variables/ParameterOutExecutionEndTypeRuleTest.php

+14-1
Original file line numberDiff line numberDiff line change
@@ -26,13 +26,17 @@ public function testRule(): void
2626
'Parameter &$p @param-out type of method ParameterOutExecutionEnd\Foo::foo2() expects string, string|null given.',
2727
21,
2828
],
29+
[
30+
'Parameter &$p @param-out type of method ParameterOutExecutionEnd\Foo::foo2() expects string, string|null given.',
31+
23,
32+
],
2933
[
3034
'Parameter &$p @param-out type of method ParameterOutExecutionEnd\Foo::foo3() expects string, string|null given.',
3135
34,
3236
],
3337
[
3438
'Parameter &$p @param-out type of method ParameterOutExecutionEnd\Foo::foo4() expects string, string|null given.',
35-
45,
39+
47,
3640
],
3741
[
3842
'Parameter &$p @param-out type of method ParameterOutExecutionEnd\Foo::foo6() expects int, string given.',
@@ -42,7 +46,16 @@ public function testRule(): void
4246
'Parameter &$p @param-out type of function ParameterOutExecutionEnd\foo2() expects string, string|null given.',
4347
80,
4448
],
49+
[
50+
'Parameter &$p @param-out type of function ParameterOutExecutionEnd\foo2() expects string, string|null given.',
51+
82,
52+
],
4553
]);
4654
}
4755

56+
public function testBug11363(): void
57+
{
58+
$this->analyse([__DIR__ . '/data/bug-11363.php'], []);
59+
}
60+
4861
}

Diff for: ‎tests/PHPStan/Rules/Variables/data/bug-11363.php

+17
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug11363;
4+
5+
/**
6+
* @param mixed $a
7+
* @param-out int $a
8+
*/
9+
function bar(&$a): void {
10+
if (is_string($a)) {
11+
$a = (int) $a;
12+
return;
13+
}
14+
else {
15+
throw new \Exception("err");
16+
}
17+
}

0 commit comments

Comments
 (0)