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

void|never shouldn't be combined to null but keep void #10759

Open
wants to merge 3 commits into
base: 5.x
Choose a base branch
from

Conversation

kkmuffme
Copy link
Contributor

@kkmuffme kkmuffme commented Feb 29, 2024

Since psalm internally often checks for isVoid (or isNever) which should include cases of void|never from what I saw, but since the type is changed to null some errors aren't reported.
Initiated by #10742 (review)

@kkmuffme kkmuffme marked this pull request as ready for review February 29, 2024 15:12
@kkmuffme
Copy link
Contributor Author

kkmuffme commented Feb 29, 2024

@weirdan review & merge please

@@ -533,9 +533,9 @@ function bar() {
}',
'output' => '<?php
/**
* @return null|string
* @return string|void
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

void in a union looks wrong. Looking at the code here I'd expect null|string actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But actually makes sense here - since it uses implicit return; and not explicit return null; therefore I guess it makes sense, as this clearly distinguishes these 2 cases. E.g. some errors should only report on explicit null but not on void.

I know this isn't possible in native PHP types, but we're in a docblock.

Comment on lines +3187 to +3188
* this is just one example where psalm uses isVoid and isNever, which is why void|never should not be collapsed to null
* @param void|never $i
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should result in void (as never is the bottom type) and then trigger ReservedWord

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as never is the bottom type

Yes. but when used explictly in docblock never doesn't behave as the bottom type since a long time already on purpose (pretty much only relevant for @return though)

This test is purely to highlight the changes and isn't a feature implemented in this PR, but a bug fix - since currently it this doesn't report an error at all https://psalm.dev/r/bcbea8b9ef

There are various other places that have this bug in psalm and I don't have time to fix them all to implement their checks correctly - that should have happened at the time those were added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants