bmewburn / vscode-intelephense

PHP intellisense for Visual Studio Code
https://intelephense.com
Other
1.63k stars 94 forks source link

Lack of diagnosis: attempting to store result of a `void/never` function call #2204

Open Paril opened 2 years ago

Paril commented 2 years ago

Describe the bug Functions that return void or never should never be allowed to be assigned to anything.

To Reproduce

function test(): void
{
}

$x = test();

Expected behavior Diagnosis for "void/never should not be assigned to a variable". This isn't a PHP error (unfortunately...) but it should be a simple diagnostic change.

tianyiw2013 commented 2 years ago

Similar #1323

KapitanOczywisty commented 2 years ago

Void function returns null and this is valid syntax. I'm not even sure if any static analysis tool has check like that.

Paril commented 2 years ago

Void function returns null and this is valid syntax. I'm not even sure if any static analysis tool has check like that.

Yeah I know it's technically valid, but in what context would you ever want this behavior? The only reason I can think of for this to ever happen in production code is an oversight. Looking at PHP's justification:

Attempting to use a void function's return value simply evaluates to null, with no warnings emitted. The reason for this is because warnings would implicate the use of generic higher order functions.

I'm not exactly certain what they meant by this or how this would cause any issues, but maybe there's a certain pattern in PHP that I've not seen before that may be affected by this kind of diagnostic (perhaps a built-in function like array_map that is called with a callable that doesn't return a value...?)

EDIT: okay, I found an explanation, and they too were confused by the wording of the PHP feature post. I see the use case now: call wrappers that can call any function, whether they return a value or not. However, the RFC does mention that a diagnostic of this type is useful, and relies on them doing that rather than enforcing it at a language level:

Moreover, IDEs and other tools can warn the user when the return value of a void function is being used. It isn't strictly necessary for the language itself to cover this.
KapitanOczywisty commented 2 years ago

in what context would you ever want this behavior?

I'm not sure if assigning void to variable is ever good solution, but PHP doesn't care about that, there are many ugly things allowed and widely accepted in PHP.

As for why void can be used in expressions (beside historical reasons), for example you may want to pass void callback when ?SomeObject is expected, but you will never return that object.

Intelephense, as far I know, is not intended to prevent people from writing ugly code, thus unlikely void assignment would be detected. However never probably should be, at least as part of dead code detection (#1718).

Static analyzers are more suited for detecting void assignment, at least phpstan has check for that: https://phpstan.org/r/57b36b31-3a5c-44b4-8bd0-eb10746b6ac7