Don't give UndefinedClass error in instanceof when declared as global

[kkmuffme]

https://psalm.dev/r/748ed1ea68

When working with optional libraries, there are tons of those errors. While they generally make sense, for "instanceof" checks where the variable type is declared (with either @global or @var maybe too), this shouldn't report an error. As this is what this check is for.

Just like isset( $foo ) does not give an UndefinedVariable error, I expect the same behavior for instanceof and UndefinedClass

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/748ed1ea68

<?php

/**
 * @global FooBar|null $foo
 * @return void
 */
function abc() {
	global $foo;
    if ( $foo instanceof FooBar ) {
        echo "hello";
    }    
}

Psalm output (using commit afe85fa):

ERROR: UndefinedClass - 9:26 - Class, interface or enum named FooBar does not exist

[AndrolGenhald]

Not sure I like that solution, but we should probably figure something out. Surprisingly this still shows an error, but this is fine.

I think a better solution might be to create a separate UndefinedInstanceofClass error that can be globally suppressed separately from other UndefinedClass errors, or have a non-default config to disable showing UndefinedClass errors for instanceof. What do you think @orklah?

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/e1fbf7e718

<?php

/**
 * @global FooBar|null $foo
 * @return void
 */
function abc() {
	global $foo;
    if ( class_exists(FooBar::class) && $foo instanceof FooBar ) {
        echo "hello";
    }    
}

Psalm output (using commit afe85fa):

ERROR: UndefinedClass - 9:57 - Class, interface or enum named FooBar does not exist

https://psalm.dev/r/eed4314e00

<?php

/**
 * @global FooBar|null $foo
 * @return void
 */
function abc() {
	global $foo;
    if (class_exists(FooBar::class)) {
        if ($foo instanceof FooBar) {
        	echo "hello";
        }
    }    
}

Psalm output (using commit afe85fa):

No issues!

[kkmuffme]

I think the way class_exists suppresses it is fine and should be xtended to instanceof.

If we add UndefinedInstanceofClass, then this must give an error too: https://psalm.dev/r/efcdbbdfa0 (which would be kind of bad to be honest, as it would lead to 1000s of errors in many codebases)

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/efcdbbdfa0

<?php

/**
 * @global FooBar|null $foo
 * @return void
 */
function abc() {
	global $foo;
    if ( isset( $foo ) ) {
        echo "hello";
    }    
}

Psalm output (using commit afe85fa):

No issues!

[AndrolGenhald]

If we add UndefinedInstanceofClass, then this must give an error too: https://psalm.dev/r/efcdbbdfa0

Why would that give an error too? I actually think that should be treated as an UndefinedDocblockClass the same way this is, but that seems unrelated to instanceof.

I don't like ignoring undefined classes for instanceof because it's helpful to catch typos. For class_exists the only purpose of the function is to check if the class exists, so if the function is being used there must be a case where the class might not exist, so we don't care about UndefinedClass.

To be honest I'm not really convinced we need to change anything, if you look at config.xsd ClassIssueHandlerType has a referencedClass attribute, so it probably wouldn't be a ton of work to just blanket suppress all UndefinedClass and UndefinedDocblockClass issues for a library. Maybe we could try adding a namespace option to that to make it even easier?

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/efcdbbdfa0

<?php

/**
 * @global FooBar|null $foo
 * @return void
 */
function abc() {
	global $foo;
    if ( isset( $foo ) ) {
        echo "hello";
    }    
}

Psalm output (using commit afe85fa):

No issues!

https://psalm.dev/r/c7a2af30e0

<?php

/**
 * @global FooBar|null $foo
 * @param FooBar|null $bar
 * @return void
 */
function abc($bar) {
	global $foo;
    if ( isset( $foo ) ) {
        echo "hello";
    }    
}

Psalm output (using commit afe85fa):

ERROR: UndefinedDocblockClass - 5:11 - Docblock-defined class, interface or enum named FooBar does not exist

[orklah]

You can have a project that can work without a specific library at runtime, but I don't get the concept of checking a lib that is not even loadable at some point.

And if you somehow have this, I don't see why you wouldn't just make it so that Psalm has the lib available on analysis. Having missing classes must be the worst thing you can do to a static analyzer. There are cases when Psalm analyze your code, if it catch an UndefinedClass, it just falls on the ground and give up the whole block of code after that.

Suppressing explicitly the only thing that tells you that Psalm just stopped looking at whatever you may have done wrong is not a good idea either. Suppressing it automatically would be incredibly tricky for users to debug and we're bound to have issues by user complaining that Psalm just didn't tell them something was wrong

[AndrolGenhald]

That's a good point, I forgot UndefinedClass causes Psalm to just give up and skip further analysis. Probably the only good way to handle this is to just have the optional library installed when analyzing. It would be nice to have a way to analyze it without the optional library and make sure everything that doesn't need that library still works, but I don't think that's something Psalm can provide anytime soon.

[kkmuffme]

You can have a project that can work without a specific library at runtime, but I don't get the concept of checking a lib that is not even loadable at some point.

I have a library (= I am the library author) and I need to check which class is loaded in a global variable (not declared by me).

e.g. global $foo;

$foo can have class X, Y, Z depending on the environment. I do not actually use $foo/the class it is set to, but only use it to check in which "environment" the library is running. e.g. if $foo instanceof X => I'm in environment A, $foo instanceof Y => I'm in environment B

I cannot load the class or stubs, since I do not have the code of that/these classes

What I did now is just ignore this error for this class globally.

===

How about in this case: https://psalm.dev/r/a0676afa53

Where I explicitly checked for class_exists, maybe instanceof shouldn't give an error then?

it just falls on the ground and give up the whole block of code after that.

Will this be a problem in my use case? (if not, I guess I will just leave it as it is)

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/a0676afa53

<?php

/**
 * @global FooBar|null $foo
 * @return void
 */
function abc() {
	global $foo;
    if ( class_exists( FooBar::class ) && $foo instanceof FooBar ) {
        echo "hello";
    }    
}

Psalm output (using commit afe85fa):

ERROR: UndefinedClass - 9:59 - Class, interface or enum named FooBar does not exist

[AndrolGenhald]

How about in this case: https://psalm.dev/r/a0676afa53

Where I explicitly checked for class_exists, maybe instanceof shouldn't give an error then?

Yeah, I mentioned that in my first reply, I think our handling of && and || needs improved. It works fine if you nest it in a separate if. I think that would probably be the "safest" solution for Psalm, since I don't think that'll cause it to give up further analysis.

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/a0676afa53

<?php

/**
 * @global FooBar|null $foo
 * @return void
 */
function abc() {
	global $foo;
    if ( class_exists( FooBar::class ) && $foo instanceof FooBar ) {
        echo "hello";
    }    
}

Psalm output (using commit afe85fa):

ERROR: UndefinedClass - 9:59 - Class, interface or enum named FooBar does not exist