Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Analyser/ExprHandler/AssignOpHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion src/Analyser/ExprHandler/CoalesceHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions src/Node/CoalesceExpressionNode.php
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
{
Expand All @@ -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;
Expand Down
32 changes: 32 additions & 0 deletions src/Rules/Variables/NullCoalesceRule.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -15,6 +17,7 @@
use PHPStan\Rules\Rule;
use PHPStan\Rules\RuleErrorBuilder;
use PHPStan\Type\Type;
use function array_filter;
use function sprintf;

/**
Expand Down Expand Up @@ -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.
Expand All @@ -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;
Comment on lines +138 to +141

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these look untested

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed and pushed as e56caaa5c.

staabm was right — only the Assign branch of containsAssign() was exercised (by $foo->stringOrNull ?? $x = null on line 94). The other six node kinds had no coverage.

What I added to tests/PHPStan/Rules/Variables/data/unnecessary-null-coalesce-side-effects.php — one right side per remaining node kind, each expecting no error:

Node kind Right side Line
AssignRef $ref = &$null 147
AssignOp $nulls[$i += 1] 153
PostInc $nulls[$i++] 159
PreInc $nulls[++$i] 165
PostDec $nulls[$i--] 171
PreDec $nulls[--$i] 177

The four inc/dec cases put the operator inside an array offset on an array<int, null> rather than writing $null++ directly: ++/-- applied to a null variable is deprecated since PHP 8.3 and would be testing a construct nobody writes, while the offset form keeps the right side's type null (verified with \PHPStan\dumpType() — all six infer as null, so each actually reaches the containsAssign() check rather than bailing earlier on the type test). AssignOp needed the same treatment because the only AssignOp that evaluates to null on its own is ??=, which would have nested a second coalesce into the case.

Verification that each branch is load-bearing, not just that the tests pass:

  • With || $this->containsAssign($right) removed entirely, all 7 assignment cases report the false positive (94, 147, 153, 159, 165, 171, 177); the 6 legitimate reports are unaffected.
  • Removing each instanceof clause one at a time makes exactly one case regress — Assign→94, AssignRef→147, AssignOp→153, PostInc→159, PreInc→165, PostDec→171, PreDec→177. No clause is redundant, and no case covers two clauses.

NullCoalesceRuleTest 44/44, make tests 21353 OK (97 skipped, unchanged), make phpstan no errors, make lint clean. The rule source is untouched by this commit; the expected-error list in the test is unchanged since every new case expects no error.

}

}
35 changes: 35 additions & 0 deletions tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.',
111,
],
[
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
118,
],
[
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
123,
],
[
'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.',
135,
],
[
'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
140,
],
]);
}

public function testBug12179(): void
{
$this->analyse([__DIR__ . '/data/bug-12179.php'], [
Expand Down
30 changes: 30 additions & 0 deletions tests/PHPStan/Rules/Variables/data/bug-15134.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?php // lint >= 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;
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
<?php

declare(strict_types = 1);

namespace UnnecessaryNullCoalesceSideEffects;

use LogicException;

const NULL_CONSTANT = null;

/** @return null */
function returnsNull(bool $value)
{
if ($value === false) {
throw new LogicException('nope');
}

return null;
}

/**
* @phpstan-pure
* @return null
*/
function pureReturnsNull()
{
return null;
}

class Foo
{

public const NULL_CONSTANT = null;

/** @var string|null */
public $stringOrNull = null;

/** @var null */
public $alwaysNull = null;

/** @return null */
public function returnsNull()
{
return null;
}

/** @return null */
public static function staticReturnsNull()
{
return null;
}

/** @return null */
public function __invoke()
{
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 () {
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;
}

function assignRefOnRightSide(Foo $foo): ?string
{
$null = null;

return $foo->stringOrNull ?? $ref = &$null;
}

/** @param array<int, null> $nulls */
function assignOpInOffsetOnRightSide(Foo $foo, array $nulls, int $i): ?string
{
return $foo->stringOrNull ?? $nulls[$i += 1];
}

/** @param array<int, null> $nulls */
function postIncOnRightSide(Foo $foo, array $nulls, int $i): ?string
{
return $foo->stringOrNull ?? $nulls[$i++];
}

/** @param array<int, null> $nulls */
function preIncOnRightSide(Foo $foo, array $nulls, int $i): ?string
{
return $foo->stringOrNull ?? $nulls[++$i];
}

/** @param array<int, null> $nulls */
function postDecOnRightSide(Foo $foo, array $nulls, int $i): ?string
{
return $foo->stringOrNull ?? $nulls[$i--];
}

/** @param array<int, null> $nulls */
function preDecOnRightSide(Foo $foo, array $nulls, int $i): ?string
{
return $foo->stringOrNull ?? $nulls[--$i];
}
Loading