Do not report dead catch with treatPhpDocTypesAsCertain: false

[VincentLanglet]

I'd like to move #4156 forward, and kinda disagree with your request change from calling getType to getNativeType inside BackedEnumFromDynamicStaticMethodThrowTypeExtension.

Because when using the exception analysis, PHPStan will still ask for a @throws tag while I know there is not because I trust my phpdoc (value-of<...>).

Also, when looking at all the existing DynamicMethodThrowTypeExtension they are using getType.

So I tried to find a solution you will agree with.

• When the throw type extension return NULL
  -> If I trust the phpdoc, I consider it doesn't throw an exception.
  -> If I don't, I check the throw type extension with the native type
  --> If I don't have an exception with the native type I can still return NULL
  --> If I have one, I had an implicit ThrowPoint.

The benefit of the implicit throwpoint is that:

• It won't report the catch as a dead catch
• It won't force people to add @throws tag on the method

which is a similar behavior of treatPhpDocTypesAsCertain: false which allow extra safety check but doesn't enforce them.

I didn't know how to correctly write test about this but the current one I wrote/updated should show the new behavior.

[VincentLanglet]

Damned, seems like an implicit throwPoint has impact on variable certainty...

But maybe that's an ok change when looking at the existing behavior
https://phpstan.org/r/14eb124e-de59-407e-b803-04468b99e564

The variable certainly will be Maybe only in the try part and only with treatPhpDocTypesAsCertain: false

I'll need your thought on this @ondrejmirtes

[VincentLanglet]

Damned, seems like an implicit throwPoint has impact on variable certainty...

But maybe that's an ok change when looking at the existing behavior phpstan.org/r/14eb124e-de59-407e-b803-04468b99e564

The variable certainly will be Maybe only in the try part and only with treatPhpDocTypesAsCertain: false

Also, this would be the behavior you're asking for BackedEnumFromDynamicStaticMethodThrowTypeExtension
Since the dead catch is not reported, the certainty of the variable in finally will be maybe.

[ondrejmirtes]

The way it'd make sense for this to be implemented is:

• With treatPhpDocTypesAsCertain: true, there's a dead catch being reported thanks to calling something that has @throws void, or @throws FooException (while there only being catch for BarException) or a throw type coming from a dynamic extension.
• By setting to treatPhpDocTypesAsCertain: false, these dead catches would no longer be reported.
• With treatPhpDocTypesAsCertain: false, some dead catches would still be reported - for example if there's an empty try {} block, or if there's a certain throw new FooException which doesn't depend on any PHPDoc being interpreted.
• With treatPhpDocTypesAsCertain: true and catch.neverThrown being reported for situations that would make the error disappear with treatPhpDocTypesAsCertain: false, there has to be a tip attached to the error informing about this (like all current errors dependent on this do).

I think all this means that explicit throw points (objects of ThrowPoint class) need to carry whether they're PHPDoc-based or not.

[ondrejmirtes]

With treatPhpDocTypesAsCertain: false, calling a @throws void function should probably create an implicit ThrowPoint.

[VincentLanglet]

The way it'd make sense for this to be implemented is:

• With treatPhpDocTypesAsCertain: true, there's a dead catch being reported thanks to calling something that has @throws void, or @throws FooException (while there only being catch for BarException) or a throw type coming from a dynamic extension.
• By setting to treatPhpDocTypesAsCertain: false, these dead catches would no longer be reported.
• With treatPhpDocTypesAsCertain: false, some dead catches would still be reported - for example if there's an empty try {} block, or if there's a certain throw new FooException which doesn't depend on any PHPDoc being interpreted.
• With treatPhpDocTypesAsCertain: true and catch.neverThrown being reported for situations that would make the error disappear with treatPhpDocTypesAsCertain: false, there has to be a tip attached to the error informing about this (like all current errors dependent on this do).

I think all this means that explicit throw points (objects of ThrowPoint class) need to carry whether they're PHPDoc-based or not.

Isn't this a separate (big) next step after this draft ?

We could

• In a first time having the same behavior between Dynamic extension, currently we're having
  https://phpstan.org/r/254167dd-11ae-4a7d-946a-1379e91a2d67
  https://phpstan.org/r/cbc3af21-48a1-4731-b915-40209936ff31

• In a second time handle @throws tag correctly with treatPhpDocTypesAsCertain: false ?

Looking at

phpstan-src/src/Analyser/MutatingScope.php

Line 6246 in 844f569

$resolvedType = $dynamicMethodReturnTypeExtension->getTypeFromMethodCall($methodReflection, $normalizedMethodCall, $this);

I feel like to have the exact same behavior, I should just change to

$throwType = $extension->getThrowTypeFromFunctionCall($functionReflection, $normalizedFuncCall, $this->treatPhpDocTypesAsCertain ? $scope : $scope->doNotTreatPhpDocTypesAsCertain());

[ondrejmirtes]

My proposition is in line of what treatPhpDocTypesAsCertain: false does in the rest of the codebase.

[VincentLanglet]

