EnforceNativeReturnTypehintRule: try narrowing enforcement

[janedbal]

• attempt, but when tested on big codebase, there are plenty problematic situations, like
  • typehint says interface, but we return multiple children -> forcing to use union of them
  • anon classes returned (fixable)
  • invalid (old) phpdocs like Generator|mixed[] breaking proper suggestion -> forcing generinc iterable in this example
  • invalid phpdocs (this is not reported by phpstan) like this -> forcing wrong native typehint
    • now checked by our rule

[enumag]

Hmm 🤔

typehint says interface, but we return multiple children -> forcing to use union of them

I think that the rule could simply ignore such cases. If the narrow typehint results with single type or a union of null + one other type then enforce it, otherwise ignore.

anon classes returned (fixable)

Similarly to the above such case can be ignored.

invalid (old) phpdocs like Generator|mixed[] breaking proper suggestion -> forcing generinc iterable in this example

I don't think this is a problem. The rule won't give you the best suggestion how to fix it but you can fix it better when it reports the error.

invalid phpdocs (this is not reported by phpstan) like this -> forcing wrong native typehint

This isn't necessarily a problem now that you have a rule to check it I think.

[janedbal]

Yeah, I might try to revive this one one day with some ignores. I still want to have something like that.

Or you can try if you want, but better to try it on some real-world codebase :)

[enumag]

I'm trying it on our main project now. The vast majority of reports is about changing self to static. Is that intentional? I don't quite see static as narrower personally. Especially when the class is final which it nearly always is in my case.

Also a funny things I ran into:

Native return typehint is ?\Ramsey\Uuid\UuidInterface, but can be narrowed to mixed 
Native return typehint is string, but can be narrowed to mixed 

Anyway after ignoring some of the errors the pattern I see is this: errors to change void to never are quite useful. However in many other cases it wants to change interface type to implementation which I actually don't like in most cases as I don't want the callers to rely on the specific implementation (typically I don't want to change iterable types to array or Generator). So in the end I have to admit I don't actually like this all that much.

[enumag]

Also the void to never seems to actually be false-positive sometimes...

public function x(): void
{
    if (...) {
        return;
    }

    throw ...;
}

The rule said this can be narrowed to never which is incorrect.

Sadly in the end I don't like the rule enough to try and finish it myself. But maybe my notes will be useful if you look into it later.

[janedbal]

Thanks for trying anyway :) I'll check those issues once I come back to this.