slevomat / coding-standard

Slevomat Coding Standard for PHP_CodeSniffer provides many useful sniffs
MIT License
1.39k stars 176 forks source link

Vol. 2: SlevomatCodingStandard.TypeHints.ReturnTypeHint.MissingNativeTypeHint suggests void union return type for mixed|void #1220

Closed simPod closed 3 years ago

simPod commented 3 years ago

I'm using v7.0.7 which should have the error fixed https://github.com/slevomat/coding-standard/issues/1211. However, for this code it fails still:

Edit: invalid, see https://github.com/slevomat/coding-standard/issues/1220#issuecomment-832543919


<?php

interface A
{
    /** @return object|void */
    public function b();
}

Method \A::b() does not have native return type hint for its return value but it should be possible to add it based on @return annotation "object|void". SlevomatCodingStandard.TypeHints.ReturnTypeHint.MissingNativeTypeHint

kukulich commented 3 years ago

Yes, because the fixer fix the code to the right type hint object|null.

simPod commented 3 years ago

@kukulich Ah, got it. I tried to reproduce it as close as possible to the original issue and sniffer reported the error still.

In that case it works and only this case remains:

<?php

interface A
{
    /** @return mixed|void */
    public function b();
}

mixed|null can't be used nor mixed|void

kukulich commented 3 years ago

@simPod I don't think there's error, see: https://github.com/slevomat/coding-standard/commit/27348e52a4ebb38aa8dfeb0442e5c62f632c6fe2

simPod commented 3 years ago

void seems not to be covariant with mixed. Sorry for bothering, no sure if I'm missing something.

https://3v4l.org/Gfd9i

kukulich commented 3 years ago

mixed|void does not make sense, the right type hint is mixed|null - but that's invalid so the final right type hint is mixed.

simPod commented 3 years ago

How does it not make sense? It's common use case that implementations can return or not return at all. Eg. when using command bus most commands return nothing though few do. Cannot express absent return statement with return null.

ondrejmirtes commented 3 years ago

@simPod Function either returns a value, or doesn't return anything (and the result isn't supposed to used). Combining void with anything else doesn't make sense. If you used the same thing in native typehints, PHP would report an error:

simPod commented 3 years ago

@ondrejmirtes I get this and my original intention was not to union mixed|void in native type hints. But how should I type interface A in this example then? https://3v4l.org/Gfd9i

I wanted to use

/** @return mixed|void */
public function b();

because implementing methods either return something or nothing at all. So what is the best approach here?

Should I keep no return type hint and suppress sniff warning that type hint is missing?

kukulich commented 3 years ago

Should I keep no return type hint and suppress sniff warning that type hint is missing

Yes, if you still want use invalid type hint (even only in comments).

ondrejmirtes commented 3 years ago

@simPod I'd argue that Y shouldn't have a native typehint at all and it should use @return null in the PHPDoc.

simPod commented 3 years ago

Seems less safe to remove a native void type hint and replace it with @return null doc in each implementation TBH as stated in void return type RFC

function returns_null(): void {
    return null; // Fatal error: A void function must not return a value
}

Also, I checked this https://php.watch/versions/8.0/mixed-type which states that omitting a native return type hint means function having a return type hint of mixed|void. Thus I still believe it's perfectly valid type value in phpdoc to avoid function has no return type hint phpcs error.

So this union seems like valid php type that can exists, though cannot be defined using native type hints.

With never I guess it will become mixed|void|never.

github-actions[bot] commented 3 years ago

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