Skip to content

Commit 19dd4b0

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 ea0260a commit 19dd4b0

File tree

3 files changed

+88
-12
lines changed

3 files changed

+88
-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: 10 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,14 @@ public function testBug13921(): void
372360
]);
373361
}
374362

363+
public function testNullsafeCoalesceNullableObject(): void
364+
{
365+
$this->analyse([__DIR__ . '/data/bug-nullsafe-coalesce-nullable-object.php'], [
366+
[
367+
'Expression on left side of ?? is not nullable.',
368+
66,
369+
],
370+
]);
371+
}
372+
375373
}
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
<?php // lint >= 8.0
2+
3+
namespace BugNullsafeCoalesceNullableObject;
4+
5+
class TransactionVoucher
6+
{
7+
public string $code;
8+
9+
public function __construct(string $code)
10+
{
11+
$this->code = $code;
12+
}
13+
}
14+
15+
class Transaction
16+
{
17+
private ?TransactionVoucher $voucher;
18+
19+
public function __construct(?TransactionVoucher $voucher)
20+
{
21+
$this->voucher = $voucher;
22+
}
23+
24+
public function getTransactionVoucher(): ?TransactionVoucher
25+
{
26+
return $this->voucher;
27+
}
28+
}
29+
30+
class Disbursement
31+
{
32+
private ?Transaction $transaction;
33+
34+
public function __construct(?Transaction $transaction)
35+
{
36+
$this->transaction = $transaction;
37+
}
38+
39+
public function getTransaction(): ?Transaction
40+
{
41+
return $this->transaction;
42+
}
43+
}
44+
45+
class Foo
46+
{
47+
public function doFoo(Disbursement $disbursement): void
48+
{
49+
// Both ?-> are needed: getTransaction() and getTransactionVoucher() return nullable types.
50+
// Without ?->, null->prop on left of ?? throws TypeError (not caught by ??).
51+
$a = $disbursement->getTransaction()?->getTransactionVoucher()?->code ?? '';
52+
53+
// Single nullable method in chain
54+
$b = $disbursement->getTransaction()?->getTransactionVoucher()?->code ?? '';
55+
}
56+
57+
public function nonNullableObject(Transaction $transaction): void
58+
{
59+
// getTransactionVoucher() returns nullable, so ?->code is needed
60+
$a = $transaction->getTransactionVoucher()?->code ?? '';
61+
}
62+
63+
public function allNonNullable(TransactionVoucher $voucher): void
64+
{
65+
// ?->code is unnecessary here: $voucher is non-null, code is non-null string
66+
$a = $voucher?->code ?? '';
67+
}
68+
}

0 commit comments

Comments
 (0)