Fix added null type after if

[MoonE]

Fixes #9395, but not #9348

Maybe the condition needs to be refined according to this:

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

Lines 478 to 481 in 222887a

	} elseif (!($statements_analyzer->getSource()->getSource() instanceof TraitAnalyzer)
	|| ($key !== '$this'
	&& !($existing_var_type->hasLiteralClassString()
	&& $assertion instanceof IsAClass))

With a simple else instead of elseif ($key !== '$this') these three tests started to fail:

• ClassTemplateExtendsTest => inferPropertyTypeOnThisInstanceofExtended
• TraitTest => staticClassTraitUser
• TraitTest => getClassTraitUser

Some helpful comments for debugging: 6638deb

As noted above, I'm very unsure about the correct condition needed here, but this seems to work with the current test base.

[weirdan]

🤔

At first glance, this fix looks dubious to me, especially the fact that it touches the refine() method. I suspect the correct fix would have something to do with either branch interruption logic or the distinction between asserted and assigned variables.

[orklah]

It makes some sense though. The issue seems definitely linked to reconciliation to me, not control flow.

See snippet: https://psalm.dev/r/9432deb60a
Psalms sees the redundant condition but correctly infers that the type inside the if is -1. However, it still tries to reconcile -1 and null and somehow it results in null in the else branch. That's the error, this should be never because we can't enter in the else (at least when we have a type that's not from_docblock). Psalm then proceeds to reconcile the if and the else branch and at that point, it's bound to make -1|null incorrectly whereas -1|never would have found the correct -1 as a result

I'm not perfectly sure about the fix, especially the part where it exclude $this seems suspicious, but I think there's something correct in this

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/9432deb60a

<?php

$a = -1;
if ($a !== null) {
    /** @psalm-trace $a */;
}
else {
    /** @psalm-trace $a */;
}
/** @psalm-trace $a */;
echo $a;

Psalm output (using commit 222887a):

ERROR: RedundantCondition - 4:5 - int can never contain null

INFO: Trace - 5:27 - $a: -1

INFO: Trace - 8:27 - $a: null

INFO: Trace - 10:23 - $a: -1|null

[weirdan]

If no assignments have been made to the variable in the if/else block, then that if/else block couldn't have affected the variable type in the enclosing scope, and there's no point even trying to reconcile it. Even though assertions made in the if clause do affect the type in the if body and inverse assertions affect the type in the else branch. That's the difference between the asserted and assigned types I was talking about.

You can also say that assertions for if and else cancel each other (null & !null => never, and then reconciliation would result in the same type the variable had outside the if/else), but it doesn't seem to work all too good in practice, as not every assertion can be negated. Also, why reconcile assertions that we know should have cancelled each other?

[weirdan]

assertions for if and else cancel each other (null & !null => never)

On second thought, it should be null | !null => mixed.

[orklah]

I don't know enough of condition reconciliation then. I thought we made a union between the branches in every case.

[weirdan]

That's why we track assigned/redefined vars in IfScope. The distinction eludes me, though.

[weirdan]

Psalm indeed seems to rely on the mutual cancellation because my 'fix' resulted in over 3.5K errors reported on Psalm's own codebase.

But there's definitely also something wrong with the scope manipulation logic as well

$a = 1;
if ($a !== null) {}

evaluates a hypothetical else with a negated set of assertions introduced by if. And then it treats the types that were generated by applying those assertions as if they were generated by an assignment in that hypothetical else, resulting in the type ending up in $if_scope->possibly_redefined_variables, even though there was no such else (and no assignments) at all. It does work fine for cases when the assertions in if and (hypothetical) else had opposite effects on the original type, but it breaks badly when one of the sides had no effect, like in these cases we have in the original issue and here.

Psalm also doesn't seem to treat hypothetical and real else constructs any differently; thus, the errors are similar.

[weirdan]

I believe #9538 is a better fix for that specific issue.