Skip to content

Commit d445074

Browse files
staabmphpstan-bot
authored andcommitted
Fix spurious nullCoalesce.offset error after nested ?? with nullable types
- Fixed false positive where `$x[0] ?? null` was flagged as unnecessary after `$x[0]['bar'] ?? null` when the inner type was nullable (e.g. list<array<?string>>) - Root cause: revertNonNullability used specifyExpressionType which recursively creates expression type holders for parent arrays as side effects, leaving spurious Yes-certainty expression types for intermediate array dim fetches - Added sideEffectSave flag to EnsuredNonNullabilityResultExpression to distinguish target saves (null actually removed) from side-effect saves (type changed by recursive parent narrowing) - After reverting, unset expression types for side-effect saves that didn't exist in the original scope - Added unsetExpressionType method to MutatingScope - New regression test in tests/PHPStan/Rules/Variables/data/bug-13921.php Closes phpstan/phpstan#13921
1 parent 1b3396d commit d445074

File tree

5 files changed

+73
-4
lines changed

5 files changed

+73
-4
lines changed

src/Analyser/EnsuredNonNullabilityResultExpression.php

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ public function __construct(
1414
private Type $originalType,
1515
private Type $originalNativeType,
1616
private TrinaryLogic $certainty,
17+
private bool $existedInOriginalScope = true,
18+
private bool $sideEffectSave = false,
1719
)
1820
{
1921
}
@@ -38,4 +40,14 @@ public function getCertainty(): TrinaryLogic
3840
return $this->certainty;
3941
}
4042

43+
public function existedInOriginalScope(): bool
44+
{
45+
return $this->existedInOriginalScope;
46+
}
47+
48+
public function isSideEffectSave(): bool
49+
{
50+
return $this->sideEffectSave;
51+
}
52+
4153
}

src/Analyser/MutatingScope.php

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3547,6 +3547,34 @@ public function specifyExpressionType(Expr $expr, Type $type, Type $nativeType,
35473547
return $scope;
35483548
}
35493549

3550+
public function unsetExpressionType(Expr $expr): self
3551+
{
3552+
$exprString = $this->getNodeKey($expr);
3553+
$expressionTypes = $this->expressionTypes;
3554+
$nativeTypes = $this->nativeExpressionTypes;
3555+
unset($expressionTypes[$exprString]);
3556+
unset($nativeTypes[$exprString]);
3557+
3558+
return $this->scopeFactory->create(
3559+
$this->context,
3560+
$this->isDeclareStrictTypes(),
3561+
$this->getFunction(),
3562+
$this->getNamespace(),
3563+
$expressionTypes,
3564+
$nativeTypes,
3565+
$this->conditionalExpressions,
3566+
$this->inClosureBindScopeClasses,
3567+
$this->anonymousFunctionReflection,
3568+
$this->inFirstLevelStatement,
3569+
$this->currentlyAssignedExpressions,
3570+
$this->currentlyAllowedUndefinedExpressions,
3571+
$this->inFunctionCallsStack,
3572+
$this->afterExtractCall,
3573+
$this->parentScope,
3574+
$this->nativeTypesPromoted,
3575+
);
3576+
}
3577+
35503578
public function assignExpression(Expr $expr, Type $type, Type $nativeType): self
35513579
{
35523580
$scope = $this;

src/Analyser/NodeScopeResolver.php

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2473,7 +2473,8 @@ private function ensureShallowNonNullability(MutatingScope $scope, Scope $origin
24732473
// keep certainty
24742474
$certainty = TrinaryLogic::createYes();
24752475
$hasExpressionType = $originalScope->hasExpressionType($exprToSpecify);
2476-
if (!$hasExpressionType->no()) {
2476+
$existedInOriginalScope = !$hasExpressionType->no();
2477+
if ($existedInOriginalScope) {
24772478
$certainty = $hasExpressionType;
24782479
}
24792480

@@ -2484,7 +2485,7 @@ private function ensureShallowNonNullability(MutatingScope $scope, Scope $origin
24842485
$originalNativeType = $originalScope->getNativeType($exprToSpecify);
24852486

24862487
return new EnsuredNonNullabilityResult($scope, [
2487-
new EnsuredNonNullabilityResultExpression($exprToSpecify, $originalExprType, $originalNativeType, $certainty),
2488+
new EnsuredNonNullabilityResultExpression($exprToSpecify, $originalExprType, $originalNativeType, $certainty, $existedInOriginalScope, true),
24882489
]);
24892490
}
24902491
return new EnsuredNonNullabilityResult($scope, []);
@@ -2493,7 +2494,7 @@ private function ensureShallowNonNullability(MutatingScope $scope, Scope $origin
24932494
$nativeType = $scope->getNativeType($exprToSpecify);
24942495

24952496
$specifiedExpressions = [
2496-
new EnsuredNonNullabilityResultExpression($exprToSpecify, $exprType, $nativeType, $certainty),
2497+
new EnsuredNonNullabilityResultExpression($exprToSpecify, $exprType, $nativeType, $certainty, $existedInOriginalScope),
24972498
];
24982499

24992500
// When narrowing an ArrayDimFetch, specifyExpressionType also recursively
@@ -2503,14 +2504,17 @@ private function ensureShallowNonNullability(MutatingScope $scope, Scope $origin
25032504
$parentExpr = $exprToSpecify->var;
25042505
$parentCertainty = TrinaryLogic::createYes();
25052506
$hasParentExpressionType = $originalScope->hasExpressionType($parentExpr);
2506-
if (!$hasParentExpressionType->no()) {
2507+
$parentExistedInOriginalScope = !$hasParentExpressionType->no();
2508+
if ($parentExistedInOriginalScope) {
25072509
$parentCertainty = $hasParentExpressionType;
25082510
}
25092511
array_unshift($specifiedExpressions, new EnsuredNonNullabilityResultExpression(
25102512
$parentExpr,
25112513
$scope->getType($parentExpr),
25122514
$scope->getNativeType($parentExpr),
25132515
$parentCertainty,
2516+
$parentExistedInOriginalScope,
2517+
true,
25142518
));
25152519
}
25162520

@@ -2556,6 +2560,17 @@ private function revertNonNullability(MutatingScope $scope, array $specifiedExpr
25562560
);
25572561
}
25582562

2563+
// specifyExpressionType recursively narrows parent array types as a side effect,
2564+
// which can introduce expression type holders that didn't exist in the original scope.
2565+
// Clean up side-effect saves that shouldn't have expression types.
2566+
foreach ($specifiedExpressions as $specifiedExpressionResult) {
2567+
if (!$specifiedExpressionResult->isSideEffectSave() || $specifiedExpressionResult->existedInOriginalScope()) {
2568+
continue;
2569+
}
2570+
2571+
$scope = $scope->unsetExpressionType($specifiedExpressionResult->getExpression());
2572+
}
2573+
25592574
return $scope;
25602575
}
25612576

tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,4 +362,9 @@ public function testBug14213(): void
362362
]);
363363
}
364364

365+
public function testBug13921(): void
366+
{
367+
$this->analyse([__DIR__ . '/data/bug-13921.php'], []);
368+
}
369+
365370
}
Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug13921;
4+
5+
/** @param list<array<?string>> $x */
6+
function foo(array $x): void {
7+
var_dump($x[0]['bar'] ?? null);
8+
var_dump($x[0] ?? null);
9+
}

0 commit comments

Comments
 (0)