Hi, you can achieve faster feedback loop by running vendor/bin/phing cs + vendor/bin/phing tests-fast + vendor/bin/phing phpstan locally (I do the same thing).

Anyway, I'm not sure it's possible what you want to achieve without advanced exception tracking. There's a milestone grouping similar issues: https://github.com/phpstan/phpstan/milestone/11

My thinking about implementing this is that ExpressionResult and also StatementResult (which NodeScopeResolver works with heavily) would also carry information about possible thrown exceptions. The result of $foo = 1; would be that it cannot ever throw an exception, whereas throw new Exception(); always throws an exception, and bar(); might throw an exception, and the try-catch-finally tracking would be able to leverage that information.

Also once a called function/method would have a @throws tag above it, the exception tracking would be even more precise.

Thanks, I'll do that. I was hoping to get some early feedback on this edge case:

try {
    $foo = 3;
}

$foo is now being considered 'maybe' (which is technically correct). I believe I'd need exit points at an expression level to make the distinction between $foo = 3 and $foo = exprThatMightThrow().

Something that might throw wouldn't be an exit point, just something that always throws/exits/returns. I believe we need a new field for possibly thrown exceptions in ExpressionResult and StatementResult.

Just ExpressionResult::throws(): TrinaryLogic might be enough 🤔

I would argue that in $foo = maybeThrows();, since maybeThrows() might throw and thus interrupt the flow, it might be considered an exit point. Maybe the definition/terminology needs to be altered a bit, but in terms of flow analysis it's equally valid as an explicit throw.

That's not what the exit points do. They're useful as terminating statements and detecting unreachable code.

Just ExpressionResult::throws(): TrinaryLogic might be enough 🤔

Nope, my thinking is that:

• If the called function isn't annotated with @throws, it might throw anything (the type will be Throwable).
• If the called function is annotated with @throws void, it doesn't throw anything.
• If the called function is annotated with @throws XxxException as usual, we can be sure what it throws.

If the user annotates their methods really honestly, they're going to be rewarded with precise analysis like this:

try {
    $foo = throwsInvalidArgumentException(); // scope 1
    $bar = throwsLogicException(); // scope 2
} catch (\InvalidArgumentException $e) {
    // scope 1
} catch (\LogicException $e) {
    // scope 2
}

Additionally, we can write a new rule that will show that some catch is dead because there isn't any call that might throw that kind of exception.

• If the called function is annotated with @throws XxxException as usual, we can be sure what it throws.

Well not really, RuntimeException and Error are usually not annotated (they're considered 'unchecked'), so I think only @throws void calls should ever be considered certain (and even then Error can appear almost anywhere). That's why I believe the trinary would be enough in this case.

If they have to be separate from exit points, then I think we should start by having processStmtNodes return a list of 'throw points'. In the future those could include types, and then you could do the analysis per point, but for a first version just taking the scope up to the first throw point is already a big improvement I think.

Well not really, RuntimeException and Error are usually not annotated (they're considered 'unchecked'), so I think only @throws void calls should ever be considered certain (and even then Error can appear almost anywhere).

I also thought about this, but also at the same time unchecked exceptions are rarely in a catch statement :)

In the future those could include types

I disagree. It can include them in the first iteration. The most time consuming thing on this task is to go through all the node types in processStmtNode and processExprNode and handle whether each node can throw an exception (and merge those informations in high level nodes that work with different nested nodes). You need to read getThrowType() anyway, so you can pass around the types. There's no need to lose information by downgrading from a list of types to just TrinaryLogic instance.

This task isn't that hard, it's just time-consuming. It's about passing around those possible thrown types in StatementResult and ExpressionResult, and then reimplement try-catch analysis to merge the right scopes based on the captured data.

I also thought about this, but also at the same time unchecked exceptions are rarely in a catch statement :)

Yeah that's true. In practice it will probably be the best fit. I don't think it would be a big change to change the behavior or make it configurable later anyway.

You need to read getThrowType() anyway, so you can pass around the types.

I wasn't aware of that. In that case it definitely makes sense to include the types from the start.

It's about passing around those possible thrown types in StatementResult and ExpressionResult, and then reimplement try-catch analysis to merge the right scopes based on the captured data.

As is already done for $hasYield, $alwaysTerminating, etc. right? I'll do this in a separate branch and rebase this when that is done.

As is already done for $hasYield, $alwaysTerminating, etc. right?

Yes :) I'm also for doing features in phases, but I'm not sure how to divide this one. Just passing stuff around without reading it anywhere will make it hard to verify the code works correctly...

I'll figure out something to assert them, I can base it off other tests in NodeScopeResolverTest.

Ah wait I see what you mean... only scope information is assertable, the local state of the resolver is not. 🤔

Something like StatementResultTest should be possible.

I prefer everything to be tested in NodeScopeResolverTest::testFileAsserts(). Any implementation detail should surface as a Scope state.

The simplest test that should pass is:

function (): void {
    try {
        $foo = 1;
    } catch (\Exception $e) {
        assertVariableCertainty(TrinaryLogic::createYes(), $foo);
    }

    try {
        $foo = doFoo();
    } catch (\Exception $e) {
        assertVariableCertainty(TrinaryLogic::createMaybe(), $foo);
    }
};

Once this is green, you know you're on the right path :)

Another point - you should get yourself familiar with Type::isSuperTypeOf() method. It will be useful when you have a scope that's used when InvalidArgumentException is thrown, but the catch references some supertype like Exception or Throwable. To know whether the scope is applicable for the current catch block, you need to ask $caughtType->isSuperTypeOf($thrownType)->yes().

I believe that in the first iteration assertType and assertVariableCertainty is everything you need.

Superseded by #481