False positive on return null|void

[dereuromark]

Bug report

	/**
	 * Overview action
	 *
	 * @return \Cake\Http\Response|null|void
	 */
	public function index() { ... }

Framework controller actions usually don't have to return, the |void addition should suffice to silence PHPStan.

But it complains with

Method DatabaseLog\Controller\Admin\LogsController::index() should
return Cake\Http\Response|void|null but return statement is missing.

Expected output

No error here. Only with \Cake\Http\Response|null I would expect the above message.

[mergeable]

This bug report is missing a link to reproduction on phpstan.org.
It will most likely be closed after manual review.

[challgren]

Here you go @mergeable https://phpstan.org/r/3be445ca-511c-4557-ae3d-aedcb952ec0e

[ondrejmirtes]

Hi, it doesn't make much sense to combine void with other return types...

[ondrejmirtes]

You're either not returning anything, or you're returning something. In that case, return; can be exchange with return null;.

[dereuromark]

In that case, return; can be exchange with return null;.

No, it cannot. As in that case the return statement must be present semantically.
But that is not possible or desired in many high level (controller/command) calls as well as callbacks etc.

Thus you use null|void to say it may have a return statement, but can also be omitted.
It is a soft-case in between and fully valid as such.

Please adjust this tool to not false positive on these cases, as it otherwise compromises its usage on many projects. That would be very sad.

[dereuromark]

Note:
If a project may decide to not allow this soft case (decide for hard typehints), they can simple add a cs sniffer or other tooling to do so, but for all others this is still not sth that should be disallowed by phpstan false-positive reporting these things.

[ondrejmirtes]

I understand, I’ll think about it.

On Fri, 6 Dec 2019 at 18:08, Mark Sch. ***@***.***> wrote: Note: If a project may decide to not allow this soft case (decide for hard typehints), they can simple add a cs sniffer or other tooling to do so, but for all others this is still not sth that should be disallowed by phpstan false-positive reporting these things. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2643?email_source=notifications&email_token=AAAZTODA6MF3AA2HOEFOT63QXKBILA5CNFSM4JVBE4CKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGEXQRI#issuecomment-562657349>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZTOENPGX7LWBK5HY5BD3QXKBILANCNFSM4JVBE4CA> .

-- Ondřej Mirtes

[dereuromark]

Note: This is also well understood by IDEs, e.g. PHPStorm.
They hint in yellow if the return type is missing for ...|null and it becomes fine if ...|null|void - which it should.

We need this for several libraries and cannot otherwise continue migrating to PHPStan 0.12. It currently is a hard blocker.

[GrahamCampbell]

PHP 8's union types implementation doesn't allow void to be in a union with anything. PHPStan should instead complain the return typehint is garbage, IMO. null|void is not a valid type.

[GrahamCampbell]

// cc @nikic

[GrahamCampbell]

Hi, it doesn't make much sense to combine void with other return types...

Exactly.

[dereuromark]

PHP 8's union types implementation doesn't allow void to be in a union with anything. PHPStan should instead complain the return typehint is garbage, IMO. null|void is not a valid type.

The validity is currently not the question. Even now in PHP7 no one can and should actually typehint as such.
This is about the docblock and documentation on the subject referring to the case where you do not have a typehint in place, and the code can or can not contain a return statement.
Please do not talk about garbage or alike here in this instance - as you are missing the point.

As a 2nd point and side effect ...|null|void also helps to not have cs sniffers and typehinting sniff auto-add wrong typehints where no typehint currently is acceptable to be added. But again, just a side effect of this.

[dereuromark]

If we can have a config option to toggle this on/off that would also be fine IMO. Just that by default it now complains about something it should not complain (stay silent about as this is the scope of different tooling/syntax to assert). And this is the issue at hand.

[GrahamCampbell]

should actually typehint as such

I disagree. null|void should not be allowed as a type, and isn't in PHP 8.

[GrahamCampbell]

I don't view PHP 8's implementation as a change from PHP 7. It is how it was always intended to be. It was just never implemented.

[GrahamCampbell]

You should change your code to return null in place of all the places it would have not returned.

[dereuromark]

null|void should not be allowed as a type, and isn't in PHP 8.

Exactly, I didnt say it should be allowed as type. I am referring to the docblock line only (which is not a type, it is a combination of documented types). Big difference.

You should change your code to return null in place of all the places it would have not returned.

That is exactly what in this case, with also no typehint in place, is not what should have to be forced.

[dereuromark]

@GrahamCampbell If you are not using this semantic type doc pair (and it sounds like you never would), why do you care about this?
You should not have to.

[GrahamCampbell]

I am referring to the docblock line only (which is not a type, it is a combination of documented types).

I don't agree with this view. A union of types is a type, and void is not allowed in the union. The phpdoc is meant to document the type. Not the types.

[dereuromark]

I don't agree with this view. A union of types is a type, and void is not allowed in the union. The phpdoc is meant to document the type. Not the types.

The docblock is also mainly for humans and IDEs/tooling to further intospect/use the code.
Otherwise you could also just omit it and only use/refer to the actual code type in place.

So it is given/good standard/practice to add semantic additions on top of what you define as the true type.

Again: This is only for the ommited typehint and as such, you will most likely have a CS sniff in place to not allow the combination there and then to force or auto-add the typehint void or null and all is well for you.
This is not something you should necessarily be affected/worried by.

[klausi]

Ran into the same problem for Drupal's Coder, our workaround is to ignore this specific message for now https://github.com/pfrenssen/coder/blob/8.x-3.x/phpstan.neon#L54

A return type void means that the function does not always have to return something.

[dereuromark]

Exactly :)
My point still stands: PHPStan should not report the false positive above on existence of void in the tool type(s).
This is for other tools and means to detect/error if necessary/desired. PHPStan should be adjusted to allow the missing return statement in the body as outlined.

If void is part of the types, it should just abort if there indeed no return statements inside the body.

[ondrejmirtes]

I was surprised that the other side was already fixed in PHPStan: phpstan/phpstan-src@80afe10

I now fixed the remaining case, e.g. return in a function isn't required in case of void union: phpstan/phpstan-src@e208c05

We might rethink this when PHP 8 is out. PHPStan typically wants the same constraints from phpDoc types as from the native types (so that they can be automatically converted by other tools).

[ondrejmirtes]

Released as 0.12.5 :)

[lock]

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.