Remove special handling for elseifs that breaks for else if

[muglug]

This is a weird PR, because it breaks a niche existing analysis, but I think for the right reasons.

The test twoVarLogicNotNestedWithElseifCorrectlyReinforcedInIf breaks when the elseif is changed to else if, and the "elseif" version doesn't work online either: https://psalm.dev/r/a1f9a6439e for unknown reasons.

Generally I think it's bad when elseif and else if are treated differently (maybe I erred in having separate handling for elseif). Either way, I think this is a minimally-bad change, and it limits the use of the Union::substitute method, which I don't think should be used in if/else handling — if/else handling should just use regular type reconciliation everywhere.

I saw this mismatch when looking at Hack (which has no elseif)

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/a1f9a6439e

<?php
class A {}
class B extends A {}
function foo(?A $a, ?A $b): A {
    if ($a) {
        $a = new B;
    } elseif ($b) {
        // do nothing
    } else {
        return new A;
    }
    if (!$a) return $b;
    return $a;
}

Psalm output (using commit 8c716f8):

ERROR: NullableReturnStatement - 12:21 - The declared return type 'A' for foo is not nullable, but the function returns 'A|null'

ERROR: InvalidNullableReturnType - 4:29 - The declared return type 'A' for foo is not nullable, but 'A|null' contains null

[orklah]

Feels weird to make a change that only worsen the analysis, even for the good reasons. I guess fixing it for all the cases would be hard, right?

Maybe we should leave a small comment to describe that a feature is missing there?

[muglug]

I added a comment!

The original test was added over 5 years ago, which likely means it was found in Vimeo code at some point. It's possible that Psalm's analysis will generate false-positives on other codebases due to this change, but I think that's pretty unlikely. And I think slightly-simplifying if/else analysis is worth that disruption.

[muglug]

This matches a general limitation of Psalm when dealing with nested versions of things:

Consider https://psalm.dev/r/c8a6cfe83c vs https://psalm.dev/r/9a69e87e69

Both equivalent, but Psalm's special-sauce generation of logical clauses from these sorts of if statements only works at the level they're expressed on.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/c8a6cfe83c

<?php
class A {}
function foo(?A $one, ?A $two) : A {
    if (!$one) {
        if (!$two) {
            throw new \Exception();
        }
    }

    if (!$one) {
        return $two;
    }
    
    return $one;
}

Psalm output (using commit 8c716f8):

ERROR: NullableReturnStatement - 11:16 - The declared return type 'A' for foo is not nullable, but the function returns 'A|null'

ERROR: InvalidNullableReturnType - 3:34 - The declared return type 'A' for foo is not nullable, but 'A|null' contains null

https://psalm.dev/r/9a69e87e69

<?php
class A {}
function foo(?A $one, ?A $two) : A {
    if (!$one && !$two) {
		throw new \Exception();
    }

    if (!$one) {
        return $two;
    }
    
    return $one;
}

Psalm output (using commit 8c716f8):

No issues!

[orklah]

Let's roll with it then! We'll see if we get feedback!

[orklah]

Thanks!