Wrong dead catch analysis when using immediately called callback

[janedbal]

Bug report

• See snippet. Currently, we are using pepakriz/phpstan-exception-rules to track exceptions. There is this limitation which allows this snippet to work without any flaws. But how can I achieve the same result with this new phpstan feature? I understand that phpstan analyses the "caller" only, but I think there is no way to tell phpstan that some method just "adds" a new exception to those thrown in the callback it calls.
• Locking (as simply shown in snippet) might be a great real-world example - I want to prevent some callback (which throws some exception) to meet race condition, but the method itself may fail due to occupied lock key so I should handle all the exceptions.
• Also, ignoring this whole new functionality is not really feasable since it affects dead-code detection etc.
• Used latest phpstan 0.12.83

Code snippet that reproduces the problem

• Snippet: https://phpstan.org/r/0d464c81-835d-4962-91a7-36d36d3be2f1

Expected output

• No dead catch error.

[jakubvojacek]

I ran into the very same issue, subscribing

[janedbal]

@ondrejmirtes Do you have any input towards this issue? This is a blocker for us to upgrade to version >= 0.12.83, which I would love to do.

[ondrejmirtes]

PHPStan bug is rarely a blocker for upgrading to a newer version. In most cases you can easily ignore any false positive.

I thought about this and I think there are two ways to solve this:

• Dynamic throw type extension - you'll inspect the closure body in the extension and tell PHPStan the call can also throw what's inside the body.
• Inline @throws annotation similar to @var.

[janedbal]

Yeah, I can ignore all the invalid dead catches + all the "always-true" cases etc, but this affects plenty of places in our codebase (since we use this approach a lot) so we would have to adjust the ignores alongside many refactorings which is not negligible effort.

Dynamic throw seems like a best option to me. Adding inline throws everywhere would be crazy in our case.

Thx

[ondrejmirtes]

Implemented by: phpstan/phpstan-src@2bf30bf

Documentation: https://phpstan.org/developing-extensions/dynamic-throw-type-extensions

Your extension should inspect the AST of the closure argument to see if there's any throw and TypeCombinator::union() that with $methodReflection->getThrowType()

There might be an opportunity in the future to explore more precise closure analysis with annotations like @param-closure-called-immediately $callback and similar, but that day is not today.

[janedbal]

Thanks a lot!

Do you have any hint how to inspect the AST easily? Is there any way I can reuse your ThrowPoints implemented recently? Or do I really need to go through the AST deeply and check all method calls and throws?

[ondrejmirtes]

Alright, just tried implementing it, this works:

<?php

use PhpParser\Node\Expr\MethodCall;
use PHPStan\Analyser\MutatingScope;
use PHPStan\Analyser\NodeScopeResolver;
use PHPStan\Analyser\Scope;
use PHPStan\Reflection\MethodReflection;
use PHPStan\Type\Type;
use PHPStan\Type\TypeCombinator;

class ImmediatelyCalledClosureInMethodCallThrowTypeExtension implements \PHPStan\Type\DynamicMethodThrowTypeExtension
{

	private NodeScopeResolver $nodeScopeResolver;

	public function __construct(NodeScopeResolver $nodeScopeResolver)
	{
		$this->nodeScopeResolver = $nodeScopeResolver;
	}

	public function isMethodSupported(MethodReflection $methodReflection): bool
	{
		return $methodReflection->getDeclaringClass()->getName() === HelloWorld::class
			&& $methodReflection->getName() === 'lock';
	}

	public function getThrowTypeFromMethodCall(MethodReflection $methodReflection, MethodCall $methodCall, Scope $scope): ?Type
	{
		if (!isset($methodCall->args[0])) {
			return $methodReflection->getThrowType();
		}

		$arg = $methodCall->args[0]->value;
		if (!$arg instanceof \PhpParser\Node\Expr\Closure) {
			return $methodReflection->getThrowType();
		}

		assert($scope instanceof MutatingScope);

		$closureBodyResult = $this->nodeScopeResolver->processStmtNodes(
			$methodCall,
			$arg->stmts,
			$scope->enterAnonymousFunction($arg),
			static function (): void {}
		);

		$throwPoints = $closureBodyResult->getThrowPoints();
		if (count($throwPoints) === 0) {
			return $methodReflection->getThrowType();
		}

		$types = [];
		if ($methodReflection->getThrowType() !== null) {
			$types[] = $methodReflection->getThrowType();
		}

		foreach ($throwPoints as $throwPoint) {
			$types[] = $throwPoint->getType();
		}

		return TypeCombinator::union(...$types);
	}

}

[janedbal]

Well I managed to achieve it by using StatementResult returned from NodeScopeResolver::processStmtNodes the same way it was used in Doctrine extension, but it just doesn't feel right... but it works.

public function getThrowTypeFromMethodCall(
    MethodReflection $methodReflection,
    MethodCall $methodCall,
    Scope $scope
): ?Type {

    $throwTypes = $methodReflection->getThrowType() !== null
        ? [$methodReflection->getThrowType()]
        : [];
        
    ... // get deps for NodeScopeResolver call
                
    $result = $nodeScopeResolver->processStmtNodes($closure, $closure->getStmts(), $methodScope);

    foreach ($result->getThrowPoints() as $throwPoint) {
        $throwTypes[] = $throwPoint->getType();
    }

    return TypeCombinator::union(... $throwTypes);
}

Any hint for better implementation will be appreciated, thank you

[ondrejmirtes]

Just posted it, looks similar :D

[janedbal]

Great, you were faster, seems like the complicated deps for NodeScopeResolver are gone in your solution, thanks!

[janedbal]

@ondrejmirtes Are you considering also the inline throws annotation? After fixing the rest of codebase, I found few cases where it would help me not ignoring the dead catch.

    /**
     * @throws MyEntityAlreadyExistsException
     */
    private function flushChanges(): void
    {
        try {
            $this->entityManager->flush();
        } catch (UniqueConstraintViolationException $e) {
            if (Strings::endsWith($e->getMessage(), sprintf("'%s'", MyEntity::UNIQUE_INDEX_NAME))) {
                throw new MyEntityAlreadyExistsException();
            }

            throw $e;
        }
    }

Now this piece of code produces those errors I need to ignore:

Unused @throws MyEntityAlreadyExistsException annotation            
Dead catch - Doctrine\DBAL\Exception\UniqueConstraintViolationException is never thrown in the try block.

[ondrejmirtes]

@janedbal Yes, but in this case it would be better to add @throws for EntityManager::flush() in doctrine/orm or at least phpstan-doctrine. Any DBAL exception can be probably thrown from there.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.