Fix nullsafe.neverNull false positive when the ?-> operand is nullable

$a?->b on the left of ??, or inside isset()/empty(), was reported as
redundant whenever $a?->b as a whole was nullable, without checking $a
itself. When $a is nullable the ?-> is what yields the null those
constructs handle, so it's required, and switching to -> as advised would
fatal at runtime.

Report only when the operand is never null, and recurse into a nullsafe
operand so an inner never-null link ($nonNull?->nullable?->x) is still
caught, matching NullsafePropertyFetchRule outside these contexts. Adds the
same treatPhpDocTypesAsCertain tip, and fixes the bug-7109 expectations
that had baked in the false positive.

Fixes phpstan/phpstan#14311

Author: clarg18

src/Rules/IssetCheck.php

@@ -31,6 +31,8 @@ public function __construct(
 		private bool $checkAdvancedIsset,
 		#[AutowiredParameter]
 		private bool $treatPhpDocTypesAsCertain,
+		#[AutowiredParameter(ref: '%tips.treatPhpDocTypesAsCertain%')]
+		private bool $treatPhpDocTypesAsCertainTip,
 	)
 	{
 	}
@@ -259,15 +261,35 @@ static function (Type $type) use ($typeMessageCallback): ?string {
 		}
 
 		if ($expr instanceof Expr\NullsafePropertyFetch) {
-			if ($expr->name instanceof Node\Identifier) {
-				return RuleErrorBuilder::message(sprintf('Using nullsafe property access "?->%s" %s is unnecessary. Use -> instead.', $expr->name->name, $operatorDescription))
-					->identifier('nullsafe.neverNull')
-					->build();
+			// Only redundant when the operand itself is never null; otherwise the
+			// ?-> produces the null that ??/isset()/empty() handles. A nullable
+			// operand can still wrap a never-null one ($a?->b?->c), so recurse.
+			$operandType = $this->treatPhpDocTypesAsCertain
+				? $scope->getScopeType($expr->var)
+				: $scope->getScopeNativeType($expr->var);
+			if (!$operandType->isNull()->no()) {
+				if ($expr->var instanceof Expr\NullsafePropertyFetch) {
+					return $this->check($expr->var, $scope, $operatorDescription, $identifier, $typeMessageCallback);
+				}
+
+				return null;
 			}
 
-			return RuleErrorBuilder::message(sprintf('Using nullsafe property access "?->(Expression)" %s is unnecessary. Use -> instead.', $operatorDescription))
-				->identifier('nullsafe.neverNull')
-				->build();
+			$message = $expr->name instanceof Node\Identifier
+				? sprintf('Using nullsafe property access "?->%s" %s is unnecessary. Use -> instead.', $expr->name->name, $operatorDescription)
+				: sprintf('Using nullsafe property access "?->(Expression)" %s is unnecessary. Use -> instead.', $operatorDescription);
+
+			$ruleErrorBuilder = RuleErrorBuilder::message($message)->identifier('nullsafe.neverNull');
+
+			if (
+				$this->treatPhpDocTypesAsCertain
+				&& $this->treatPhpDocTypesAsCertainTip
+				&& !$scope->getScopeNativeType($expr->var)->isNull()->no()
+			) {
+				$ruleErrorBuilder->treatPhpDocTypesAsCertainTip();
+			}
+
+			return $ruleErrorBuilder->build();
 		}
 
 		return null;

tests/PHPStan/Rules/Properties/data/bug-7109.php

@@ -62,7 +62,7 @@ public function emptyNotFalsy2(): void
 	public ?HelloWorld $prop = null;
 	public function edgeCaseWithMethodCall(): void
 	{
-		// only ?->aaa should be reported
+		// nothing to report: every ?-> operand can be null (phpstan/phpstan#14311)
 		$this->get()?->prop?->get()?->aaa ?? 'edge';
 		isset($this->get()?->prop?->get()?->aaa) ?: 'edge';
 		empty($this->get()?->prop?->get()?->aaa) ?: 'edge';

tests/PHPStan/Rules/Variables/EmptyRuleTest.php

@@ -25,6 +25,7 @@ protected function getRule(): Rule
 			new PropertyReflectionFinder(),
 			true,
 			$this->treatPhpDocTypesAsCertain,
+			true,
 		));
 	}
 
