False positive: Using nullsafe property access on non-nullable type

[schlndh]

Bug report

It seems that phpstan 1.6 gets confused when using ?-> together with ?? and complains about ?-> even when the left side of it is indeed nullable.

Code snippet that reproduces the problem

https://phpstan.org/r/b9aade5f-f220-4b10-934f-dd6338acfe05

Expected output

No issues.

[ondrejmirtes]

/cc @rajyan Looks like a job for you :) Thanks.

[rajyan]

@ondrejmirtes @schlndh
I think I can fix it, but not sure this should be fixed.
The null safe property fetch is redundant because it cannot be null (when property fetch is called) in coalesce.
verified https://3v4l.org/MJenT

The error message should be improved though.

[ondrejmirtes]

Oh nice, I haven't realized PHPStan is actually right here!

[schlndh]

Hmm, you're right that it works (I didn't realize that). But since the left side can be null, I don't think it's necessary to complain about the ?-> even though it's not required for properties when combined with ?? (to suppress the warning).

[rajyan]

@schlndh

I don't think it's necessary to complain about the ?->

Agreed with you.
I'll look into it today to look for whether there is a fix without messing the non null context in coalesce!

[rajyan]

These are the related issues and PR (why this issue started to occur)
#4559
#3171

phpstan/phpstan-src#1234

[p4veI]

Hello, I came across this issue with native php enum, replicated here:

https://phpstan.org/r/8525ac50-91f9-4c0d-a343-36842dbdf9fe

Hope it helps

[ondrejmirtes]

It might be annoying, but PHPStan is right :) This code is functionally equivalent: $foo = $bar->foo->value ?? (Foo::A)->value;

So we might be able to improve the error message, but otherwise this error is going to continue be reported :)

[p4veI]

Oh yeah, you're right my mistake. Sorry 🤦

[jacekkarczmarczyk]

tbh i'm not sure if it should be checked, without this rule code could be more consistent:

<?php

class Foo
{
	public string $str = '';
	public function test(): string
	{
		return '';
	}
}

class Bar 
{
	public ?Foo $foo = null;
}

$bar = new Bar;
echo $bar->foo?->str ?? 'one';
echo $bar->foo?->test() ?? 'two';

In the example above php indeed doesn't require ? in the first echo but it does require it in the second one

[p4veI]

In the example above php indeed doesn't require ? in the first echo but it does require it in the second one

but that doesn't produce any errors in my case though..

[rajyan]

@jacekkarczmarczyk
PHPStan can correctly handle the method call case because I intentionally implemented like that :)

https://phpstan.org/r/c5fbd20e-c88a-47e9-a6e6-e45a983ed5ea

but looking at number of reports from users, I was thinking that it might be better not to report it in property fetch case too.

[jacekkarczmarczyk]

but that doesn't produce any errors in my case though..

Not sure what you mean? It does produce the error as shown in the @rajyan comment.
Maybe my comment was not clear so I'll explain - I mean that if we fix the code to make phpstan happy we'll end up with

echo $bar->foo->str ?? 'one';
echo $bar->foo?->test() ?? 'two';

where as you see sometimes foo-> is used and sometimes foo?->, which I'm not a fan of as it's not consistent

[rajyan]

@jacekkarczmarczyk
Thanks, understood your intending.

[rajyan]

@ondrejmirtes
I am starting to think that reporting this behavior is a little opinionated. It is not clear enough that null safe is not needed at a glance, and deleting the null safe doesn’t worth much benefit (sometimes worse readability?) as looking at number of reports from users.

[ondrejmirtes]

@rajyan I consider it useful and from the same category as "Strict comparison using === between string and null will always evaluate to false.".

My thinking is that the path forward is:

• Stop reporting this scenario by default.
• Report it in bleedingEdge with a different message - something like "Using nullsafe property access on the left side of ?? is not necessary."

Could you please implement that? Thanks.

[zonuexe]

I like this checker, but it doesn't seem to fit the actual use cases.

// This point is useful because neither getUser() nor User->user_id are nullable.
$user_id_1 = getUser()?->user_id ?? 0; // expected

// This point is verbose because `null?->user_id` returns null.
$user_id_2 = getUserOrNull()?->user_id ?? 0;

https://phpstan.org/r/19a8fa59-c950-49d7-972b-8dc9401c49da

[rajyan]

@zonuexe

The point (and where opinions are divided) here is that in coalesce/isset/empty null?->prop and null->prop are functionally equivalent, so the null safe is (or can be said) unnecessary.

null->test ?? "test";
isset(null->test) ?: "test";

null->test->call() ?? "test";
empty(null->test->call()) ?: "test";

https://3v4l.org/fltef
Property fetch would not be called if the object is null.
If there is a method call, all expression before method call is evaluated.

PHPStan is understanding that behavior right, but because the expression before null safe can be null, at least the error message is wrong.
https://phpstan.org/r/0bf05ff8-7bc1-43b9-8f66-e96646583d82

[zonuexe]

@rajyan Thank you, I finally understand.

<?php

var_dump(null->user_id); // Raise the warning as expected
var_dump(null->user_id ?? 0); // Without Warning

https://3v4l.org/LZ0RH

Focusing on using ?->, I seem to have missed that it was the original feature of ??.
It may be a common perception for people like me who write these codes.

Knowing that, I could understand that the focus was only on whether to warn of such preventative and verbose code.

[ondrejmirtes]

Nightmare is over! Fix is released :) https://github.com/phpstan/phpstan/releases/tag/1.6.2

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.