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

get_class does not return false #7042

Merged
merged 1 commit into from Dec 2, 2021
Merged

Conversation

kamil-tekiela
Copy link
Contributor

Technically all functions could return false or null when a wrong parameter is provided, but that was undefined behavour in PHP <8 and should not be relied upon.

Technically all functions could return false or null when a wrong parameter is provided, but that was undefined behavour in PHP <8 and should not be relied upon.
@kamil-tekiela kamil-tekiela marked this pull request as draft December 1, 2021 21:49
@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Dec 1, 2021
@kamil-tekiela
Copy link
Contributor Author

Ok, it did return false prior to PHP 8. I will change the PR to account for this tomorrow.

@weirdan
Copy link
Collaborator

weirdan commented Dec 1, 2021

Did it ever return false without also producing a warning?

@kamil-tekiela
Copy link
Contributor Author

Did it ever return false without also producing a warning?

Not to my knowledge. See https://heap.space/xref/PHP-7.4/Zend/zend_builtin_functions.c?r=1e0bc6e3#955

@orklah
Copy link
Collaborator

orklah commented Dec 1, 2021

not since PHP 5.3:
https://3v4l.org/XBMmE
https://3v4l.org/clSSM
https://3v4l.org/n7SgA

Psalm is already emitting an error in all those cases. The only case where Psalm does not emit error is: https://psalm.dev/r/781da1e35e but it gives a warning even in PHP 5.2. I say we can get rid of the false return as we already did on such cases for other functions

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/781da1e35e
<?php
var_dump(get_class());
Psalm output (using commit bc11b10):

ERROR: ForbiddenCode - 2:1 - Unsafe var_dump

@weirdan
Copy link
Collaborator

weirdan commented Dec 1, 2021

If we can reliably flag cases where it would return false then we don't need |false in the signature.

There's one I can think of: calling get_class() without arguments in a non-class context: https://psalm.dev/r/58287e0c4e. I think it could be handled similarly to __CLASS__ reference in a non-class context: https://psalm.dev/r/dd677898db

@psalm-github-bot
Copy link

psalm-github-bot bot commented Dec 1, 2021

I found these snippets:

https://psalm.dev/r/58287e0c4e
<?php

echo get_class();
Psalm output (using commit bc11b10):

No issues!
https://psalm.dev/r/dd677898db
<?php
echo __CLASS__;
Psalm output (using commit bc11b10):

ERROR: UndefinedConstant - 2:6 - Cannot get __class__ outside a class

@orklah
Copy link
Collaborator

orklah commented Dec 1, 2021

It's not flagged by Psalm but it never worked AND always emitted a warning or worse: https://3v4l.org/TkeS5 (weirdly, it started as a warning, then was an error, again a warning and it's now an error!)

@weirdan
Copy link
Collaborator

weirdan commented Dec 1, 2021

but it never worked AND always emitted a warning

It's a good indication that we need to flag it.

@orklah
Copy link
Collaborator

orklah commented Dec 1, 2021

That should do it: #7043

@kamil-tekiela
Copy link
Contributor Author

What about this PR? Is it ok to just remove |false from all versions or should I add delta?

@weirdan
Copy link
Collaborator

weirdan commented Dec 2, 2021

Just dropping |false would be perfect.

@kamil-tekiela kamil-tekiela marked this pull request as ready for review December 2, 2021 00:39
@orklah orklah merged commit 396ff96 into vimeo:master Dec 2, 2021
@orklah
Copy link
Collaborator

orklah commented Dec 2, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants