Rewrite TryAnalyzer

[AndrolGenhald]

As usual I bit off way more than I intended. It turns out try/catch/finally is actually really complicated...

Before reviewing I strongly recommend looking through the unionAssignmentsWithMultipleNestedTry test line by line and fully understanding it.

Fixes #5123, fixes #5700, fixes #5967, fixes #5973, fixes #6286, fixes #6342, fixes #6659, fixes #7249.

The one thing that really annoys me about this change is that the finally analysis assumes that there will never be an uncaught exception thrown in a catch:

$foo = -1;
try {
    maybeThrow();
    $foo = 1;
} catch (Exception $_) {
    maybeThrow();
    $foo = 2;
} finally {
    if ($foo < 1) {} // False positive
    takesPositiveInt($foo); // False negative
}

If anyone strongly objects to this I'll change it back, I just hate to throw away all that work :/.

I'm personally fairly ok with this since I don't think throwing in a catch with a finally after it is too common of a pattern anyway, and even when it does happen I'd be surprised if these false negatives/positives caused a problem. It also helps prevent other false positives as well:

try {
    maybeThrow();
    $foo = 1;
} catch (Throwable $_) {
    $foo = 2;
} finally {
    return $foo; // If we assume `catch`es can throw anywhere, $foo is possibly undefined
}

[AndrolGenhald]

Shepherd failure is waiting for #7684.

[AndrolGenhald]

Could someone baseline the PHPUnit cast to int errors? I almost didn't notice the extra errors that showed up there from this PR. I'll look tomorrow to see if they're anything to worry about.

[AndrolGenhald]

This is currently failing with RedundantCondition - src/somefile.php:12:30 - Type Exception for $e is never null, I'm wondering if it's related to the $vars_in_scope not being cloned issue. Maybe I'll end up fixing that here after all...

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/491bbefc48

<?php

try {
} catch (Exception $_e) {
    $e = $_e;
}

if (!isset($e)) {
}

try {
} catch (Throwable $_e) {
    $e = $e ?? $_e;
}

Psalm output (using commit d7d846e):

INFO: MixedAssignment - 13:5 - Unable to determine the type that $e is being assigned to

INFO: UnusedVariable - 13:5 - $e is never referenced or the value is not used

[AndrolGenhald]

Still waiting on #7684, otherwise I think this is good to go. The two remaining issues for PHPUnit are sort of like this, so I think they can be ignored.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/ea3ae076d5

<?php

class A {}
class B {}

$is_a = false;
$foo = new B();
if (random_int(0, 1)) {
    $is_a = true;
    $foo = new A();
}

if ($is_a) {
    takesA($foo);
}
    
function takesA(A $_): void {}

Psalm output (using commit d7d846e):

ERROR: PossiblyInvalidArgument - 14:12 - Argument 1 of takesA expects A, possibly different type A|B provided

[weirdan]

It probably makes sense to extend it from ClassIssue so that users could suppress it for individual exception classes.

[weirdan]

Is there any reason to alias this? $expr doesn't seem to be used in the loop body.

[AndrolGenhald]

Nope, that must be a leftover change from when I was figuring out how to get it working.

[weirdan]

One possible pattern that employs both finally and exception re-throw in catch is nested transactions, e.g. https://psalm.dev/r/8e926f8c41

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/8e926f8c41

<?php

class DB {
    private int $transactionLevel = 0;
    /**
     * @template T
     * @param callable():T $f
     * @return T
     */
    public function transactional(callable $f) {
        try {
            if (0 === $this->transactionLevel++) {
                $this->begin();
            }
            $ret = $f();
            if (1 === $this->transactionLevel) {
                $this->commit();
            }
        } catch (Throwable $e) {
            $this->rollBack();
            throw $e;
        } finally {
            $this->transactionLevel--;
        }
        return $ret;
    }
    
    private function begin(): void {}
    private function commit(): void {}
    private function rollBack(): void {}
}

Psalm output (using commit 997bded):

No issues!

[weirdan]

Shouldn't it be '$foo?===' => '2'? Or do we assume exception could have been thrown after the assignment?

[orklah]

I think it's assumed to consider that an exception could happen at any moment. Maybe not on a simple case like this, but if you replace $foo = 2; by $foo[] = 2; you get an error that could be caught.

[weirdan]

I mean, assignment either happened and foo is 2 or didn't (due to exception) and foo is unset, so to me it looks like it should result in a possibly unset 2 => '$foo?===' => '2'.

[AndrolGenhald]

You're right, I'll have to take a look at that. If an uncaught exception is thrown execution won't continue, if a caught exception is thrown $foo is unset, and if no exception is thrown $foo is 2. I think the issue is that right now it only considers a variable overridden by the catch if an assignment happens, it doesn't realize that the unset also lets us take only the last value from the try.

[weirdan]

I don't see how this could ever result in 1.

[AndrolGenhald]

Similar to above, returning catches should allow us to assume the try completed successfully. I'm not quite sure why this one isn't working though, I thought I'd accounted for that.

[weirdan]

it's probably just mixed

[orklah]

We could probably add a TypeCombiner::combine to collapse those

[AndrolGenhald]

🤷 that's what extract gives. I've never liked that Psalm allows combining mixed in Unions without collapsing. I think the idea was to allow better completion for the language server, but I'd rather have the language server address that separately and have a sane type system. This is something I'm changing in my type comparison rewrite.

[orklah]

the idea was to allow better completion

I think it's just a bug. Given the sheer amount of Unions created, it would probably be a horrible place to put an automatic combining there for performances, but IMHO, we should never end up with this kind of types, it's just the symptom of a wrong handling somewhere

[AndrolGenhald]

One possible pattern that employs both finally and exception re-throw in catch is nested transactions, e.g. https://psalm.dev/r/8e926f8c41

That one should be fine, it only becomes an issue if the finally assumes something happened in the catch but the catch might throw an exception before that happens.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/8e926f8c41

<?php

class DB {
    private int $transactionLevel = 0;
    /**
     * @template T
     * @param callable():T $f
     * @return T
     */
    public function transactional(callable $f) {
        try {
            if (0 === $this->transactionLevel++) {
                $this->begin();
            }
            $ret = $f();
            if (1 === $this->transactionLevel) {
                $this->commit();
            }
        } catch (Throwable $e) {
            $this->rollBack();
            throw $e;
        } finally {
            $this->transactionLevel--;
        }
        return $ret;
    }
    
    private function begin(): void {}
    private function commit(): void {}
    private function rollBack(): void {}
}

Psalm output (using commit 997bded):

No issues!

[theodorejb]

Is it still planned to finish this? It would be nice for all the related issues to be fixed (I just ran into #5700).

[AndrolGenhald]

I should have some time again next month, I'll try to rebase it and see what issues pop up, I think there was a thorny one with the if analysis since I ended up redoing a significant portion of that to fix some scope issues with possibly undefined variables.