My proposition is in line of what treatPhpDocTypesAsCertain: false does in the rest of the codebase.

Maybe it's because I don't know enough this part of the codebase but this seems to me to be a too big work just to move forward the PR #4156 which seems to get unfortunately randomly blocked by a request change that every other DynamicThrowTypeExtension didn't got 😢

So I feel like I just have to add the DynamicThrowTypeExtension on my side rather than on PHPStan one

[ondrejmirtes]

I don't think it's too much complicated work. You need to look at every ThrowPoint::createExplicit() call and decide if they come from a PHPDoc or not. And slightly modify CatchWithUnthrownExceptionNode and CatchWithUnthrownExceptionRule.

Alright, changing the handling of TryCatch in NodeScopeResolver might be necessary and that's 200 lines of code that you'd need to get into but still it's not rocket (or typesystem) science:

phpstan-src/src/Analyser/NodeScopeResolver.php

Lines 1678 to 1895 in 844f569

	} elseif ($stmt instanceof TryCatch) {
	$branchScopeResult = $this->processStmtNodes($stmt, $stmt->stmts, $scope, $nodeCallback, $context);
	$branchScope = $branchScopeResult->getScope();
	$finalScope = $branchScopeResult->isAlwaysTerminating() ? null : $branchScope;
	$exitPoints = [];
	$finallyExitPoints = [];
	$alwaysTerminating = $branchScopeResult->isAlwaysTerminating();
	$hasYield = $branchScopeResult->hasYield();
	if ($stmt->finally !== null) {
	$finallyScope = $branchScope;
	} else {
	$finallyScope = null;
	}
	foreach ($branchScopeResult->getExitPoints() as $exitPoint) {
	$finallyExitPoints[] = $exitPoint;
	if ($exitPoint->getStatement() instanceof Node\Stmt\Expression && $exitPoint->getStatement()->expr instanceof Expr\Throw_) {
	continue;
	}
	if ($finallyScope !== null) {
	$finallyScope = $finallyScope->mergeWith($exitPoint->getScope());
	}
	$exitPoints[] = $exitPoint;
	}
	$throwPoints = $branchScopeResult->getThrowPoints();
	$impurePoints = $branchScopeResult->getImpurePoints();
	$throwPointsForLater = [];
	$pastCatchTypes = new NeverType();
	foreach ($stmt->catches as $catchNode) {
	$nodeCallback($catchNode, $scope);
	$originalCatchTypes = array_map(static fn (Name $name): Type => new ObjectType($name->toString()), $catchNode->types);
	$catchTypes = array_map(static fn (Type $type): Type => TypeCombinator::remove($type, $pastCatchTypes), $originalCatchTypes);
	$originalCatchType = TypeCombinator::union(...$originalCatchTypes);
	$catchType = TypeCombinator::union(...$catchTypes);
	$pastCatchTypes = TypeCombinator::union($pastCatchTypes, $originalCatchType);
	$matchingThrowPoints = [];
	$matchingCatchTypes = array_fill_keys(array_keys($originalCatchTypes), false);
	// throwable matches all
	foreach ($originalCatchTypes as $catchTypeIndex => $catchTypeItem) {
	if (!$catchTypeItem->isSuperTypeOf(new ObjectType(Throwable::class))->yes()) {
	continue;
	}
	foreach ($throwPoints as $throwPointIndex => $throwPoint) {
	$matchingThrowPoints[$throwPointIndex] = $throwPoint;
	$matchingCatchTypes[$catchTypeIndex] = true;
	}
	}
	// explicit only
	$onlyExplicitIsThrow = true;
	if (count($matchingThrowPoints) === 0) {
	foreach ($throwPoints as $throwPointIndex => $throwPoint) {
	foreach ($catchTypes as $catchTypeIndex => $catchTypeItem) {
	if ($catchTypeItem->isSuperTypeOf($throwPoint->getType())->no()) {
	continue;
	}
	$matchingCatchTypes[$catchTypeIndex] = true;
	if (!$throwPoint->isExplicit()) {
	continue;
	}
	$throwNode = $throwPoint->getNode();
	if (
	!$throwNode instanceof Expr\Throw_
	&& !($throwNode instanceof Node\Stmt\Expression && $throwNode->expr instanceof Expr\Throw_)
	) {
	$onlyExplicitIsThrow = false;
	}
	$matchingThrowPoints[$throwPointIndex] = $throwPoint;
	}
	}
	}
	// implicit only
	if (count($matchingThrowPoints) === 0 || $onlyExplicitIsThrow) {
	foreach ($throwPoints as $throwPointIndex => $throwPoint) {
	if ($throwPoint->isExplicit()) {
	continue;
	}
	foreach ($catchTypes as $catchTypeIndex => $catchTypeItem) {
	if ($catchTypeItem->isSuperTypeOf($throwPoint->getType())->no()) {
	continue;
	}
	$matchingThrowPoints[$throwPointIndex] = $throwPoint;
	}
	}
	}
	// include previously removed throw points
	if (count($matchingThrowPoints) === 0) {
	if ($originalCatchType->isSuperTypeOf(new ObjectType(Throwable::class))->yes()) {
	foreach ($branchScopeResult->getThrowPoints() as $originalThrowPoint) {
	if (!$originalThrowPoint->canContainAnyThrowable()) {
	continue;
	}
	$matchingThrowPoints[] = $originalThrowPoint;
	$matchingCatchTypes = array_fill_keys(array_keys($originalCatchTypes), true);
	}
	}
	}
	// emit error
	foreach ($matchingCatchTypes as $catchTypeIndex => $matched) {
	if ($matched) {
	continue;
	}
	$nodeCallback(new CatchWithUnthrownExceptionNode($catchNode, $catchTypes[$catchTypeIndex], $originalCatchTypes[$catchTypeIndex]), $scope);
	}
	if (count($matchingThrowPoints) === 0) {
	continue;
	}
	// recompute throw points
	$newThrowPoints = [];
	foreach ($throwPoints as $throwPoint) {
	$newThrowPoint = $throwPoint->subtractCatchType($originalCatchType);
	if ($newThrowPoint->getType() instanceof NeverType) {
	continue;
	}
	$newThrowPoints[] = $newThrowPoint;
	}
	$throwPoints = $newThrowPoints;
	$catchScope = null;
	foreach ($matchingThrowPoints as $matchingThrowPoint) {
	if ($catchScope === null) {
	$catchScope = $matchingThrowPoint->getScope();
	} else {
	$catchScope = $catchScope->mergeWith($matchingThrowPoint->getScope());
	}
	}
	$variableName = null;
	if ($catchNode->var !== null) {
	if (!is_string($catchNode->var->name)) {
	throw new ShouldNotHappenException();
	}
	$variableName = $catchNode->var->name;
	}
	$catchScopeResult = $this->processStmtNodes($catchNode, $catchNode->stmts, $catchScope->enterCatchType($catchType, $variableName), $nodeCallback, $context);
	$catchScopeForFinally = $catchScopeResult->getScope();
	$finalScope = $catchScopeResult->isAlwaysTerminating() ? $finalScope : $catchScopeResult->getScope()->mergeWith($finalScope);
	$alwaysTerminating = $alwaysTerminating && $catchScopeResult->isAlwaysTerminating();
	$hasYield = $hasYield || $catchScopeResult->hasYield();
	$catchThrowPoints = $catchScopeResult->getThrowPoints();
	$impurePoints = array_merge($impurePoints, $catchScopeResult->getImpurePoints());
	$throwPointsForLater = array_merge($throwPointsForLater, $catchThrowPoints);
	if ($finallyScope !== null) {
	$finallyScope = $finallyScope->mergeWith($catchScopeForFinally);
	}
	foreach ($catchScopeResult->getExitPoints() as $exitPoint) {
	$finallyExitPoints[] = $exitPoint;
	if ($exitPoint->getStatement() instanceof Node\Stmt\Expression && $exitPoint->getStatement()->expr instanceof Expr\Throw_) {
	continue;
	}
	if ($finallyScope !== null) {
	$finallyScope = $finallyScope->mergeWith($exitPoint->getScope());
	}
	$exitPoints[] = $exitPoint;
	}
	foreach ($catchThrowPoints as $catchThrowPoint) {
	if ($finallyScope === null) {
	continue;
	}
	$finallyScope = $finallyScope->mergeWith($catchThrowPoint->getScope());
	}
	}
	if ($finalScope === null) {
	$finalScope = $scope;
	}
	foreach ($throwPoints as $throwPoint) {
	if ($finallyScope === null) {
	continue;
	}
	$finallyScope = $finallyScope->mergeWith($throwPoint->getScope());
	}
	if ($finallyScope !== null) {
	$originalFinallyScope = $finallyScope;
	$finallyResult = $this->processStmtNodes($stmt->finally, $stmt->finally->stmts, $finallyScope, $nodeCallback, $context);
	$alwaysTerminating = $alwaysTerminating || $finallyResult->isAlwaysTerminating();
	$hasYield = $hasYield || $finallyResult->hasYield();
	$throwPointsForLater = array_merge($throwPointsForLater, $finallyResult->getThrowPoints());
	$impurePoints = array_merge($impurePoints, $finallyResult->getImpurePoints());
	$finallyScope = $finallyResult->getScope();
	$finalScope = $finallyResult->isAlwaysTerminating() ? $finalScope : $finalScope->processFinallyScope($finallyScope, $originalFinallyScope);
	if (count($finallyResult->getExitPoints()) > 0) {
	$nodeCallback(new FinallyExitPointsNode(
	$finallyResult->getExitPoints(),
	$finallyExitPoints,
	), $scope);
	}
	$exitPoints = array_merge($exitPoints, $finallyResult->getExitPoints());
	}
	return new StatementResult($finalScope, $hasYield, $alwaysTerminating, $exitPoints, array_merge($throwPoints, $throwPointsForLater), $impurePoints);
	} elseif ($stmt instanceof Unset_) {