Return fixer should be risky

[GrahamCampbell]

The return null; to return; fixer we have needs to be marked as risky as it could break code on php 7.1.

See https://wiki.php.net/rfc/void_return_type.

Also note that the rfc actually changes the definition of void from what we thought it was before. It now strictly means nothing rather than just garbage.

[keradus]

Could you provide example of risky change as well?

[GrahamCampbell]

Just have a fuction marked as return void, and put return null; in it. It's beahviour changes before and after the fixer is run.

[GrahamCampbell]

One could argue the code that said return null; was broken already, but who's to say the intention of the original code wasn't to raise a type error.

[GrahamCampbell]

We've made a change to the code behaviour, if you see what I mean.

[keradus]

https://3v4l.org/h0tn2

👍 for change.

Also, it needs to be documented well why it is risky ;)

[GrahamCampbell]

3v4l.org

That website doesn't contain the void return type code that's new in php 7.1.

[GrahamCampbell]

Only HHVM 3.11 actually gave an output of anything close to what php 7.1.0 will give when it's released. The old version of php 7.1 still thought void was the name of an object.

[keradus]

I'm ignoring hhvm...

Behavior on php7.1 will be same way, just a bit different Error will be thrown, isn't it?

[GrahamCampbell]

Yeh, but in terms of recognising void as a type rather than a class name, it was closer.

[GrahamCampbell]

We'll need to wait for the december version of php 7.1 to make it to 3v4l.org

[keradus]

The problem exists even without change to PHP, as showed on 3v4l.

[GrahamCampbell]

https://3v4l.org/1h9P9

[GrahamCampbell]

not really - there's still a type error before and after on 7.0

[keradus]

So let us see what error will be thrown, maybe it will be in same inheritance line.

[stof]

Well, actually, this fixer should be removed from the Symfony level if this RFC is accepted in PHP 7, because the rule defined in the Symfony coding standards will become invalid (I already don't like this rule)

[stof]

OK, actually, the RFC is already accepted. So we should start a discussion in Symfony instead.

[GrahamCampbell]

OK, actually, the RFC is already accepted. So we should start a discussion in Symfony instead.

Doesn't mean another RFC can't be proposed that changes the definition. I preferred the definition that void just meant the returned value shouldn't be used.

[stof]

Doesn't mean another RFC can't be proposed that changes the definition. I preferred the definition that void just meant the returned value shouldn't be used.

I actually prefer this definition: if the return value should not be used, why should the code be allowed to return one ?

[stof]

The problem exists even without change to PHP, as showed on 3v4l.

this is not true. The existing failure on 3v4l is the PHP 7.0 one, which is checking a class typehint.

The validation of the void return type is done at compile time, not at runtime, so they won't be any exception catched in your 3v4l snippet (the try statement will not even be reached)

[GrahamCampbell]

I actually prefer this definition: if the return value should not be used, why should the code be allowed to return one ?

Well, why not?

[GrahamCampbell]

It doesn't matter, that's the point.

[SpacePossum]

note; if the void type will happen we need to split the phpdoc_no_empty_return into two fixers as well, since remove null or void are than 2 diff. things.

[keradus]

change is finally accepted by Symfony

symfony/symfony#17201
symfony/symfony-docs#6614

TODO:

1. remove from symfony ruleset phpdoc_no_empty_return and empty_return
2. split phpdoc_no_empty_return as suggested by @SpacePossum
3. mark empty_return risky for PHP 7.1 reason

[stof]

mark empty_return risky for PHP 7.1 reason

that's not risky. The risky fixer would be the one doing the opposite change (return null; is forbidden in void function, but return; can always be used).

remove from symfony ruleset phpdoc_no_empty_return

This one should not be removed. We haven't changed our phpdoc CS.

[stof]

I created #1916 to suggest another fixer which would be useful in the Symfony ruleset to replace empty_return to match our updated CS.

[keradus]

This one should not be removed. We haven't changed our phpdoc CS.

So we still want to remove both @return null and @return void from PHPDoc ?

[SpacePossum]

So we still want to remove both @return null and @return void from PHPDoc ?

What will be the default return value after 7.1? If it is void (would be an interesting BC but most valid), than we should remove @return void and leave null IMO.

(on a side note; for constructors I still like to remove any @return hint form the PHPDocs)

[keradus]

(constructors are totally different case, eg you shouldn't typehint constructor output with: Foo)

[rob006]

What will be the default return value after 7.1? If it is void (would be an interesting BC but most valid), than we should remove @return void and leave null IMO.

void is only pseudo-type, all functions still returns null by default, including void-functions. There is no need to change return; to return null; or return null; to return; or mark this fixer as risky.

[keradus]

huh?

<?php
function foo(): void {
    return null;
}

PHP 7.1.0alpha1:

Fatal error: A void function must not return a value (did you mean "return;" instead of "return null;"?)

[rob006]

@keradus Are we still talking about return null; to return; fixer?

[stof]

@keradus but the existing fixer is not risky, because it does not add null. It removes it.

[keradus]

@stof removing it could break the code as well, see #2256