Report ?? null / ??= null on an always-set left side as an unnecessary null coalesce

conf/bleedingEdge.neon

@@ -20,3 +20,4 @@ parameters:
 		reportMethodPurityOverride: true
 		checkDynamicConstantNameValues: true
 		unusedLabel: true
+		unnecessaryNullCoalesce: true

conf/config.neon

@@ -47,6 +47,7 @@ parameters:
 		reportMethodPurityOverride: false
 		checkDynamicConstantNameValues: false
 		unusedLabel: false
+		unnecessaryNullCoalesce: false
 	fileExtensions:
 		- php
 	checkAdvancedIsset: false

conf/parametersSchema.neon

@@ -49,6 +49,7 @@ parametersSchema:
 		reportMethodPurityOverride: bool()
 		checkDynamicConstantNameValues: bool()
 		unusedLabel: bool()
+		unnecessaryNullCoalesce: bool()
 	])
 	fileExtensions: listOf(string())
 	checkAdvancedIsset: bool()

src/Analyser/NodeScopeResolver.php

@@ -1865,7 +1865,7 @@ public function processStmtNode(
 
 			$bodyScope = $initScope;
 			$isIterableAtLeastOnce = TrinaryLogic::createYes();
-			$lastCondExpr = array_last($stmt->cond) ?? null;
+			$lastCondExpr = array_last($stmt->cond);
 			if (count($stmt->cond) > 0) {
 				$storage = $originalStorage->duplicate();
 
@@ -3635,7 +3635,7 @@ public function processArgs(
 				}
 
 				$this->callNodeCallbackWithExpression($nodeCallback, $arg->value, $scopeToPass, $storage, $context);
-				$closureResult = $this->processClosureNode($stmt, $arg->value, $scopeToPass, $storage, $nodeCallback, $context, $parameterType ?? null, $parameterNativeType);
+				$closureResult = $this->processClosureNode($stmt, $arg->value, $scopeToPass, $storage, $nodeCallback, $context, $parameterType, $parameterNativeType);
 				if ($this->callCallbackImmediately($parameter, $parameterType, $calleeReflection)) {
 					$throwPoints = array_merge($throwPoints, array_map(static fn (InternalThrowPoint $throwPoint) => $throwPoint->isExplicit() ? InternalThrowPoint::createExplicit($scope, $throwPoint->getType(), $arg->value, $throwPoint->canContainAnyThrowable()) : InternalThrowPoint::createImplicit($scope, $arg->value), $closureResult->getThrowPoints()));
 					$impurePoints = array_merge($impurePoints, $closureResult->getImpurePoints());
@@ -3694,7 +3694,7 @@ public function processArgs(
 				}
 
 				$this->callNodeCallbackWithExpression($nodeCallback, $arg->value, $scopeToPass, $storage, $context);
-				$arrowFunctionResult = $this->processArrowFunctionNode($stmt, $arg->value, $scopeToPass, $storage, $nodeCallback, $parameterType ?? null, $parameterNativeType);
+				$arrowFunctionResult = $this->processArrowFunctionNode($stmt, $arg->value, $scopeToPass, $storage, $nodeCallback, $parameterType, $parameterNativeType);
 				if ($this->callCallbackImmediately($parameter, $parameterType, $calleeReflection)) {
 					$throwPoints = array_merge($throwPoints, array_map(static fn (InternalThrowPoint $throwPoint) => $throwPoint->isExplicit() ? InternalThrowPoint::createExplicit($scope, $throwPoint->getType(), $arg->value, $throwPoint->canContainAnyThrowable()) : InternalThrowPoint::createImplicit($scope, $arg->value), $arrowFunctionResult->getThrowPoints()));
 					$impurePoints = array_merge($impurePoints, $arrowFunctionResult->getImpurePoints());

src/Reflection/SignatureMap/Php8SignatureMapProvider.php

@@ -477,7 +477,7 @@ private function getSignature(
 
 		return new FunctionSignature(
 			$parameters,
-			TypehintHelper::decideType($returnType, $phpDocReturnType ?? null),
+			TypehintHelper::decideType($returnType, $phpDocReturnType),
 			$returnType,
 			$variadic,
 		);

src/Rules/Variables/NullCoalesceRule.php

@@ -4,10 +4,14 @@
 
 use PhpParser\Node;
 use PHPStan\Analyser\Scope;
+use PHPStan\DependencyInjection\AutowiredParameter;
 use PHPStan\DependencyInjection\RegisteredRule;
+use PHPStan\Rules\IdentifierRuleError;
 use PHPStan\Rules\IssetCheck;
 use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleErrorBuilder;
 use PHPStan\Type\Type;
+use function sprintf;
 
 /**
  * @implements Rule<Node\Expr>
@@ -16,7 +20,11 @@
 final class NullCoalesceRule implements Rule
 {
 
-	public function __construct(private IssetCheck $issetCheck)
+	public function __construct(
+		private IssetCheck $issetCheck,
+		#[AutowiredParameter(ref: '%featureToggles.unnecessaryNullCoalesce%')]
+		private bool $unnecessaryNullCoalesce,
+	)
 	{
 	}
 
@@ -41,18 +49,50 @@ public function processNode(Node $node, Scope $scope): array
 		};
 
 		if ($node instanceof Node\Expr\BinaryOp\Coalesce) {
-			$error = $this->issetCheck->check($node->left, $scope, 'on left side of ??', 'nullCoalesce', $typeMessageCallback);
+			$left = $node->left;
+			$right = $node->right;
+			$operator = '??';
 		} elseif ($node instanceof Node\Expr\AssignOp\Coalesce) {
-			$error = $this->issetCheck->check($node->var, $scope, 'on left side of ??=', 'nullCoalesce', $typeMessageCallback);
+			$left = $node->var;
+			$right = $node->expr;
+			$operator = '??=';
 		} else {
 			return [];
 		}
 
-		if ($error === null) {
-			return [];
+		$error = $this->issetCheck->check($left, $scope, sprintf('on left side of %s', $operator), 'nullCoalesce', $typeMessageCallback);
+		if ($error !== null) {
+			return [$error];
+		}
+
+		$unnecessaryError = $this->checkUnnecessaryNullCoalesce($left, $right, $operator, $scope);
+		if ($unnecessaryError !== null) {
+			return [$unnecessaryError];
+		}
+
+		return [];
+	}
+
+	private function checkUnnecessaryNullCoalesce(Node\Expr $left, Node\Expr $right, string $operator, Scope $scope): ?IdentifierRuleError
+	{
+		if (!$this->unnecessaryNullCoalesce) {
+			return null;
+		}
+
+		if (!$scope->getType($right)->isNull()->yes()) {
+			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.
+		if ($scope->toMutatingScope()->issetCheck($left, static fn (): bool => true) !== true) {
+			return null;
 		}
 
-		return [$error];
+		return RuleErrorBuilder::message(
+			sprintf('Coalesce operator %s is unnecessary because the left side is always set and the right side is null.', $operator),
+		)->identifier('nullCoalesce.unnecessary')->build();
 	}
 
 }

src/Type/FileTypeMapper.php

@@ -484,8 +484,8 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA
 					}
 				}
 
-				$className = array_last($classStack) ?? null;
-				$functionName = array_last($functionStack) ?? null;
+				$className = array_last($classStack);
+				$functionName = array_last($functionStack);
 				$nameScopeKey = $this->getNameScopeKey($originalClassFileName, $className, $lookForTrait, $functionName);
 
 				$phpDocNode = null;
@@ -506,7 +506,7 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA
 							$typeAliasStack[] = $this->getTypeAliasesMap($phpDocNode);
 						}
 
-						$parentNameScope = array_last($typeMapStack) ?? null;
+						$parentNameScope = array_last($typeMapStack);
 
 						$typeMapStack[] = new IntermediaryNameScope(
 							$namespace,
@@ -522,7 +522,7 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA
 					} elseif ($node instanceof Node\Stmt\ClassLike) {
 						$typeAliasStack[] = [];
 					} else {
-						$parentNameScope = array_last($typeMapStack) ?? null;
+						$parentNameScope = array_last($typeMapStack);
 						$typeMapStack[] = new IntermediaryNameScope(
 							$namespace,
 							$uses,
@@ -553,7 +553,7 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA
 						)
 					) && !array_key_exists($nameScopeKey, $nameScopeMap)
 				) {
-					$parentNameScope = array_last($typeMapStack) ?? null;
+					$parentNameScope = array_last($typeMapStack);
 					$typeAliasesMap = array_last($typeAliasStack) ?? [];
 					$nameScopeMap[$nameScopeKey] = new IntermediaryNameScope(
 						$namespace,
@@ -640,7 +640,7 @@ function (Node $node) use ($fileName, $lookForTrait, &$traitFound, $traitMethodA
 							continue;
 						}
 
-						$className = array_last($classStack) ?? null;
+						$className = array_last($classStack);
 						if ($className === null) {
 							throw new ShouldNotHappenException();
 						}

tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php

@@ -23,7 +23,7 @@ protected function getRule(): Rule
 			new PropertyReflectionFinder(),
 			true,
 			$this->shouldTreatPhpDocTypesAsCertain(),
-		));
+		), true);
 	}
 
 	public function testCoalesceRule(): void
@@ -365,6 +365,10 @@ public function testPr4372(): void
 	public function testBug14213(): void
 	{
 		$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-14213.php'], [
+			[
+				'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
+				21,
+			],
 			[
 				'Variable $x1 on left side of ?? always exists and is always null.',
 				22,
@@ -430,6 +434,44 @@ public function testBug14459Hooked(): void
 		]);
 	}
 
+	public function testBug4337(): void
+	{
+		$this->analyse([__DIR__ . '/data/bug-4337.php'], [
+			[
+				'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
+				37,
+			],
+			[
+				'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
+				42,
+			],
+			[
+				'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
+				47,
+			],
+			[
+				'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
+				53,
+			],
+			[
+				'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
+				58,
+			],
+			[
+				'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
+				63,
+			],
+			[
+				'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
+				69,
+			],
+			[
+				'Coalesce operator ??= is unnecessary because the left side is always set and the right side is null.',
+				75,
+			],
+		]);
+	}
+
 	public function testBug14393(): void
 	{
 		$this->analyse([__DIR__ . '/data/bug-14393.php'], [

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

@@ -0,0 +1,98 @@
+<?php
+
+namespace Bug4337;
+
+class Foo
+{
+
+	/** @var string|null */
+	public $stringOrNull = null;
+
+	/** @var string|null */
+	public static $staticStringOrNull = null;
+
+	public function method(): ?string
+	{
+		return null;
+	}
+
+	public static function staticMethod(): ?string
+	{
+		return null;
+	}
+
+}
+
+function hello1(?string $name): ?string
+{
+	if ($name === null) {
+		return null;
+	}
+
+	return 'Hello, ' . $name . '!';
+}
+
+function hello2(?string $name): ?string
+{
+	return hello1($name) ?? null;
+}
+
+function methodCall(Foo $foo): ?string
+{
+	return $foo->method() ?? null;
+}
+
+function staticCall(): ?string
+{
+	return Foo::staticMethod() ?? null;
+}
+
+function definedNullableVariable(?string $name): ?string
+{
+	$x = $name;
+	return $x ?? null;
+}
+
+function alwaysSetNullableProperty(Foo $foo): ?string
+{
+	return $foo->stringOrNull ?? null;
+}
+
+function alwaysSetNullableStaticProperty(): ?string
+{
+	return Foo::$staticStringOrNull ?? null;
+}
+
+/** @param array{a: string|null} $arr */
+function alwaysSetNullableOffset(array $arr): ?string
+{
+	return $arr['a'] ?? null;
+}
+
+function assignCoalesceAlwaysSet(?string $name): void
+{
+	$x = $name;
+	$x ??= null;
+}
+
+// these are NOT unnecessary and must not be reported
+
+function maybeUndefinedVariable(): ?string
+{
+	if (rand() > 0.5) {
+		$x = 'foo';
+	}
+
+	return $x ?? null;
+}
+
+/** @param array<string, string|null> $arr */
+function maybeUndefinedOffset(array $arr): ?string
+{
+	return $arr['a'] ?? null;
+}
+
+function nonNullRightSide(?string $name): string
+{
+	return hello1($name) ?? 'default';
+}