Skip to content

Commit 6bac0de

Browse files
phpstan-botVincentLangletclaude
authored
Fix phpstan/phpstan#14323: Weird variable might not be defined behavior (#5245)
Co-authored-by: VincentLanglet <9052536+VincentLanglet@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Vincent Langlet <vincentlanglet@hotmail.fr>
1 parent 3681e1d commit 6bac0de

File tree

8 files changed

+263
-34
lines changed

8 files changed

+263
-34
lines changed

‎src/Analyser/ExprHandler/NewHandler.php‎

Lines changed: 18 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
8181
{
8282
$parametersAcceptor = null;
8383
$constructorReflection = null;
84+
$classReflection = null;
8485
$hasYield = false;
8586
$throwPoints = [];
8687
$impurePoints = [];
@@ -89,8 +90,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
8990
if ($expr->class instanceof Name) {
9091
$className = $scope->resolveName($expr->class);
9192

92-
[$constructorReflection, $parametersAcceptor, $constructorThrowPoints, $constructorImpurePoints] = $this->processConstructorReflection($className, $expr, $scope);
93-
$throwPoints = array_merge($throwPoints, $constructorThrowPoints);
93+
[$constructorReflection, $classReflection, $parametersAcceptor, $constructorImpurePoints] = $this->processConstructorReflection($className, $expr, $scope);
9494
$impurePoints = array_merge($impurePoints, $constructorImpurePoints);
9595

9696
if ($parametersAcceptor !== null) {
@@ -136,19 +136,13 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
136136
}
137137
} else {
138138
$nodeScopeResolver->processStmtNode($expr->class, $scope, $storage, $nodeCallback, StatementContext::createTopLevel());
139-
$declaringClass = $constructorReflection->getDeclaringClass();
140-
$constructorThrowPoint = $this->getConstructorThrowPoint($constructorReflection, $parametersAcceptor, $classReflection, $expr, new Name\FullyQualified($declaringClass->getName()), $expr->getArgs(), $scope);
141-
if ($constructorThrowPoint !== null) {
142-
$throwPoints[] = $constructorThrowPoint;
143-
}
144-
145139
if (!$constructorReflection->hasSideEffects()->no()) {
146140
$certain = $constructorReflection->isPure()->no();
147141
$impurePoints[] = new ImpurePoint(
148142
$scope,
149143
$expr,
150144
'new',
151-
sprintf('instantiation of class %s', $declaringClass->getDisplayName()),
145+
sprintf('instantiation of class %s', $constructorReflection->getDeclaringClass()->getDisplayName()),
152146
$certain,
153147
);
154148
}
@@ -176,11 +170,9 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
176170
$throwPoints = array_merge($throwPoints, $additionalThrowPoints);
177171

178172
if ($className !== null) {
179-
[$constructorReflection, $parametersAcceptor, $constructorThrowPoints, $constructorImpurePoints] = $this->processConstructorReflection($className, $expr, $scope);
180-
$throwPoints = array_merge($throwPoints, $constructorThrowPoints);
173+
[$constructorReflection, $classReflection, $parametersAcceptor, $constructorImpurePoints] = $this->processConstructorReflection($className, $expr, $scope);
181174
$impurePoints = array_merge($impurePoints, $constructorImpurePoints);
182175
} else {
183-
$throwPoints[] = InternalThrowPoint::createImplicit($scope, $expr);
184176
$impurePoints[] = new ImpurePoint(
185177
$scope,
186178
$expr,
@@ -202,6 +194,16 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
202194
$impurePoints = array_merge($impurePoints, $argsResult->getImpurePoints());
203195
$isAlwaysTerminating = $isAlwaysTerminating || $argsResult->isAlwaysTerminating();
204196

197+
if ($constructorReflection !== null && $parametersAcceptor !== null) {
198+
$className ??= $constructorReflection->getDeclaringClass()->getName();
199+
$constructorThrowPoint = $this->getConstructorThrowPoint($constructorReflection, $parametersAcceptor, $expr, new Name\FullyQualified($className), $expr->getArgs(), $scope);
200+
if ($constructorThrowPoint !== null) {
201+
$throwPoints[] = $constructorThrowPoint;
202+
}
203+
} elseif ($classReflection === null) {
204+
$throwPoints[] = InternalThrowPoint::createImplicit($scope, $expr);
205+
}
206+
205207
return new ExpressionResult(
206208
$scope,
207209
hasYield: $hasYield,
@@ -212,13 +214,12 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
212214
}
213215

214216
/**
215-
* @return array{?MethodReflection, ?ParametersAcceptor, InternalThrowPoint[], ImpurePoint[]}
217+
* @return array{?MethodReflection, ?ClassReflection, ?ParametersAcceptor, ImpurePoint[]}
216218
*/
217219
private function processConstructorReflection(string $className, New_ $expr, MutatingScope $scope): array
218220
{
219221
$constructorReflection = null;
220222
$parametersAcceptor = null;
221-
$throwPoints = [];
222223
$impurePoints = [];
223224

224225
$classReflection = null;
@@ -232,13 +233,7 @@ private function processConstructorReflection(string $className, New_ $expr, Mut
232233
$constructorReflection->getVariants(),
233234
$constructorReflection->getNamedArgumentsVariants(),
234235
);
235-
$constructorThrowPoint = $this->getConstructorThrowPoint($constructorReflection, $parametersAcceptor, $classReflection, $expr, new Name\FullyQualified($className), $expr->getArgs(), $scope);
236-
if ($constructorThrowPoint !== null) {
237-
$throwPoints[] = $constructorThrowPoint;
238-
}
239236
}
240-
} else {
241-
$throwPoints[] = InternalThrowPoint::createImplicit($scope, $expr);
242237
}
243238

244239
if ($constructorReflection !== null) {
@@ -262,13 +257,13 @@ private function processConstructorReflection(string $className, New_ $expr, Mut
262257
);
263258
}
264259

265-
return [$constructorReflection, $parametersAcceptor, $throwPoints, $impurePoints];
260+
return [$constructorReflection, $classReflection, $parametersAcceptor, $impurePoints];
266261
}
267262

268263
/**
269264
* @param list<Node\Arg> $args
270265
*/
271-
private function getConstructorThrowPoint(MethodReflection $constructorReflection, ParametersAcceptor $parametersAcceptor, ClassReflection $classReflection, New_ $new, Name $className, array $args, MutatingScope $scope): ?InternalThrowPoint
266+
private function getConstructorThrowPoint(MethodReflection $constructorReflection, ParametersAcceptor $parametersAcceptor, New_ $new, Name $className, array $args, MutatingScope $scope): ?InternalThrowPoint
272267
{
273268
$methodCall = new StaticCall($className, $constructorReflection->getName(), $args);
274269
$normalizedMethodCall = ArgumentsNormalizer::reorderStaticCallArguments($parametersAcceptor, $methodCall);
@@ -293,7 +288,7 @@ private function getConstructorThrowPoint(MethodReflection $constructorReflectio
293288
return InternalThrowPoint::createExplicit($scope, $throwType, $new, true);
294289
}
295290
} elseif ($this->implicitThrows) {
296-
if (!$classReflection->is(Throwable::class)) {
291+
if (!$constructorReflection->getDeclaringClass()->is(Throwable::class)) {
297292
return InternalThrowPoint::createImplicit($scope, $methodCall);
298293
}
299294
}

‎src/Analyser/InternalScopeFactory.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ interface InternalScopeFactory
1515
* @param array<string, ExpressionTypeHolder> $expressionTypes
1616
* @param array<string, ExpressionTypeHolder> $nativeExpressionTypes
1717
* @param array<string, ConditionalExpressionHolder[]> $conditionalExpressions
18-
* @param list<string> $inClosureBindScopeClasses
18+
* @param list<non-empty-string> $inClosureBindScopeClasses
1919
* @param array<string, true> $currentlyAssignedExpressions
2020
* @param array<string, true> $currentlyAllowedUndefinedExpressions
2121
* @param list<array{FunctionReflection|MethodReflection|null, ParameterReflection|null}> $inFunctionCallsStack

‎src/Analyser/MutatingScope.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ class MutatingScope implements Scope, NodeCallbackInvoker
164164
* @param callable(Node $node, Scope $scope): void|null $nodeCallback
165165
* @param array<string, ExpressionTypeHolder> $expressionTypes
166166
* @param array<string, ConditionalExpressionHolder[]> $conditionalExpressions
167-
* @param list<string> $inClosureBindScopeClasses
167+
* @param list<non-empty-string> $inClosureBindScopeClasses
168168
* @param array<string, true> $currentlyAssignedExpressions
169169
* @param array<string, true> $currentlyAllowedUndefinedExpressions
170170
* @param array<string, ExpressionTypeHolder> $nativeExpressionTypes
@@ -1822,7 +1822,7 @@ public function enterNamespace(string $namespaceName): self
18221822
}
18231823

18241824
/**
1825-
* @param list<string> $scopeClasses
1825+
* @param list<non-empty-string> $scopeClasses
18261826
*/
18271827
public function enterClosureBind(?Type $thisType, ?Type $nativeThisType, array $scopeClasses): self
18281828
{

‎src/Analyser/NodeScopeResolver.php‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1878,7 +1878,6 @@ public function processStmtNode(
18781878

18791879
// explicit only
18801880
$onlyExplicitIsThrow = true;
1881-
$hasDirectExplicitNonThrowMatch = false;
18821881
if (count($matchingThrowPoints) === 0) {
18831882
foreach ($throwPoints as $throwPointIndex => $throwPoint) {
18841883
foreach ($catchTypes as $catchTypeIndex => $catchTypeItem) {
@@ -1896,17 +1895,15 @@ public function processStmtNode(
18961895
&& !($throwNode instanceof Node\Stmt\Expression && $throwNode->expr instanceof Expr\Throw_)
18971896
) {
18981897
$onlyExplicitIsThrow = false;
1899-
if ($catchTypeItem->isSuperTypeOf($throwPoint->getType())->yes()) {
1900-
$hasDirectExplicitNonThrowMatch = true;
1901-
}
19021898
}
1899+
19031900
$matchingThrowPoints[$throwPointIndex] = $throwPoint;
19041901
}
19051902
}
19061903
}
19071904

19081905
// implicit only
1909-
if (count($matchingThrowPoints) === 0 || $onlyExplicitIsThrow || !$hasDirectExplicitNonThrowMatch) {
1906+
if (count($matchingThrowPoints) === 0 || $onlyExplicitIsThrow) {
19101907
foreach ($throwPoints as $throwPointIndex => $throwPoint) {
19111908
if ($throwPoint->isExplicit()) {
19121909
continue;

‎src/Analyser/Scope.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,8 @@ public function getScopeNativeType(Expr $expr): Type;
173173
* (they should already be fully qualified by the PHP parser's name resolver).
174174
*
175175
* Inside a Closure::bind() context, `self`/`static` resolve to the bound class.
176+
*
177+
* @return non-empty-string
176178
*/
177179
public function resolveName(Name $name): string;
178180

‎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+
24,
1446+
],
1447+
[
1448+
'Variable $command might not be defined.',
1449+
50,
1450+
],
1451+
[
1452+
'Variable $command might not be defined.',
1453+
119,
1454+
],
1455+
]);
1456+
}
1457+
14351458
#[RequiresPhp('>= 8.0')]
14361459
public function testBug14274(): void
14371460
{

0 commit comments

Comments
 (0)