Skip to content

Commit 2bdcfe6

Browse files
ChrisAbnerChrisAbner
committed
Check only the right side of ??= as the value written to its target
`$x ??= $v` evaluates to `$x ?? $v`, but only ever writes `$v`. The nodes rules use to check the write (PropertyAssignNode, VariableAssignNode and the innermost value of the Set*OffsetValueTypeExpr chain) now carry `$v` when the target is the left side of the `??=`, while the scope update keeps `$x ?? $v`. The write is also conditional, so a by-ref parameter keeps its original-value marker with Maybe certainty after `$p ??= ...` when the left side may be non-null, and ParameterOutExecutionEndTypeRule still checks it at exit. Ported to the turbo-ext mirrors of AssignHandler and AssignOpHandler.
1 parent 49861be commit 2bdcfe6

12 files changed

Lines changed: 532 additions & 165 deletions

File tree

‎src/Analyser/ExprHandler/AssignHandler.php‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -760,7 +760,7 @@ private function doPrepareTarget(
760760
// 1b. build the write chain (Set*OffsetValueTypeExpr nesting) after
761761
// the root walk, so a property base's holder and current types are
762762
// read from its stored result instead of pricing the unwalked fetch
763-
$assignedPropertyExpr = $assignedExpr;
763+
$assignedPropertyExpr = $this->getWrittenExpr($originalVar, $assignedExpr);
764764
$chainVar = $originalVar;
765765
while ($chainVar instanceof ArrayDimFetch) {
766766
$varForSetOffsetValue = $chainVar->var;
@@ -1374,7 +1374,7 @@ public function applyWrite(
13741374
$conditionalExpressions = $this->processSureNotTypesForConditionalExpressionsAfterAssign($nodeScopeResolver, $scope, $storage, $var->name, $conditionalExpressions, $identicalSpecifiedTypes, $falseyType, $impurePoints, $assignedExpr, $storedAssignedExprResult);
13751375
}
13761376

1377-
$nodeScopeResolver->callNodeCallback($nodeCallback, new VariableAssignNode($var, $assignedExpr), $scopeBeforeAssignEval, $storage);
1377+
$nodeScopeResolver->callNodeCallback($nodeCallback, new VariableAssignNode($var, $this->getWrittenExpr($var, $assignedExpr)), $scopeBeforeAssignEval, $storage);
13781378

13791379
$nativeType = $this->readAssignedValueType($nodeScopeResolver, $storedAssignedExprResult, $assignedExpr, $scope->doNotTreatPhpDocTypesAsCertain());
13801380
$scope = $scope->assignVariable(
@@ -1596,7 +1596,7 @@ public function applyWrite(
15961596
if ($templateArgumentFrame !== null) {
15971597
$scope = $scope->addTemplateArgumentConstraints($this->templateArgumentObserver->collectSend($propertyReflection->getWritableType(), $assignedExprType));
15981598
}
1599-
$nodeScopeResolver->callNodeCallback($nodeCallback, new PropertyAssignNode($var, $assignedExpr, $isAssignOp), $scopeBeforeAssignEval, $storage);
1599+
$nodeScopeResolver->callNodeCallback($nodeCallback, new PropertyAssignNode($var, $this->getWrittenExpr($var, $assignedExpr), $isAssignOp), $scopeBeforeAssignEval, $storage);
16001600
if ($propertyReflection->canChangeTypeAfterAssignment()) {
16011601
if ($propertyReflection->hasNativeType()) {
16021602
$propertyNativeType = $propertyReflection->getNativeType();
@@ -1657,7 +1657,7 @@ public function applyWrite(
16571657
} else {
16581658
// fallback
16591659
$assignedExprType = $this->readAssignedValueType($nodeScopeResolver, $assignedValueResult, $assignedExpr, $scope);
1660-
$nodeScopeResolver->callNodeCallback($nodeCallback, new PropertyAssignNode($var, $assignedExpr, $isAssignOp), $scopeBeforeAssignEval, $storage);
1660+
$nodeScopeResolver->callNodeCallback($nodeCallback, new PropertyAssignNode($var, $this->getWrittenExpr($var, $assignedExpr), $isAssignOp), $scopeBeforeAssignEval, $storage);
16611661
$scope = $scope->assignExpression($var, $assignedExprType, $this->readAssignedValueType($nodeScopeResolver, $assignedValueResult, $assignedExpr, $scope->doNotTreatPhpDocTypesAsCertain()));
16621662
// simulate dynamic property assign by __set to get throw points
16631663
if (!$propertyHolderType->hasMethod('__set')->no()) {
@@ -1693,7 +1693,7 @@ public function applyWrite(
16931693
$scope = $scope->addTemplateArgumentConstraints($this->templateArgumentObserver->collectSend($propertyReflection->getWritableType(), $assignedExprType));
16941694
}
16951695
}
1696-
$nodeScopeResolver->callNodeCallback($nodeCallback, new PropertyAssignNode($var, $assignedExpr, $isAssignOp), $scopeBeforeAssignEval, $storage);
1696+
$nodeScopeResolver->callNodeCallback($nodeCallback, new PropertyAssignNode($var, $this->getWrittenExpr($var, $assignedExpr), $isAssignOp), $scopeBeforeAssignEval, $storage);
16971697
if ($propertyReflection !== null && $propertyReflection->canChangeTypeAfterAssignment()) {
16981698
if ($propertyReflection->hasNativeType()) {
16991699
$propertyNativeType = $propertyReflection->getNativeType();
@@ -1724,7 +1724,7 @@ public function applyWrite(
17241724
} else {
17251725
// fallback
17261726
$assignedExprType = $this->readAssignedValueType($nodeScopeResolver, $assignedValueResult, $assignedExpr, $scope);
1727-
$nodeScopeResolver->callNodeCallback($nodeCallback, new PropertyAssignNode($var, $assignedExpr, $isAssignOp), $scopeBeforeAssignEval, $storage);
1727+
$nodeScopeResolver->callNodeCallback($nodeCallback, new PropertyAssignNode($var, $this->getWrittenExpr($var, $assignedExpr), $isAssignOp), $scopeBeforeAssignEval, $storage);
17281728
$scope = $scope->assignExpression($var, $assignedExprType, $this->readAssignedValueType($nodeScopeResolver, $assignedValueResult, $assignedExpr, $scope->doNotTreatPhpDocTypesAsCertain()));
17291729
}
17301730
} elseif ($kind === PreparedAssignTarget::KIND_LIST) {
@@ -2049,6 +2049,18 @@ private function assignDynamicVariable(MutatingScope $scope, ExpressionResult $n
20492049
return $scope;
20502050
}
20512051

2052+
/**
2053+
* `$var ??= $value` evaluates to `$var ?? $value`, but writes only `$value` to `$var`.
2054+
*/
2055+
private function getWrittenExpr(Expr $var, Expr $assignedExpr): Expr
2056+
{
2057+
if ($assignedExpr instanceof Expr\AssignOp\Coalesce && $assignedExpr->var === $var) {
2058+
return $assignedExpr->expr;
2059+
}
2060+
2061+
return $assignedExpr;
2062+
}
2063+
20522064
private function unwrapAssign(Expr $expr): Expr
20532065
{
20542066
if ($expr instanceof Assign) {

‎src/Analyser/ExprHandler/AssignOpHandler.php‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,11 @@
2626
use PHPStan\Dependency\Dependencies;
2727
use PHPStan\DependencyInjection\AutowiredService;
2828
use PHPStan\Node\CoalesceExpressionNode;
29+
use PHPStan\Node\Expr\ParameterVariableOriginalValueExpr;
2930
use PHPStan\Node\Variable\VariableWrite;
3031
use PHPStan\Reflection\InitializerExprTypeResolver;
3132
use PHPStan\ShouldNotHappenException;
33+
use PHPStan\TrinaryLogic;
3234
use PHPStan\Turbo\ShadowedByTurboExtension;
3335
use PHPStan\Type\Constant\ConstantIntegerType;
3436
use PHPStan\Type\MixedType;
@@ -271,6 +273,29 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
271273
$context,
272274
);
273275
$scope = $assignResult->getScope();
276+
if ($expr instanceof Expr\AssignOp\Coalesce && $expr->var instanceof Expr\Variable && is_string($expr->var->name)) {
277+
if ($condResult === null) {
278+
throw new ShouldNotHappenException();
279+
}
280+
281+
// the write only happens when the left side is null - a by-ref
282+
// parameter may still hold its original value afterwards (a
283+
// closure's copy of an outer parameter carries no native marker)
284+
$originalValueExpr = new ParameterVariableOriginalValueExpr($expr->var->name);
285+
$nativeValueBeforeScope = $valueBeforeScope->doNotTreatPhpDocTypesAsCertain();
286+
if (
287+
!$condResult->getType()->isNull()->yes()
288+
&& !$valueBeforeScope->hasExpressionType($originalValueExpr)->no()
289+
&& !$nativeValueBeforeScope->hasExpressionType($originalValueExpr)->no()
290+
) {
291+
$scope = $scope->specifyExpressionType(
292+
$originalValueExpr,
293+
$valueBeforeScope->getTrackedExpressionType($originalValueExpr),
294+
$nativeValueBeforeScope->getTrackedExpressionType($originalValueExpr),
295+
TrinaryLogic::createMaybe(),
296+
);
297+
}
298+
}
274299
$throwPoints = $assignResult->getThrowPoints();
275300
$impurePoints = $assignResult->getImpurePoints();
276301
if (

‎tests/PHPStan/Rules/Properties/TypesAssignedToPropertiesRuleTest.php‎

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ class TypesAssignedToPropertiesRuleTest extends RuleTestCase
1515

1616
private bool $checkNullables = true;
1717

18+
private bool $checkUnionTypes = true;
19+
1820
private bool $checkExplicitMixed = false;
1921

2022
private bool $checkImplicitMixed = false;
@@ -26,7 +28,7 @@ protected function getRule(): Rule
2628
self::createReflectionProvider(),
2729
checkNullables: $this->checkNullables,
2830
checkThisOnly: false,
29-
checkUnionTypes: true,
31+
checkUnionTypes: $this->checkUnionTypes,
3032
checkExplicitMixed: $this->checkExplicitMixed,
3133
checkImplicitMixed: $this->checkImplicitMixed,
3234
checkBenevolentUnionTypes: false,
@@ -1131,6 +1133,58 @@ public function testBug9096(): void
11311133
$this->analyse([__DIR__ . '/data/bug-9096.php'], []);
11321134
}
11331135

1136+
public function testBug15228(): void
1137+
{
1138+
$this->analyse([__DIR__ . '/data/bug-15228.php'], $this->getBug15228Errors());
1139+
}
1140+
1141+
public function testBug15228WithoutUnionTypes(): void
1142+
{
1143+
$this->checkUnionTypes = false;
1144+
$this->analyse([__DIR__ . '/data/bug-15228.php'], $this->getBug15228Errors());
1145+
}
1146+
1147+
/**
1148+
* @return list<array{0: string, 1: int, 2?: string|null}>
1149+
*/
1150+
private function getBug15228Errors(): array
1151+
{
1152+
return [
1153+
[
1154+
'Property Bug15228\\CoalesceAssignDemo::$a (int|null) does not accept string.',
1155+
26,
1156+
],
1157+
[
1158+
'Property Bug15228\\CoalesceAssignDemo::$a (int|null) does not accept string.',
1159+
32,
1160+
],
1161+
[
1162+
'Property Bug15228\\CoalesceAssignDemo::$b (list<int>|null) does not accept list<string>.',
1163+
41,
1164+
],
1165+
[
1166+
'Property Bug15228\\CoalesceAssignDemo::$b (list<int>|null) does not accept list<string>.',
1167+
47,
1168+
],
1169+
[
1170+
'Static property Bug15228\\Adjacent::$s (int|null) does not accept string.',
1171+
64,
1172+
],
1173+
[
1174+
'Property Bug15228\\Adjacent::$a (int|null) does not accept string.',
1175+
70,
1176+
],
1177+
[
1178+
'Property Bug15228\\Adjacent::$uninitialized (int) does not accept string.',
1179+
75,
1180+
],
1181+
[
1182+
'Property Bug15228\\Offsets::$m (array<string, int>) does not accept array<string, int|string>.',
1183+
102,
1184+
],
1185+
];
1186+
}
1187+
11341188
public function testIntersectionWithTemplateMixed(): void
11351189
{
11361190
$this->analyse([__DIR__ . '/data/intersection-template-mixed-property.php'], [

‎tests/PHPStan/Rules/Properties/data/bug-15228.php‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug15228;
4+
5+
use function PHPStan\dumpType;
6+
use function PHPStan\Testing\assertType;
7+
8+
class CoalesceAssignDemo
9+
{
10+
public ?int $a = null;
11+
12+
/** @var list<int>|null */
13+
public $b;
14+
15+
/** @return list<string> */
16+
private function strings(): array
17+
{
18+
return ['x'];
19+
}
20+
21+
// === Symptom 1: false negative below level 7 ===
22+
// Both methods write a string into ?int - a guaranteed TypeError.
23+
24+
public function coalesceInvalid(): void
25+
{
26+
$this->a ??= 'boom'; // (NOT reported at levels 0-6.)
27+
}
28+
29+
public function ifInvalid(): void
30+
{
31+
if (null === $this->a) {
32+
$this->a = 'boom'; // (Reported from level 0. Correct.)
33+
}
34+
}
35+
36+
// === Symptom 2: false positive at level 7+ ===
37+
// Both methods write list<int>, which the property accepts.
38+
39+
public function coalesceValid(): void
40+
{
41+
$this->b = $this->strings(); // (Genuine error, both methods.)
42+
$this->b ??= [1, 2]; // VALID write - reported at 7+ anyway.
43+
}
44+
45+
public function ifValid(): void
46+
{
47+
$this->b = $this->strings(); // (Genuine error, both methods.)
48+
if (null === $this->b) {
49+
$this->b = [1, 2]; // VALID write - correctly not reported.
50+
}
51+
}
52+
}
53+
54+
class Adjacent
55+
{
56+
public static ?int $s = null;
57+
58+
public ?int $a = null;
59+
60+
public int $uninitialized;
61+
62+
public function staticProperty(): void
63+
{
64+
self::$s ??= 'boom';
65+
}
66+
67+
public function neverNull(): void
68+
{
69+
$this->a = 1;
70+
$this->a ??= 'never written';
71+
}
72+
73+
public function maybeUninitialized(): void
74+
{
75+
$this->uninitialized ??= 'boom';
76+
}
77+
78+
public function valid(): void
79+
{
80+
$this->a ??= 5;
81+
self::$s ??= 5;
82+
}
83+
}
84+
85+
class RightSideNarrowing
86+
{
87+
public ?string $q = null;
88+
89+
public function rightSideSeesNullLeftSide(): void
90+
{
91+
$this->q ??= $this->q === null ? 'x' : 1;
92+
}
93+
}
94+
95+
class Offsets
96+
{
97+
/** @var array<string, int> */
98+
public array $m = [];
99+
100+
public function invalid(): void
101+
{
102+
$this->m['k'] ??= 'boom';
103+
}
104+
105+
public function valid(): void
106+
{
107+
$this->m['k'] ??= 5;
108+
}
109+
}

‎tests/PHPStan/Rules/TooWideTypehints/TooWidePropertyTypeRuleTest.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,4 +144,15 @@ public function testBug11844(): void
144144
$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-11844.php'], []);
145145
}
146146

147+
#[RequiresPhp('>= 8.0.0')]
148+
public function testBug15228(): void
149+
{
150+
$this->analyse([__DIR__ . '/data/bug-15228.php'], [
151+
[
152+
'Property Bug15228\\OnlyCoalesceAssigned::$x (int|string|null) is never assigned string so it can be removed from the property type.',
153+
9,
154+
],
155+
]);
156+
}
157+
147158
}

‎tests/PHPStan/Rules/TooWideTypehints/data/bug-15228.php‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
<?php // lint >= 8.0
2+
3+
declare(strict_types = 1);
4+
5+
namespace Bug15228;
6+
7+
final class OnlyCoalesceAssigned
8+
{
9+
private int|string|null $x = null;
10+
11+
public function set(): void
12+
{
13+
$this->x ??= 1;
14+
}
15+
16+
public function get(): int|string|null
17+
{
18+
return $this->x;
19+
}
20+
}

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,4 +145,19 @@ public function testBug15428(): void
145145
$this->analyse([__DIR__ . '/data/bug-15428.php'], []);
146146
}
147147

148+
#[RequiresPhp('>= 8.0.0')]
149+
public function testBug15228(): void
150+
{
151+
$this->analyse([__DIR__ . '/data/bug-15228.php'], [
152+
[
153+
'Parameter &$p @param-out type of function Bug15228\\writesString() expects int, string given.',
154+
16,
155+
],
156+
[
157+
'Parameter &$p @param-out type of function Bug15228\\validWrite() expects list<int>|null, list<string> given.',
158+
22,
159+
],
160+
]);
161+
}
162+
148163
}

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,4 +86,23 @@ public function testBug15066(): void
8686
]);
8787
}
8888

89+
#[RequiresPhp('>= 8.0.0')]
90+
public function testBug15228(): void
91+
{
92+
$this->analyse([__DIR__ . '/data/bug-15228.php'], [
93+
[
94+
'Parameter &$p @param-out type of function Bug15228\\writesString() expects int, int|string given.',
95+
16,
96+
],
97+
[
98+
'Parameter &$p @param-out type of function Bug15228\\keepsOriginalValue() expects string, int|string given.',
99+
29,
100+
],
101+
[
102+
'Parameter &$p @param-out type of function Bug15228\\keepsOriginalValueTwice() expects string, int|string given.',
103+
36,
104+
],
105+
]);
106+
}
107+
89108
}

0 commit comments

Comments
 (0)