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

False positive when comparing ReflectionNamedType::getName() with 'static' #8167

Closed
BenMorel opened this issue Jun 25, 2022 · 5 comments · Fixed by #8201
Closed

False positive when comparing ReflectionNamedType::getName() with 'static' #8167

BenMorel opened this issue Jun 25, 2022 · 5 comments · Fixed by #8201
Labels
bug easy problems Issues that can be fixed without background knowledge of Psalm good first issue internal stubs/callmap

Comments

@BenMorel
Copy link
Contributor

BenMorel commented Jun 25, 2022

When ReflectionNamedType::isBuiltin() returns false, Psalm assumes that getName() returns a class-string:

if ($returnType instanceof \ReflectionNamedType) {
    if (! $returnType->isBuiltin() && $returnType->getName() !== 'static') {
        
    }
}

Psalm output (using commit 8b7bc07):

ERROR: RedundantCondition - 16:39 - 'static' can never contain class-string

Demo: https://psalm.dev/r/2c06d6b1d3

Even though 'static' is also a valid return type since PHP 8.0: https://3v4l.org/ZAqYK

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/2c06d6b1d3
<?php

namespace Test;

class Foo {
    public function bar(): static {
    	return $this;
    }
}

$class = new \ReflectionClass(Foo::class);
$method = $class->getMethod('bar');

$returnType = $method->getReturnType();
if ($returnType instanceof \ReflectionNamedType) {
    if (! $returnType->isBuiltin() && $returnType->getName() !== 'static') {
        
    }
}
Psalm output (using commit 8b7bc07):

ERROR: RedundantCondition - 16:39 - 'static' can never contain class-string

@AndrolGenhald
Copy link
Collaborator

Looks to me like the assertion in the stub needs changed from class-string to class-string|'static'|'self'. Are there any other possible return values we're missing?

@AndrolGenhald AndrolGenhald added bug easy problems Issues that can be fixed without background knowledge of Psalm internal stubs/callmap good first issue labels Jun 27, 2022
@BenMorel
Copy link
Contributor Author

@AndrolGenhald Good point about self, I thought this would be replaced with the FQCN by reflection or even at compilation time, but it looks like it's not, it's actually returning 'self': https://3v4l.org/U3JtZ

I don't see any other possible return value, so class-string|'static'|'self' should be alright.

I couldn't find where the return type is documented, it's not in the CallMap at least. Because the result is dependent on whether isBuiltin() has been called first, I guess there's some custom logic around this class.

@AndrolGenhald
Copy link
Collaborator

It's in stubs/Reflection.phpstub, it just uses @psalm-assert-if-false to assert the type of $this->getName().

@BenMorel
Copy link
Contributor Author

@AndrolGenhald TIL, thanks! Fixed in #8201.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug easy problems Issues that can be fixed without background knowledge of Psalm good first issue internal stubs/callmap
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants