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

@@ -1864,7 +1864,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();
 
@@ -3634,7 +3634,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());
@@ -3693,7 +3693,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
@@ -364,12 +364,20 @@ public function testPr4372(): void
 
 	public function testBug14213(): void
 	{
-		$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-14213.php'], [
-			[
-				'Variable $x1 on left side of ?? always exists and is always null.',
-				22,
-			],
-		]);
+		$errors = [];
+		if (PHP_VERSION_ID >= 80100) {
+			// This is only detected with FiberScope.
+			$errors[] = [
+				'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
+				21,
+			];
+		}
+		$errors[] = [
+			'Variable $x1 on left side of ?? always exists and is always null.',
+			22,
+		];
+
+		$this->analyse([__DIR__ . '/../../Analyser/nsrt/bug-14213.php'], $errors);
 	}
 
 	public function testBug11488(): void
@@ -430,6 +438,72 @@ 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 testBug12179(): void
+	{
+		$this->analyse([__DIR__ . '/data/bug-12179.php'], [
+			[
+				'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
+				8,
+			],
+			[
+				'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
+				24,
+			],
+		]);
+	}
+
+	public function testBug9966(): void
+	{
+		$this->analyse([__DIR__ . '/data/bug-9966.php'], [
+			[
+				'Offset \'key1\' on array{key1: string, key2: string|null, key3?: string, key4?: string|null} on left side of ?? always exists and is not nullable.',
+				9,
+			],
+			[
+				'Coalesce operator ?? is unnecessary because the left side is always set and the right side is null.',
+				12,
+			],
+		]);
+	}
+
 	public function testBug14393(): void
 	{
 		$this->analyse([__DIR__ . '/data/bug-14393.php'], [

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

@@ -0,0 +1,25 @@
+<?php
+
+namespace Bug12179;
+
+function alwaysDefinedNullableParam(?string $name): ?string
+{
+	// $name is always defined but nullable - the `?? null` is unnecessary
+	return $name ?? null;
+}
+
+function maybeUndefinedVariable(): ?string
+{
+	if (rand() > 0.5) {
+		$x = 'foo';
+	}
+
+	// $x might be undefined - not reported
+	return $x ?? null;
+}
+
+/** @param array{foo?: ?int, bar: ?int} $x */
+function foo(array $x): void {
+	var_dump($x['foo'] ?? null); // Fine
+	var_dump($x['bar'] ?? null); // ?? null is redundant here
+}