Skip to content

Commit 06c967a

Browse files
VincentLangletphpstan-bot
VincentLanglet
authored and
phpstan-bot
committed
Fix phpstan/phpstan#14323: Inconsistent variable definedness in new expression arguments
- Constructor throw points in NewHandler were created before processArgs(), using a scope that didn't include variable assignments from constructor arguments - Refactored NewHandler to defer constructor throw point creation until after processArgs() completes, matching the pattern used by MethodCallHandler - Split processConstructorReflection() to separate throw point creation into createConstructorThrowPoints() which uses the post-args scope - Added regression test in tests/PHPStan/Rules/Variables/data/bug-14323.php
1 parent f077b36 commit 06c967a

3 files changed

Lines changed: 186 additions & 12 deletions

File tree

‎src/Analyser/ExprHandler/NewHandler.php‎

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -86,12 +86,15 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
8686
$impurePoints = [];
8787
$isAlwaysTerminating = false;
8888
$normalizedExpr = $expr;
89+
$classReflection = null;
90+
$classFound = true;
91+
$deferConstructorThrowPoints = false;
8992
if ($expr->class instanceof Name) {
9093
$className = $scope->resolveName($expr->class);
9194

92-
[$constructorReflection, $parametersAcceptor, $constructorThrowPoints, $constructorImpurePoints] = $this->processConstructorReflection($className, $expr, $scope);
93-
$throwPoints = array_merge($throwPoints, $constructorThrowPoints);
95+
[$constructorReflection, $parametersAcceptor, $classReflection, $classFound, $constructorImpurePoints] = $this->processConstructorReflection($className, $expr, $scope);
9496
$impurePoints = array_merge($impurePoints, $constructorImpurePoints);
97+
$deferConstructorThrowPoints = true;
9598

9699
if ($parametersAcceptor !== null) {
97100
$normalizedExpr = ArgumentsNormalizer::reorderNewArguments($parametersAcceptor, $expr) ?? $expr;
@@ -176,9 +179,9 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
176179
$throwPoints = array_merge($throwPoints, $additionalThrowPoints);
177180

178181
if ($className !== null) {
179-
[$constructorReflection, $parametersAcceptor, $constructorThrowPoints, $constructorImpurePoints] = $this->processConstructorReflection($className, $expr, $scope);
180-
$throwPoints = array_merge($throwPoints, $constructorThrowPoints);
182+
[$constructorReflection, $parametersAcceptor, $classReflection, $classFound, $constructorImpurePoints] = $this->processConstructorReflection($className, $expr, $scope);
181183
$impurePoints = array_merge($impurePoints, $constructorImpurePoints);
184+
$deferConstructorThrowPoints = true;
182185
} else {
183186
$throwPoints[] = InternalThrowPoint::createImplicit($scope, $expr);
184187
$impurePoints[] = new ImpurePoint(
@@ -202,6 +205,10 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
202205
$impurePoints = array_merge($impurePoints, $argsResult->getImpurePoints());
203206
$isAlwaysTerminating = $isAlwaysTerminating || $argsResult->isAlwaysTerminating();
204207

208+
if ($deferConstructorThrowPoints) {
209+
$throwPoints = array_merge($throwPoints, $this->createConstructorThrowPoints($expr, $scope, $constructorReflection, $parametersAcceptor, $classReflection, $classFound));
210+
}
211+
205212
return new ExpressionResult(
206213
$scope,
207214
hasYield: $hasYield,
@@ -212,16 +219,16 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
212219
}
213220

214221
/**
215-
* @return array{?MethodReflection, ?ParametersAcceptor, InternalThrowPoint[], ImpurePoint[]}
222+
* @return array{?MethodReflection, ?ParametersAcceptor, ?ClassReflection, bool, ImpurePoint[]}
216223
*/
217224
private function processConstructorReflection(string $className, New_ $expr, MutatingScope $scope): array
218225
{
219226
$constructorReflection = null;
220227
$parametersAcceptor = null;
221-
$throwPoints = [];
222228
$impurePoints = [];
223229

224230
$classReflection = null;
231+
$classFound = true;
225232
if ($this->reflectionProvider->hasClass($className)) {
226233
$classReflection = $this->reflectionProvider->getClass($className);
227234
if ($classReflection->hasConstructor()) {
@@ -232,13 +239,9 @@ private function processConstructorReflection(string $className, New_ $expr, Mut
232239
$constructorReflection->getVariants(),
233240
$constructorReflection->getNamedArgumentsVariants(),
234241
);
235-
$constructorThrowPoint = $this->getConstructorThrowPoint($constructorReflection, $parametersAcceptor, $classReflection, $expr, new Name\FullyQualified($className), $expr->getArgs(), $scope);
236-
if ($constructorThrowPoint !== null) {
237-
$throwPoints[] = $constructorThrowPoint;
238-
}
239242
}
240243
} else {
241-
$throwPoints[] = InternalThrowPoint::createImplicit($scope, $expr);
244+
$classFound = false;
242245
}
243246

244247
if ($constructorReflection !== null) {
@@ -262,7 +265,25 @@ private function processConstructorReflection(string $className, New_ $expr, Mut
262265
);
263266
}
264267

265-
return [$constructorReflection, $parametersAcceptor, $throwPoints, $impurePoints];
268+
return [$constructorReflection, $parametersAcceptor, $classReflection, $classFound, $impurePoints];
269+
}
270+
271+
/**
272+
* @return InternalThrowPoint[]
273+
*/
274+
private function createConstructorThrowPoints(New_ $expr, MutatingScope $scope, ?MethodReflection $constructorReflection, ?ParametersAcceptor $parametersAcceptor, ?ClassReflection $classReflection, bool $classFound): array
275+
{
276+
$throwPoints = [];
277+
if ($classReflection !== null && $constructorReflection !== null && $parametersAcceptor !== null) {
278+
$constructorThrowPoint = $this->getConstructorThrowPoint($constructorReflection, $parametersAcceptor, $classReflection, $expr, new Name\FullyQualified($classReflection->getName()), $expr->getArgs(), $scope);
279+
if ($constructorThrowPoint !== null) {
280+
$throwPoints[] = $constructorThrowPoint;
281+
}
282+
} elseif (!$classFound) {
283+
$throwPoints[] = InternalThrowPoint::createImplicit($scope, $expr);
284+
}
285+
286+
return $throwPoints;
266287
}
267288

268289
/**

‎tests/PHPStan/Rules/Variables/DefinedVariableRuleTest.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1432,6 +1432,29 @@ public function testBug14318(): void
14321432
$this->analyse([__DIR__ . '/data/bug-14318.php'], []);
14331433
}
14341434

1435+
public function testBug14323(): void
1436+
{
1437+
$this->cliArgumentsVariablesRegistered = true;
1438+
$this->polluteScopeWithLoopInitialAssignments = false;
1439+
$this->checkMaybeUndefinedVariables = true;
1440+
$this->polluteScopeWithAlwaysIterableForeach = true;
1441+
1442+
$this->analyse([__DIR__ . '/data/bug-14323.php'], [
1443+
[
1444+
'Variable $command might not be defined.',
1445+
40,
1446+
],
1447+
[
1448+
'Variable $command might not be defined.',
1449+
66,
1450+
],
1451+
[
1452+
'Variable $command might not be defined.',
1453+
118,
1454+
],
1455+
]);
1456+
}
1457+
14351458
#[RequiresPhp('>= 8.0')]
14361459
public function testBug14274(): void
14371460
{

‎tests/PHPStan/Rules/Variables/data/bug-14323.php‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug14323;
4+
5+
class ProcessFailedException extends \RuntimeException {}
6+
7+
class Process {
8+
/** @param array<string> $a */
9+
public function __construct(array $a) {}
10+
}
11+
12+
class Process2 {
13+
/**
14+
* @param array<string> $a
15+
* @throws ProcessFailedException
16+
*/
17+
public function __construct(array $a) {}
18+
}
19+
20+
class Process3 {
21+
/**
22+
* @param array<string> $a
23+
* @throws void
24+
*/
25+
public function __construct(array $a) {}
26+
}
27+
28+
abstract class DbCommand
29+
{
30+
/**
31+
* @return int
32+
*/
33+
public function handle()
34+
{
35+
try {
36+
new Process(
37+
array_merge([$command = $this->getCommand()])
38+
);
39+
} catch (ProcessFailedException $e) {
40+
echo ("{$command} not found in path.");
41+
42+
return 1;
43+
}
44+
45+
return 0;
46+
}
47+
48+
/**
49+
* @return string
50+
*/
51+
abstract public function getCommand();
52+
}
53+
54+
abstract class DbCommand2
55+
{
56+
/**
57+
* @return int
58+
*/
59+
public function handle()
60+
{
61+
try {
62+
new Process(
63+
[$command = $this->getCommand()]
64+
);
65+
} catch (ProcessFailedException $e) {
66+
echo ("{$command} not found in path.");
67+
68+
return 1;
69+
}
70+
71+
return 0;
72+
}
73+
74+
/**
75+
* @return string
76+
*/
77+
abstract public function getCommand();
78+
}
79+
80+
abstract class DbCommand3
81+
{
82+
/**
83+
* @return int
84+
*/
85+
public function handle()
86+
{
87+
try {
88+
new Process2(
89+
array_merge([$command = $this->getCommand()])
90+
);
91+
} catch (ProcessFailedException $e) {
92+
echo ("{$command} not found in path.");
93+
94+
return 1;
95+
}
96+
97+
return 0;
98+
}
99+
100+
/**
101+
* @return string
102+
*/
103+
abstract public function getCommand();
104+
}
105+
106+
abstract class DbCommand4
107+
{
108+
/**
109+
* @return int
110+
*/
111+
public function handle()
112+
{
113+
try {
114+
new Process3(
115+
array_merge([$command = $this->getCommand()])
116+
);
117+
} catch (ProcessFailedException $e) {
118+
echo ("{$command} not found in path.");
119+
120+
return 1;
121+
}
122+
123+
return 0;
124+
}
125+
126+
/**
127+
* @return string
128+
*/
129+
abstract public function getCommand();
130+
}

0 commit comments

Comments
 (0)