diff --git a/src/Rules/IssetCheck.php b/src/Rules/IssetCheck.php index 35195e7ea5..1f9e56ce3d 100644 --- a/src/Rules/IssetCheck.php +++ b/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; diff --git a/tests/PHPStan/Rules/Properties/data/bug-7109.php b/tests/PHPStan/Rules/Properties/data/bug-7109.php index 6c539b60f8..2495b5b737 100644 --- a/tests/PHPStan/Rules/Properties/data/bug-7109.php +++ b/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'; diff --git a/tests/PHPStan/Rules/Variables/EmptyRuleTest.php b/tests/PHPStan/Rules/Variables/EmptyRuleTest.php index 582fdeb107..203217515e 100644 --- a/tests/PHPStan/Rules/Variables/EmptyRuleTest.php +++ b/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, diff --git a/tests/PHPStan/Rules/Variables/IssetRuleTest.php b/tests/PHPStan/Rules/Variables/IssetRuleTest.php index 7e83c53117..8913d093bb 100644 --- a/tests/PHPStan/Rules/Variables/IssetRuleTest.php +++ b/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, diff --git a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php b/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php index fc6c577822..49eb1130dc 100644 --- a/tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php +++ b/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, ], ]); } diff --git a/tests/PHPStan/Rules/Variables/data/bug-14311.php b/tests/PHPStan/Rules/Variables/data/bug-14311.php new file mode 100644 index 0000000000..f2305782a5 --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/bug-14311.php @@ -0,0 +1,48 @@ += 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; +}