Instanceof checks aren't properly reconciled in "else" context

[tscni]

Some examples with variable re-definitions: https://psalm.dev/r/2cf106db54
In a(), $x should be 'x'
In b(), $x should be X
In c(), the instanceof check should not be redundant
d() works correctly

I mainly include the class-string cases because them working (in general) is the cause of the issue. This relates to #6739 and #6746
Specifically (in the negated case) this part of the code:

psalm/src/Psalm/Internal/Type/NegatedAssertionReconciler.php

Lines 198 to 204 in 56d7b37

	} elseif ($existing_var_type->isSingle()
	&& $existing_var_type->hasNamedObjectType()
	&& $assertion_type instanceof TNamedObject
	&& isset($existing_var_type->getAtomicTypes()[$assertion_type->getKey()])
	) {
	// checking if two types share a common parent is not enough to guarantee childs are instanceof each other
	// fall through

I'm currently trying to figure out how I can differentiate class-string<X> from X or 'X', which would be necessary to fix the issue.
Maybe I could implement something similar to TNamedObject::$definite_class, e.g. a $definite_type which would be true if it's definitely X / 'X', but not if it's class-string<X>

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/2cf106db54

<?php

class X {}

function a(X $x): void {
    /** @psalm-suppress RedundantCondition */
    if ($x instanceof X) {
        $x = 'x';
    }
    /** @psalm-trace $x */
}

function b(X $x): void {
    /** @psalm-suppress RedundantCondition */
    if (!$x instanceof X) {
        $x = 'x';
    }
    /** @psalm-trace $x */
}

/** @param class-string<X> $xClass */
function c(X $x, $xClass): void {
    if ($x instanceof $xClass) {
        $x = 'x';
    }
    /** @psalm-trace $x */
}

/** @param class-string<X> $xClass */
function d(X $x, $xClass): void {
    if (!$x instanceof $xClass) {
        $x = 'x';
    }
    /** @psalm-trace $x */
}

Psalm output (using commit 653ad66):

INFO: Trace - 10:0 - $x: 'x'|X

INFO: Trace - 18:0 - $x: 'x'|X

ERROR: RedundantCondition - 23:9 - Type X for $x is always X

INFO: Trace - 26:0 - $x: 'x'|X

INFO: Trace - 34:0 - $x: 'x'|X

[orklah]

Yeah, I remember banging my head on that multiple times :p

the issue with $definite_type is that unless a class is final, it's pretty much impossible to know. (Even if there is no child known for this type, it may be a library that expect people to implement their own)

There's definitely room for improvement anyway!

[kkmuffme]

At least for cases where the type after the "instanceof" is a literal (and not a variable) this is relatively simple to fix via a plugin (via AfterExpressionAnalysisEvent for Instanceof_) so that the negation at least reports a redundant condition (https://psalm.dev/r/4ca7336f15)
I guess that can be done similarly in psalm itself directly, just not with the assertion system, since we don't know at that stage if it's a literal or not.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/4ca7336f15

<?php

class X {}

function b(X $x): void {
    if (!$x instanceof X) {
        $x = 'x';
    }
    /** @psalm-trace $x */
}

Psalm output (using commit 5095f4e):

INFO: Trace - 9:0 - $x: 'x'|X