Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return type void is adopted by variables #6720

Closed
JanJakes opened this issue Mar 2, 2022 · 11 comments
Closed

Return type void is adopted by variables #6720

JanJakes opened this issue Mar 2, 2022 · 11 comments
Labels
Milestone

Comments

@JanJakes
Copy link

JanJakes commented Mar 2, 2022

Bug report

Return type void is adopted by variables although no such type exists.

Code snippet that reproduces the problem

https://phpstan.org/r/3fd8ffd6-0e6d-489a-9616-87ba446a038f

Expected output

No error. In PHP void becomes null. See: https://3v4l.org/6eTAd

@canvural
Copy link
Contributor

canvural commented Mar 2, 2022

I think it doesn’t make sense to combine void with another type in a union. You can use string|null instead

It’s also not allowed in native union types in PHP 8: https://3v4l.org/TnGBT

@JanJakes
Copy link
Author

JanJakes commented Mar 2, 2022

@canvural
Thanks for your response. I'm not using it but 3rd party code does. Moreover, even if one can avoid it, it's still a bug.

An example is WordPress itself:

$wpdb->prepare(...); // returns string|void
$wpdb->get_row(...); // accepts string|null

See source for prepare and for get_row.

@mvorisek
Copy link
Contributor

related with #9388

@ondrejmirtes
Copy link
Member

Fixed: phpstan/phpstan-src#2778

@jrfnl
Copy link

jrfnl commented Dec 12, 2023

The fix for this is breaking things. Maybe this should have gone into a minor, not a patch release ?
It also causes duplication with the Level 0 "Result of method * (void) is used" error.

@ondrejmirtes
Copy link
Member

Hello @jrfnl,
please note that one person's bugfix can be another person's failed build.

That's why I recommend pinning to a specific PHPStan version and update it with automated PRs from Dependabot or Renovate to have it under control.

If you have composer.lock commited then it's fine to have a range in composer.json like ^1.10.40.

If you don't have composer.lock commited then you should pin to a specific version in composer.json like 1.10.40.

With this recommendation the build is not suddenly going to fail because of an unintended PHPStan update.

In any case please reproduce the issue on phpstan.org/try and open a bug report in phpstan/phpstan repo.

Thank you!

P.S. I appreciate your work on PHPCS, it's one of my favourite projects!

@jrfnl
Copy link

jrfnl commented Dec 12, 2023

@ondrejmirtes Thanks and happy to hear you like what you see in PHPCS!

I will try and open an issue when I find the time. Might be a while. If you want more info sooner, here's a PR to fix a failing build: WordPress/WordPress-Coding-Standards#2416

And yes, this is a PHPCS related project and for those, I cannot easily "pin" PHPStan version, I cannot even add it in the composer.json file as the token backfills which PHPStan provides (via nikic/php-parser) interfere with PHPCS itself, which would make our tests unreliable.

Currently running it locally via a PHAR and in CI via the tools from setup-php. I might need to look into adding PHIVE into the mix to be able to pin the versions, but it just makes things (even more) complex for contributors and I don't know whether Dependabot can handle PHIVE, which if it doesn't would make updating manual again, which is a pain.

@ondrejmirtes
Copy link
Member

Yeah I wouldn't recommend Phive as it's not one of the supported installation methods here: https://phpstan.org/user-guide/getting-started

Best way is through Composer which allows you to also use PHPStan extensions.

Second best option is to just download the PHAR from https://github.com/phpstan/phpstan/releases which should make it easy to pin it to a specific version through URL like https://github.com/phpstan/phpstan/releases/download/1.10.49/phpstan.phar.

And don't worry, I understand semver. The expectations users should have are described in these articles:

PHPStan shares the same challenges with similar projects like TypeScript, which also can't avoid breaking users' builds in patch/minor versions.

@jrfnl
Copy link

jrfnl commented Dec 12, 2023

Yeah I wouldn't recommend Phive as it's not one of the supported installation methods here: https://phpstan.org/user-guide/getting-started

Understood.

Best way is through Composer which allows you to also use PHPStan extensions.

... which unfortunately isn't an option as I explained above.

Second best option is to just download the PHAR from https://github.com/phpstan/phpstan/releases which should make it easy to pin it to a specific version through URL like https://github.com/phpstan/phpstan/releases/download/1.10.49/phpstan.phar.

And that's basically what we currently do, just without the pinning. I deliberately didn't pin the version in the original setup as it means manual update rounds to keep up to date as Dependabot doesn't handle this kind of thing.

I'll have a look whether setup-php accepts a version number for PHPStan, that could stabilize things for now, though still would mean manual updates.

And don't worry, I understand semver. The expectations users should have are described in these articles:

PHPStan shares the same challenges with similar projects like TypeScript, which also can't avoid breaking users' builds in patch/minor versions.

Understood and thank you for your help (and for PHPStan) !

@jrfnl
Copy link

jrfnl commented Dec 12, 2023

@ondrejmirtes Just ran into another one which seem related as well:

Method *::process() should return int|void but returns int|null.

Note: the method never returns null, it really only returns int|void.

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants