Skip to content

Commit 9b0cede

Browse files
phpstan-botclaude
phpstan-bot
and
claude
authored
Do not report ?? null / ??= null as unnecessary when evaluating the right side has side effects (#6418)
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent a88f398 commit 9b0cede

7 files changed

Lines changed: 284 additions & 2 deletions

File tree

‎src/Analyser/ExprHandler/AssignOpHandler.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
139139
}
140140

141141
if ($condResult !== null) {
142-
$nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new CoalesceExpressionNode($expr, $condResult, 'on left side of ??='), $beforeScope, $storage, $context);
142+
$nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new CoalesceExpressionNode($expr, $condResult, $valueResult, 'on left side of ??='), $beforeScope, $storage, $context);
143143
}
144144

145145
return $this->expressionResultFactory->create(

‎src/Analyser/ExprHandler/CoalesceHandler.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
136136
$scope = $scope->filterByTruthyValue(new Expr\Isset_([$expr->left]))->mergeWith($rightResult->getScope());
137137
}
138138

139-
$nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new CoalesceExpressionNode($expr, $condResult, 'on left side of ??'), $beforeScope, $storage, $context);
139+
$nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new CoalesceExpressionNode($expr, $condResult, $rightResult, 'on left side of ??'), $beforeScope, $storage, $context);
140140

141141
return $this->expressionResultFactory->create(
142142
$scope,

‎src/Node/CoalesceExpressionNode.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ final class CoalesceExpressionNode extends NodeAbstract implements VirtualNode
2121
public function __construct(
2222
private Expr $originalExpr,
2323
private ExpressionResult $subjectResult,
24+
private ExpressionResult $rightResult,
2425
private string $operatorDescription,
2526
)
2627
{
@@ -37,6 +38,12 @@ public function getSubjectResult(): ExpressionResult
3738
return $this->subjectResult;
3839
}
3940

41+
/** Result of the right side - the operand that's only evaluated when the left side is null. */
42+
public function getRightResult(): ExpressionResult
43+
{
44+
return $this->rightResult;
45+
}
46+
4047
public function getOperatorDescription(): string
4148
{
4249
return $this->operatorDescription;

‎src/Rules/Variables/NullCoalesceRule.php‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@
33
namespace PHPStan\Rules\Variables;
44

55
use PhpParser\Node;
6+
use PhpParser\NodeFinder;
67
use PHPStan\Analyser\CollectedDataEmitter;
8+
use PHPStan\Analyser\InternalThrowPoint;
79
use PHPStan\Analyser\NodeCallbackInvoker;
810
use PHPStan\Analyser\Scope;
911
use PHPStan\DependencyInjection\AutowiredParameter;
@@ -15,6 +17,7 @@
1517
use PHPStan\Rules\Rule;
1618
use PHPStan\Rules\RuleErrorBuilder;
1719
use PHPStan\Type\Type;
20+
use function array_filter;
1821
use function sprintf;
1922

2023
/**
@@ -96,6 +99,20 @@ private function checkUnnecessaryNullCoalesce(CoalesceExpressionNode $node, Scop
9699
return null;
97100
}
98101

102+
// Dropping the coalesce also drops the evaluation of the right side, which
103+
// only happens when the left side is null. That's only observationally
104+
// equivalent when evaluating the right side cannot do anything but produce
105+
// its null value.
106+
$rightResult = $node->getRightResult();
107+
if (
108+
$rightResult->getImpurePoints() !== []
109+
|| array_filter($rightResult->getThrowPoints(), static fn (InternalThrowPoint $throwPoint): bool => $throwPoint->isExplicit()) !== []
110+
|| $rightResult->hasYield()
111+
|| $this->containsAssign($right)
112+
) {
113+
return null;
114+
}
115+
99116
// The coalesce only changes the result when the left side is undefined.
100117
// If the left side is always set, `?? null` (or `??= null`) never changes
101118
// anything, so the whole coalesce is redundant.
@@ -109,4 +126,19 @@ private function checkUnnecessaryNullCoalesce(CoalesceExpressionNode $node, Scop
109126
)->identifier('nullCoalesce.unnecessary')->build();
110127
}
111128

129+
/**
130+
* Writes to variables, properties and offsets are not impure points, but they
131+
* still make the right side worth keeping around.
132+
*/
133+
private function containsAssign(Node\Expr $expr): bool
134+
{
135+
return (new NodeFinder())->findFirst([$expr], static fn (Node $node): bool => $node instanceof Node\Expr\Assign
136+
|| $node instanceof Node\Expr\AssignRef
137+
|| $node instanceof Node\Expr\AssignOp
138+
|| $node instanceof Node\Expr\PostInc
139+
|| $node instanceof Node\Expr\PreInc
140+
|| $node instanceof Node\Expr\PostDec
141+
|| $node instanceof Node\Expr\PreDec) !== null;
142+
}
143+
112144
}

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

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -505,6 +505,41 @@ public function testBug4337(): void
505505
]);
506506
}
507507

