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

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.

About this issue

  • Original URL
  • State: closed
  • Created 2 years ago
  • Reactions: 1
  • Comments: 21 (14 by maintainers)

Most upvoted comments

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

@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.

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 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.

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

@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

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

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 😃

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

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

Hope it helps

@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!

@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.