Skip to content

Commit 2284215

Browse files
committed
Fix false positive nullsafe.neverNull on left side of ??
The nullsafe operator ?-> on the left side of ?? was unconditionally reported as unnecessary. This is correct for isset()/empty() which handle null objects natively, but wrong for ?? which does not catch TypeError from property access on null objects.
1 parent 8d69bd0 commit 2284215

File tree

3 files changed

+82
-12
lines changed

3 files changed

+82
-12
lines changed

src/Rules/IssetCheck.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,16 @@ static function (Type $type) use ($typeMessageCallback): ?string {
252252
}
253253

254254
if ($expr instanceof Expr\NullsafePropertyFetch) {
255+
// For ?? the nullsafe operator is needed when the object can be null,
256+
// because ?? does not catch TypeError from property access on null.
257+
// isset()/empty() handle null objects natively so ?-> is truly redundant there.
258+
if ($identifier === 'nullCoalesce') {
259+
$varType = $this->treatPhpDocTypesAsCertain ? $scope->getScopeType($expr->var) : $scope->getScopeNativeType($expr->var);
260+
if (!$varType->isNull()->no()) {
261+
return null;
262+
}
263+
}
264+
255265
if ($expr->name instanceof Node\Identifier) {
256266
return RuleErrorBuilder::message(sprintf('Using nullsafe property access "?->%s" %s is unnecessary. Use -> instead.', $expr->name->name, $operatorDescription))
257267
->identifier('nullsafe.neverNull')

tests/PHPStan/Rules/Variables/NullCoalesceRuleTest.php

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -253,22 +253,10 @@ public function testBug5933(): void
253253
public function testBug7109(): void
254254
{
255255
$this->analyse([__DIR__ . '/../Properties/data/bug-7109.php'], [
256-
[
257-
'Using nullsafe property access "?->aaa" on left side of ?? is unnecessary. Use -> instead.',
258-
17,
259-
],
260-
[
261-
'Using nullsafe property access "?->aaa" on left side of ?? is unnecessary. Use -> instead.',
262-
28,
263-
],
264256
[
265257
'Expression on left side of ?? is not nullable.',
266258
40,
267259
],
268-
[
269-
'Using nullsafe property access "?->aaa" on left side of ?? is unnecessary. Use -> instead.',
270-
66,
271-
],
272260
[
273261
'Expression on left side of ?? is not nullable.',
274262
73,
@@ -372,4 +360,15 @@ public function testBug13921(): void
372360
]);
373361
}
374362

363+
#[RequiresPhp('>= 8.0')]
364+
public function testNullsafeCoalesceNullableObject(): void
365+
{
366+
$this->analyse([__DIR__ . '/data/bug-nullsafe-coalesce-nullable-object.php'], [
367+
[
368+
'Expression on left side of ?? is not nullable.',
369+
59,
370+
],
371+
]);
372+
}
373+
375374
}
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
<?php // lint >= 8.0
2+
3+
namespace BugNullsafeCoalesceNullableObject;
4+
5+
class Detail
6+
{
7+
public string $label;
8+
9+
public function __construct(string $label)
10+
{
11+
$this->label = $label;
12+
}
13+
}
14+
15+
class Node
16+
{
17+
private ?Detail $detail;
18+
19+
public function __construct(?Detail $detail)
20+
{
21+
$this->detail = $detail;
22+
}
23+
24+
public function getDetail(): ?Detail
25+
{
26+
return $this->detail;
27+
}
28+
}
29+
30+
class Root
31+
{
32+
private ?Node $node;
33+
34+
public function __construct(?Node $node)
35+
{
36+
$this->node = $node;
37+
}
38+
39+
public function getNode(): ?Node
40+
{
41+
return $this->node;
42+
}
43+
}
44+
45+
class Foo
46+
{
47+
public function chainedNullable(Root $root): void
48+
{
49+
$a = $root->getNode()?->getDetail()?->label ?? '';
50+
}
51+
52+
public function singleNullable(Node $node): void
53+
{
54+
$a = $node->getDetail()?->label ?? '';
55+
}
56+
57+
public function allNonNullable(Detail $detail): void
58+
{
59+
$a = $detail?->label ?? '';
60+
}
61+
}

0 commit comments

Comments
 (0)