508+
public function testBug15134(): void
509+
{
510+
$this->analyse([__DIR__ . '/data/bug-15134.php'], []);
511+
}
512+
513+
public function testUnnecessaryNullCoalesceSideEffects(): void
514+
{
515+
$this->analyse([__DIR__ . '/data/unnecessary-null-coalesce-side-effects.php'], [
516+
[
517+
'Coalesce operator ??= is unnecessary because the left side is always set and the right side is null.',
518+
111,
519+
],
520+
[
521+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
522+
118,
523+
],
524+
[
525+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
526+
123,
527+
],
528+
[
529+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
530+
128,
531+
],
532+
[
533+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
534+
135,
535+
],
536+
[
537+
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
538+
140,
539+
],
540+
]);
541+
}
542+
508543
public function testBug12179(): void
509544
{
510545
$this->analyse([__DIR__ . '/data/bug-12179.php'], [

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
<?php // lint >= 8.2
2+
3+
declare(strict_types = 1);
4+
5+
namespace Bug15134;
6+
7+
use LogicException;
8+
9+
interface Node {}
10+
11+
class A implements Node {}
12+
13+
abstract class Parser {
14+
15+
protected function parseExpressionChild(bool $value): ?Node {
16+
return $this->parseNumber($value)
17+
?? $this->parseSpace($value);
18+
}
19+
20+
abstract protected function parseNumber(bool $value): ?A;
21+
22+
protected function parseSpace(bool $value): null {
23+
if ($value === false) {
24+
throw new LogicException('The string is not a mathematical expression.');
25+
}
26+
27+
return null;
28+
}
29+
30+
}

‎tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php‎

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,178 @@
1+
<?php
2+
3+
declare(strict_types = 1);
4+
5+
namespace UnnecessaryNullCoalesceSideEffects;
6+
7+
use LogicException;
8+
9+
const NULL_CONSTANT = null;
10+
11+
/** @return null */
12+
function returnsNull(bool $value)
13+
{
14+
if ($value === false) {
15+
throw new LogicException('nope');
16+
}
17+
18+
return null;
19+
}
20+
21+
/**
22+
* @phpstan-pure
23+
* @return null
24+
*/
25+
function pureReturnsNull()
26+
{
27+
return null;
28+
}
29+
30+
class Foo
31+
{
32+
33+
public const NULL_CONSTANT = null;
34+
35+
/** @var string|null */
36+
public $stringOrNull = null;
37+
38+
/** @var null */
39+
public $alwaysNull = null;
40+
41+
/** @return null */
42+
public function returnsNull()
43+
{
44+
return null;
45+
}
46+
47+
/** @return null */
48+
public static function staticReturnsNull()
49+
{
50+
return null;
51+
}
52+
53+
/** @return null */
54+
public function __invoke()
55+
{
56+
return null;
57+
}
58+
59+
}
60+
61+
function funcCallOnRightSide(Foo $foo, ?string $name): ?string
62+
{
63+
return $foo->stringOrNull ?? returnsNull($name !== null);
64+
}
65+
66+
function methodCallOnRightSide(Foo $foo): ?string
67+
{
68+
return $foo->stringOrNull ?? $foo->returnsNull();
69+
}
70+
71+
function staticCallOnRightSide(Foo $foo): ?string
72+
{
73+
return $foo->stringOrNull ?? Foo::staticReturnsNull();
74+
}
75+
76+
function invokeOnRightSide(Foo $foo): ?string
77+
{
78+
return $foo->stringOrNull ?? $foo();
79+
}
80+
81+
function closureCallOnRightSide(Foo $foo): ?string
82+
{
83+
$closure = static function () {
84+
echo 'side effect';
85+
86+
return null;
87+
};
88+
89+
return $foo->stringOrNull ?? $closure();
90+
}
91+
92+
function assignOnRightSide(Foo $foo): ?string
93+
{
94+
$result = $foo->stringOrNull ?? $x = null;
95+
echo $x;
96+
97+
return $result;
98+
}
99+
100+
function assignOpOnRightSide(Foo $foo, ?string $name): ?string
101+
{
102+
$x = $name;
103+
$x ??= $foo->returnsNull();
104+
105+
return $x;
106+
}
107+
108+
function assignOpPureOnRightSide(?string $name): ?string
109+
{
110+
$x = $name;
111+
$x ??= null;
112+
113+
return $x;
114+
}
115+
116+
function pureFuncCallOnRightSide(Foo $foo): ?string
117+
{
118+
return $foo->stringOrNull ?? pureReturnsNull();
119+
}
120+
121+
function constantOnRightSide(Foo $foo): ?string
122+
{
123+
return $foo->stringOrNull ?? NULL_CONSTANT;
124+
}
125+
126+
function classConstantOnRightSide(Foo $foo): ?string
127+
{
128+
return $foo->stringOrNull ?? Foo::NULL_CONSTANT;
129+
}
130+
131+
function nullVariableOnRightSide(Foo $foo): ?string
132+
{
133+
$null = null;
134+
135+
return $foo->stringOrNull ?? $null;
136+
}
137+
138+
function nullPropertyOnRightSide(Foo $foo, Foo $bar): ?string
139+
{
140+
return $foo->stringOrNull ?? $bar->alwaysNull;
141+
}
142+
143+
function assignRefOnRightSide(Foo $foo): ?string
144+
{
145+
$null = null;
146+
147+
return $foo->stringOrNull ?? $ref = &$null;
148+
}
149+
150+
/** @param array<int, null> $nulls */
151+
function assignOpInOffsetOnRightSide(Foo $foo, array $nulls, int $i): ?string
152+
{
153+
return $foo->stringOrNull ?? $nulls[$i += 1];
154+
}
155+
156+
/** @param array<int, null> $nulls */
157+
function postIncOnRightSide(Foo $foo, array $nulls, int $i): ?string
158+
{
159+
return $foo->stringOrNull ?? $nulls[$i++];
160+
}
161+
162+
/** @param array<int, null> $nulls */
163+
function preIncOnRightSide(Foo $foo, array $nulls, int $i): ?string
164+
{
165+
return $foo->stringOrNull ?? $nulls[++$i];
166+
}
167+
168+
/** @param array<int, null> $nulls */
169+
function postDecOnRightSide(Foo $foo, array $nulls, int $i): ?string
170+
{
171+
return $foo->stringOrNull ?? $nulls[$i--];
172+
}
173+
174+
/** @param array<int, null> $nulls */
175+
function preDecOnRightSide(Foo $foo, array $nulls, int $i): ?string
176+
{
177+
return $foo->stringOrNull ?? $nulls[--$i];
178+
}

0 commit comments

Comments
 (0)