@@ -115,30 +116,14 @@ public function testBug7109(): void
 		$this->treatPhpDocTypesAsCertain = true;
 
 		$this->analyse([__DIR__ . '/../Properties/data/bug-7109.php'], [
-			[
-				'Using nullsafe property access "?->aaa" in empty() is unnecessary. Use -> instead.',
-				19,
-			],
-			[
-				'Using nullsafe property access "?->aaa" in empty() is unnecessary. Use -> instead.',
-				30,
-			],
 			[
 				'Using nullsafe property access "?->aaa" in empty() is unnecessary. Use -> instead.',
 				42,
 			],
-			[
-				'Using nullsafe property access "?->notFalsy" in empty() is unnecessary. Use -> instead.',
-				54,
-			],
 			[
 				'Expression in empty() is not falsy.',
 				59,
 			],
-			[
-				'Using nullsafe property access "?->aaa" in empty() is unnecessary. Use -> instead.',
-				68,
-			],
 			[
 				'Using nullsafe property access "?->(Expression)" in empty() is unnecessary. Use -> instead.',
 				75,

tests/PHPStan/Rules/Variables/IssetRuleTest.php

@@ -24,6 +24,7 @@ protected function getRule(): Rule
 			new PropertyReflectionFinder(),
 			true,
 			$this->treatPhpDocTypesAsCertain,
+			true,
 		));
 	}
 
@@ -335,22 +336,10 @@ public function testBug7109(): void
 		$this->treatPhpDocTypesAsCertain = true;
 
 		$this->analyse([__DIR__ . '/../Properties/data/bug-7109.php'], [
-			[
-				'Using nullsafe property access "?->aaa" in isset() is unnecessary. Use -> instead.',
-				18,
-			],
-			[
-				'Using nullsafe property access "?->aaa" in isset() is unnecessary. Use -> instead.',
-				29,
-			],
 			[
 				'Expression in isset() is not nullable.',
 				41,
 			],
-			[
-				'Using nullsafe property access "?->aaa" in isset() is unnecessary. Use -> instead.',
-				67,
-			],
 			[
 				'Expression in isset() is not nullable.',
 				74,

tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php

@@ -23,6 +23,7 @@ protected function getRule(): Rule
 			new PropertyReflectionFinder(),
 			true,
 			$this->shouldTreatPhpDocTypesAsCertain(),
+			true,
 		), true);
 	}
 
@@ -263,25 +264,28 @@ public function testBug13623(): void
 	public function testBug7109(): void
 	{
 		$this->analyse([__DIR__ . '/../Properties/data/bug-7109.php'], [
-			[
-				'Using nullsafe property access "?->aaa" on left side of ?? is unnecessary. Use -> instead.',
-				17,
-			],
-			[
-				'Using nullsafe property access "?->aaa" on left side of ?? is unnecessary. Use -> instead.',
-				28,
-			],
 			[
 				'Expression on left side of ?? is not nullable.',
 				40,
 			],
 			[
-				'Using nullsafe property access "?->aaa" on left side of ?? is unnecessary. Use -> instead.',
-				66,
+				'Expression on left side of ?? is not nullable.',
+				73,
 			],
+		]);
+	}
+
+	#[RequiresPhp('>= 8.0.0')]
+	public function testBug14311(): void
+	{
+		$this->analyse([__DIR__ . '/data/bug-14311.php'], [
 			[
 				'Expression on left side of ?? is not nullable.',
-				73,
+				33,
+			],
+			[
+				'Using nullsafe property access "?->inner" on left side of ?? is unnecessary. Use -> instead.',
+				47,
 			],
 		]);
 	}

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

@@ -0,0 +1,48 @@
+<?php declare(strict_types = 1); // lint >= 8.0
+
+namespace Bug14311;
+
+final class Node
+{
+
+	public int $id = 5;
+
+}
+
+function resolve(bool $flag): ?Node
+{
+	return $flag ? new Node() : null;
+}
+
+function make(): Node
+{
+	return new Node();
+}
+
+function coalesce(bool $flag): int
+{
+	// $node is nullable, so the ?-> is required, not redundant.
+	$node = resolve($flag);
+
+	return $node?->id ?? 0;
+}
+
+function neverNullOperand(): int
+{
+	// make() never returns null, so the ?-> is redundant.
+	return make()?->id ?? 0;
+}
+
+final class Outer
+{
+
+	public ?Node $inner = null;
+
+}
+
+function chain(Outer $outer): int
+{
+	// $outer is never null so ?->inner is redundant; $outer->inner is
+	// nullable so ?->id is required.
+	return $outer?->inner?->id ?? 0;
+}