From 9fce02f7263c61b7035814ec9069898a00b41124 Mon Sep 17 00:00:00 2001 From: phpstan-bot <79867460+phpstan-bot@users.noreply.github.com> Date: Fri, 11 Sep 2026 13:00:18 +0000 Subject: [PATCH 1/3] Do not report `?? null` / `??= null` as unnecessary when evaluating the right side has side effects * `CoalesceExpressionNode` now also carries the `ExpressionResult` of the right operand, passed in by `CoalesceHandler` (`??`) and `AssignOpHandler` (`??=`). * `NullCoalesceRule::checkUnnecessaryNullCoalesce()` only reports the coalesce as unnecessary when evaluating the right side cannot do anything besides producing its null value: no impure points, no explicit throw points, no yield and no assignment/increment inside it. This mirrors the criterion `ExpressionHandler` already uses to decide whether an expression statement is a noop. * Both operators on the axis are covered: a call returning `null` on the right of `??` and on the right of `??=` are no longer reported. Probed and fixed for function calls, method calls, static calls, `__invoke()` and closure calls, plus plain assignments on the right side (`$a ?? $x = null`), which are not impure points but still change behaviour when the coalesce is deleted. * Side-effect-free right sides (`null`, a `null` constant, a class constant, a `null` variable, a `null`-typed property read, a `@phpstan-pure` call) keep being reported. Co-Authored-By: Claude Opus 5 --- src/Analyser/ExprHandler/AssignOpHandler.php | 2 +- src/Analyser/ExprHandler/CoalesceHandler.php | 2 +- src/Node/CoalesceExpressionNode.php | 7 + src/Rules/Variables/NullCoalesceRule.php | 32 +++++ .../Rules/Variables/NullCoalesceRuleTest.php | 35 +++++ .../Rules/Variables/data/bug-15134.php | 30 ++++ ...unnecessary-null-coalesce-side-effects.php | 134 ++++++++++++++++++ 7 files changed, 240 insertions(+), 2 deletions(-) create mode 100644 tests/PHPStan/Rules/Variables/data/bug-15134.php create mode 100644 tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php diff --git a/src/Analyser/ExprHandler/AssignOpHandler.php b/src/Analyser/ExprHandler/AssignOpHandler.php index 186c792b0b8..4f8e0a92bd2 100644 --- a/src/Analyser/ExprHandler/AssignOpHandler.php +++ b/src/Analyser/ExprHandler/AssignOpHandler.php @@ -139,7 +139,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex } if ($condResult !== null) { - $nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new CoalesceExpressionNode($expr, $condResult, 'on left side of ??='), $beforeScope, $storage, $context); + $nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new CoalesceExpressionNode($expr, $condResult, $valueResult, 'on left side of ??='), $beforeScope, $storage, $context); } return $this->expressionResultFactory->create( diff --git a/src/Analyser/ExprHandler/CoalesceHandler.php b/src/Analyser/ExprHandler/CoalesceHandler.php index c0a54c5bee3..980498eafc2 100644 --- a/src/Analyser/ExprHandler/CoalesceHandler.php +++ b/src/Analyser/ExprHandler/CoalesceHandler.php @@ -136,7 +136,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex $scope = $scope->filterByTruthyValue(new Expr\Isset_([$expr->left]))->mergeWith($rightResult->getScope()); } - $nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new CoalesceExpressionNode($expr, $condResult, 'on left side of ??'), $beforeScope, $storage, $context); + $nodeScopeResolver->callNodeCallbackWithExpression($nodeCallback, new CoalesceExpressionNode($expr, $condResult, $rightResult, 'on left side of ??'), $beforeScope, $storage, $context); return $this->expressionResultFactory->create( $scope, diff --git a/src/Node/CoalesceExpressionNode.php b/src/Node/CoalesceExpressionNode.php index fc5e1ff4942..0431876a21d 100644 --- a/src/Node/CoalesceExpressionNode.php +++ b/src/Node/CoalesceExpressionNode.php @@ -21,6 +21,7 @@ final class CoalesceExpressionNode extends NodeAbstract implements VirtualNode public function __construct( private Expr $originalExpr, private ExpressionResult $subjectResult, + private ExpressionResult $rightResult, private string $operatorDescription, ) { @@ -37,6 +38,12 @@ public function getSubjectResult(): ExpressionResult return $this->subjectResult; } + /** Result of the right side - the operand that's only evaluated when the left side is null. */ + public function getRightResult(): ExpressionResult + { + return $this->rightResult; + } + public function getOperatorDescription(): string { return $this->operatorDescription; diff --git a/src/Rules/Variables/NullCoalesceRule.php b/src/Rules/Variables/NullCoalesceRule.php index 9aff47daa6f..b27ed154ce2 100644 --- a/src/Rules/Variables/NullCoalesceRule.php +++ b/src/Rules/Variables/NullCoalesceRule.php @@ -3,7 +3,9 @@ namespace PHPStan\Rules\Variables; use PhpParser\Node; +use PhpParser\NodeFinder; use PHPStan\Analyser\CollectedDataEmitter; +use PHPStan\Analyser\InternalThrowPoint; use PHPStan\Analyser\NodeCallbackInvoker; use PHPStan\Analyser\Scope; use PHPStan\DependencyInjection\AutowiredParameter; @@ -15,6 +17,7 @@ use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Type\Type; +use function array_filter; use function sprintf; /** @@ -96,6 +99,20 @@ private function checkUnnecessaryNullCoalesce(CoalesceExpressionNode $node, Scop return null; } + // Dropping the coalesce also drops the evaluation of the right side, which + // only happens when the left side is null. That's only observationally + // equivalent when evaluating the right side cannot do anything but produce + // its null value. + $rightResult = $node->getRightResult(); + if ( + $rightResult->getImpurePoints() !== [] + || array_filter($rightResult->getThrowPoints(), static fn (InternalThrowPoint $throwPoint): bool => $throwPoint->isExplicit()) !== [] + || $rightResult->hasYield() + || $this->containsAssign($right) + ) { + return null; + } + // The coalesce only changes the result when the left side is undefined. // If the left side is always set, `?? null` (or `??= null`) never changes // anything, so the whole coalesce is redundant. @@ -109,4 +126,19 @@ private function checkUnnecessaryNullCoalesce(CoalesceExpressionNode $node, Scop )->identifier('nullCoalesce.unnecessary')->build(); } + /** + * Writes to variables, properties and offsets are not impure points, but they + * still make the right side worth keeping around. + */ + private function containsAssign(Node\Expr $expr): bool + { + return (new NodeFinder())->findFirst([$expr], static fn (Node $node): bool => $node instanceof Node\Expr\Assign + || $node instanceof Node\Expr\AssignRef + || $node instanceof Node\Expr\AssignOp + || $node instanceof Node\Expr\PostInc + || $node instanceof Node\Expr\PreInc + || $node instanceof Node\Expr\PostDec + || $node instanceof Node\Expr\PreDec) !== null; + } + } diff --git a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php index 42f5d5aaa1a..ba4d0e3c68a 100644 --- a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php +++ b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php @@ -505,6 +505,41 @@ public function testBug4337(): void ]); } + public function testBug15134(): void + { + $this->analyse([__DIR__ . '/data/bug-15134.php'], []); + } + + public function testUnnecessaryNullCoalesceSideEffects(): void + { + $this->analyse([__DIR__ . '/data/unnecessary-null-coalesce-side-effects.php'], [ + [ + 'Coalesce operator ??= is unnecessary because the left side is always set and the right side is null.', + 104, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 111, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 116, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 121, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 128, + ], + [ + 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', + 133, + ], + ]); + } + public function testBug12179(): void { $this->analyse([__DIR__ . '/data/bug-12179.php'], [ diff --git a/tests/PHPStan/Rules/Variables/data/bug-15134.php b/tests/PHPStan/Rules/Variables/data/bug-15134.php new file mode 100644 index 00000000000..0786f32b66f --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/bug-15134.php @@ -0,0 +1,30 @@ += 8.2 + +declare(strict_types = 1); + +namespace Bug15134; + +use LogicException; + +interface Node {} + +class A implements Node {} + +abstract class Parser { + + protected function parseExpressionChild(bool $value): ?Node { + return $this->parseNumber($value) + ?? $this->parseSpace($value); + } + + abstract protected function parseNumber(bool $value): ?A; + + protected function parseSpace(bool $value): null { + if ($value === false) { + throw new LogicException('The string is not a mathematical expression.'); + } + + return null; + } + +} diff --git a/tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php b/tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php new file mode 100644 index 00000000000..b3e37d8708c --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php @@ -0,0 +1,134 @@ += 8.2 + +declare(strict_types = 1); + +namespace UnnecessaryNullCoalesceSideEffects; + +use LogicException; + +const NULL_CONSTANT = null; + +function returnsNull(bool $value): null +{ + if ($value === false) { + throw new LogicException('nope'); + } + + return null; +} + +/** @phpstan-pure */ +function pureReturnsNull(): null +{ + return null; +} + +class Foo +{ + + public const NULL_CONSTANT = null; + + /** @var string|null */ + public $stringOrNull = null; + + /** @var null */ + public $alwaysNull = null; + + public function returnsNull(): null + { + return null; + } + + public static function staticReturnsNull(): null + { + return null; + } + + public function __invoke(): null + { + return null; + } + +} + +function funcCallOnRightSide(Foo $foo, ?string $name): ?string +{ + return $foo->stringOrNull ?? returnsNull($name !== null); +} + +function methodCallOnRightSide(Foo $foo): ?string +{ + return $foo->stringOrNull ?? $foo->returnsNull(); +} + +function staticCallOnRightSide(Foo $foo): ?string +{ + return $foo->stringOrNull ?? Foo::staticReturnsNull(); +} + +function invokeOnRightSide(Foo $foo): ?string +{ + return $foo->stringOrNull ?? $foo(); +} + +function closureCallOnRightSide(Foo $foo): ?string +{ + $closure = static function (): null { + echo 'side effect'; + + return null; + }; + + return $foo->stringOrNull ?? $closure(); +} + +function assignOnRightSide(Foo $foo): ?string +{ + $result = $foo->stringOrNull ?? $x = null; + echo $x; + + return $result; +} + +function assignOpOnRightSide(Foo $foo, ?string $name): ?string +{ + $x = $name; + $x ??= $foo->returnsNull(); + + return $x; +} + +function assignOpPureOnRightSide(?string $name): ?string +{ + $x = $name; + $x ??= null; + + return $x; +} + +function pureFuncCallOnRightSide(Foo $foo): ?string +{ + return $foo->stringOrNull ?? pureReturnsNull(); +} + +function constantOnRightSide(Foo $foo): ?string +{ + return $foo->stringOrNull ?? NULL_CONSTANT; +} + +function classConstantOnRightSide(Foo $foo): ?string +{ + return $foo->stringOrNull ?? Foo::NULL_CONSTANT; +} + +function nullVariableOnRightSide(Foo $foo): ?string +{ + $null = null; + + return $foo->stringOrNull ?? $null; +} + +function nullPropertyOnRightSide(Foo $foo, Foo $bar): ?string +{ + return $foo->stringOrNull ?? $bar->alwaysNull; +} From 4d16ac0aeb2b6b90ca3a460c2bfaa25e75518608 Mon Sep 17 00:00:00 2001 From: phpstan-bot Date: Fri, 11 Sep 2026 13:26:05 +0000 Subject: [PATCH 2/3] Make the null-coalesce side-effect test data PHP version independent The data file declared `null` return types natively, which is only a type on PHP 8.2+. With a lower `phpVersion` (the tests run on PHP 7.4 too) the same declaration resolves to an object type named `null`, so the right side isn't null and the pure-function case wasn't reported. Declare the null return types in PHPDoc instead so every case exercises what it claims on all analysed PHP versions. Co-Authored-By: Claude Opus 5 --- .../Rules/Variables/NullCoalesceRuleTest.php | 12 +++++----- ...unnecessary-null-coalesce-side-effects.php | 23 ++++++++++++------- 2 files changed, 21 insertions(+), 14 deletions(-) diff --git a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php index ba4d0e3c68a..304e46b4a58 100644 --- a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php +++ b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php @@ -515,27 +515,27 @@ public function testUnnecessaryNullCoalesceSideEffects(): void $this->analyse([__DIR__ . '/data/unnecessary-null-coalesce-side-effects.php'], [ [ 'Coalesce operator ??= is unnecessary because the left side is always set and the right side is null.', - 104, + 111, ], [ 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', - 111, + 118, ], [ 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', - 116, + 123, ], [ 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', - 121, + 128, ], [ 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', - 128, + 135, ], [ 'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.', - 133, + 140, ], ]); } diff --git a/tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php b/tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php index b3e37d8708c..11c47febefa 100644 --- a/tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php +++ b/tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php @@ -1,4 +1,4 @@ -= 8.2 + Date: Sat, 12 Sep 2026 09:07:11 +0000 Subject: [PATCH 3/3] Cover every node kind checked by NullCoalesceRule::containsAssign() Only the plain `Assign` branch was exercised so far. Add one right side per remaining node kind, each of them inferred as `null` so it reaches the check: `=&`, a nested `+=`, and `++`/`--` in an array offset of an `array`. Removing any single `instanceof` clause makes exactly one of these cases report the false positive again. Co-Authored-By: Claude Opus 5 --- ...unnecessary-null-coalesce-side-effects.php | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php b/tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php index 11c47febefa..5de1a355d23 100644 --- a/tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php +++ b/tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php @@ -139,3 +139,40 @@ function nullPropertyOnRightSide(Foo $foo, Foo $bar): ?string { return $foo->stringOrNull ?? $bar->alwaysNull; } + +function assignRefOnRightSide(Foo $foo): ?string +{ + $null = null; + + return $foo->stringOrNull ?? $ref = &$null; +} + +/** @param array $nulls */ +function assignOpInOffsetOnRightSide(Foo $foo, array $nulls, int $i): ?string +{ + return $foo->stringOrNull ?? $nulls[$i += 1]; +} + +/** @param array $nulls */ +function postIncOnRightSide(Foo $foo, array $nulls, int $i): ?string +{ + return $foo->stringOrNull ?? $nulls[$i++]; +} + +/** @param array $nulls */ +function preIncOnRightSide(Foo $foo, array $nulls, int $i): ?string +{ + return $foo->stringOrNull ?? $nulls[++$i]; +} + +/** @param array $nulls */ +function postDecOnRightSide(Foo $foo, array $nulls, int $i): ?string +{ + return $foo->stringOrNull ?? $nulls[$i--]; +} + +/** @param array $nulls */ +function preDecOnRightSide(Foo $foo, array $nulls, int $i): ?string +{ + return $foo->stringOrNull ?? $nulls[--$i]; +}