Merge 2.3.x into 2.2.x

src/Analyser/ExprHandler/AssignOpHandler.php

@@ -282,7 +282,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
 				throw new ShouldNotHappenException();
 			}
 
-			$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);
 		}
 
 		$writeFlow = VariableFlow::sequence(

src/Analyser/ExprHandler/CoalesceHandler.php

@@ -99,7 +99,7 @@ public function processExpr(NodeScopeResolver $nodeScopeResolver, Stmt $stmt, Ex
 			$scope = $scope->applySpecifiedTypes($leftIssetTypes)->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,

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;

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;
+	}
+
 }

tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php

@@ -510,6 +510,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'], [

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

@@ -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;
+	}
+
+}

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

@@ -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];
+}