Skip to content

Commit fa676d1

Browse files
peter17staabm
peter17
authored and
staabm
committed
Avoid dead catch false positive for inconsistently overridden trait methods
A trait's try/catch can be dead in the context of one class using the trait and alive in another, e.g. when it depends on whether an abstract method gets overridden without throwing. Apply the same ConstantConditionInTraitHelper mechanism already used for isset/empty/?? to CatchWithUnthrownExceptionRule, so disagreeing verdicts across classes using the trait suppress the error instead of reporting it. Closes phpstan/phpstan#10315
1 parent 8f78985 commit fa676d1

8 files changed

Lines changed: 194 additions & 51 deletions

‎src/Analyser/NodeScopeResolver.php‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2371,10 +2371,7 @@ public function processStmtNode(
23712371

23722372
// emit error
23732373
foreach ($matchingCatchTypes as $catchTypeIndex => $matched) {
2374-
if ($matched) {
2375-
continue;
2376-
}
2377-
$this->callNodeCallback($nodeCallback, new CatchWithUnthrownExceptionNode($catchNode, $catchTypes[$catchTypeIndex], $originalCatchTypes[$catchTypeIndex]), $scope, $storage);
2374+
$this->callNodeCallback($nodeCallback, new CatchWithUnthrownExceptionNode($catchNode, $catchTypes[$catchTypeIndex], $originalCatchTypes[$catchTypeIndex], $matched), $scope, $storage);
23782375
}
23792376

23802377
if (count($matchingThrowPoints) === 0) {

‎src/Node/CatchWithUnthrownExceptionNode.php‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
final class CatchWithUnthrownExceptionNode extends NodeAbstract implements VirtualNode
1414
{
1515

16-
public function __construct(private Catch_ $originalNode, private Type $caughtType, private Type $originalCaughtType)
16+
public function __construct(private Catch_ $originalNode, private Type $caughtType, private Type $originalCaughtType, private bool $matched)
1717
{
1818
parent::__construct($originalNode->getAttributes());
1919
}
@@ -33,6 +33,14 @@ public function getOriginalCaughtType(): Type
3333
return $this->originalCaughtType;
3434
}
3535

36+
/**
37+
* Whether this caught type is actually thrown in the try block, i.e. the catch is not dead.
38+
*/
39+
public function isMatched(): bool
40+
{
41+
return $this->matched;
42+
}
43+
3644
#[Override]
3745
public function getType(): string
3846
{

‎src/Rules/Comparison/ConstantConditionInTraitHelper.php‎

Lines changed: 38 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,35 @@ public function emitNoError(
4040
Scope&NodeCallbackInvoker&CollectedDataEmitter $scope,
4141
Expr $expr,
4242
): void
43+
{
44+
$this->emitNoErrorForKey($ruleName, $scope, $this->exprString($expr));
45+
}
46+
47+
/**
48+
* @param class-string<Rule<covariant Node>> $ruleName
49+
*/
50+
public function emitError(
51+
string $ruleName,
52+
Scope&NodeCallbackInvoker&CollectedDataEmitter $scope,
53+
Expr $expr,
54+
bool $value,
55+
RuleError $ruleError,
56+
): void
57+
{
58+
$this->emitErrorForKey($ruleName, $scope, $expr, $this->exprString($expr), $value, $ruleError);
59+
}
60+
61+
/**
62+
* Like emitNoError(), but for callers that cannot key their check by a single Expr
63+
* (e.g. one Rule node covering several distinct checks at the same location).
64+
*
65+
* @param class-string<Rule<covariant Node>> $ruleName
66+
*/
67+
public function emitNoErrorForKey(
68+
string $ruleName,
69+
Scope&NodeCallbackInvoker&CollectedDataEmitter $scope,
70+
string $key,
71+
): void
4372
{
4473
if (!$scope->isInTrait()) {
4574
return;
@@ -48,18 +77,22 @@ public function emitNoError(
4877
$scope->emitCollectedData(ConstantConditionInTraitCollector::class, [
4978
$ruleName,
5079
$scope->getTraitReflection()->getName(),
51-
$this->exprString($expr),
80+
$key,
5281
null,
5382
]);
5483
}
5584

5685
/**
86+
* Like emitError(), but for callers that cannot key their check by a single Expr
87+
* (e.g. one Rule node covering several distinct checks at the same location).
88+
*
5789
* @param class-string<Rule<covariant Node>> $ruleName
5890
*/
59-
public function emitError(
91+
public function emitErrorForKey(
6092
string $ruleName,
6193
Scope&NodeCallbackInvoker&CollectedDataEmitter $scope,
62-
Expr $expr,
94+
Node $node,
95+
string $key,
6396
bool $value,
6497
RuleError $ruleError,
6598
): void
@@ -75,9 +108,9 @@ public function emitError(
75108
$scope->emitCollectedData(ConstantConditionInTraitCollector::class, [
76109
$ruleName,
77110
$scope->getTraitReflection()->getName(),
78-
$this->exprString($expr),
111+
$key,
79112
$value,
80-
$this->ruleErrorTransformer->transform($ruleError, $scope, [], $expr),
113+
$this->ruleErrorTransformer->transform($ruleError, $scope, [], $node),
81114
]);
82115
}
83116

‎src/Rules/Exceptions/CatchWithUnthrownExceptionRule.php‎

Lines changed: 42 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,13 @@
33
namespace PHPStan\Rules\Exceptions;
44

55
use PhpParser\Node;
6+
use PHPStan\Analyser\CollectedDataEmitter;
7+
use PHPStan\Analyser\NodeCallbackInvoker;
68
use PHPStan\Analyser\Scope;
79
use PHPStan\DependencyInjection\AutowiredParameter;
810
use PHPStan\DependencyInjection\RegisteredRule;
911
use PHPStan\Node\CatchWithUnthrownExceptionNode;
12+
use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper;
1013
use PHPStan\Rules\Rule;
1114
use PHPStan\Rules\RuleErrorBuilder;
1215
use PHPStan\Type\NeverType;
@@ -25,6 +28,7 @@ public function __construct(
2528
private ExceptionTypeResolver $exceptionTypeResolver,
2629
#[AutowiredParameter(ref: '%exceptions.reportUncheckedExceptionDeadCatch%')]
2730
private bool $reportUncheckedExceptionDeadCatch,
31+
private ConstantConditionInTraitHelper $constantConditionInTraitHelper,
2832
)
2933
{
3034
}
@@ -34,41 +38,55 @@ public function getNodeType(): string
3438
return CatchWithUnthrownExceptionNode::class;
3539
}
3640

37-
public function processNode(Node $node, Scope $scope): array
41+
public function processNode(Node $node, Scope&NodeCallbackInvoker&CollectedDataEmitter $scope): array
3842
{
39-
if ($node->getCaughtType() instanceof NeverType) {
40-
return [
41-
RuleErrorBuilder::message(
42-
sprintf('Dead catch - %s is already caught above.', $node->getOriginalCaughtType()->describe(VerbosityLevel::typeOnly())),
43-
)
44-
->line($node->getStartLine())
45-
->identifier('catch.alreadyCaught')
46-
->build(),
47-
];
43+
// A trait's catch block can be dead in the context of one class using the trait
44+
// and alive in the context of another, e.g. when it depends on whether an
45+
// abstract method gets overridden. The key below identifies this specific catch
46+
// type occurrence so its verdicts can be compared across all classes using the trait.
47+
$key = sprintf('%s:%d', $node->getOriginalCaughtType()->describe(VerbosityLevel::typeOnly()), $node->getOriginalNode()->getStartLine());
48+
49+
if ($node->isMatched()) {
50+
$this->constantConditionInTraitHelper->emitNoErrorForKey(self::class, $scope, $key);
51+
return [];
4852
}
4953

50-
if (!$this->reportUncheckedExceptionDeadCatch) {
51-
$isCheckedException = false;
52-
foreach ($node->getCaughtType()->getObjectClassNames() as $objectClassName) {
53-
if ($this->exceptionTypeResolver->isCheckedException($objectClassName, $scope)) {
54-
$isCheckedException = true;
55-
break;
54+
if ($node->getCaughtType() instanceof NeverType) {
55+
$error = RuleErrorBuilder::message(
56+
sprintf('Dead catch - %s is already caught above.', $node->getOriginalCaughtType()->describe(VerbosityLevel::typeOnly())),
57+
)
58+
->line($node->getStartLine())
59+
->identifier('catch.alreadyCaught')
60+
->build();
61+
} else {
62+
if (!$this->reportUncheckedExceptionDeadCatch) {
63+
$isCheckedException = false;
64+
foreach ($node->getCaughtType()->getObjectClassNames() as $objectClassName) {
65+
if ($this->exceptionTypeResolver->isCheckedException($objectClassName, $scope)) {
66+
$isCheckedException = true;
67+
break;
68+
}
5669
}
57-
}
5870

59-
if (!$isCheckedException) {
60-
return [];
71+
if (!$isCheckedException) {
72+
return [];
73+
}
6174
}
62-
}
6375

64-
return [
65-
RuleErrorBuilder::message(
76+
$error = RuleErrorBuilder::message(
6677
sprintf('Dead catch - %s is never thrown in the try block.', $node->getCaughtType()->describe(VerbosityLevel::typeOnly())),
6778
)
6879
->line($node->getStartLine())
6980
->identifier('catch.neverThrown')
70-
->build(),
71-
];
81+
->build();
82+
}
83+
84+
if ($scope->isInTrait()) {
85+
$this->constantConditionInTraitHelper->emitErrorForKey(self::class, $scope, $node->getOriginalNode(), $key, true, $error);
86+
return [];
87+
}
88+
89+
return [$error];
7290
}
7391

7492
}

‎tests/PHPStan/Rules/Exceptions/AbilityToDisableImplicitThrowsTest.php‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,26 +2,37 @@
22

33
namespace PHPStan\Rules\Exceptions;
44

5+
use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper;
6+
use PHPStan\Rules\Comparison\ConstantConditionInTraitRule;
57
use PHPStan\Rules\Rule;
8+
use PHPStan\Testing\CompositeRule;
69
use PHPStan\Testing\RuleTestCase;
710
use PHPUnit\Framework\Attributes\RequiresPhp;
811
use function array_merge;
912

1013
/**
11-
* @extends RuleTestCase<CatchWithUnthrownExceptionRule>
14+
* @extends RuleTestCase<CompositeRule>
1215
*/
1316
class AbilityToDisableImplicitThrowsTest extends RuleTestCase
1417
{
1518

1619
protected function getRule(): Rule
1720
{
18-
return new CatchWithUnthrownExceptionRule(new DefaultExceptionTypeResolver(
19-
self::createReflectionProvider(),
20-
[],
21-
[],
22-
[],
23-
[],
24-
), true);
21+
// @phpstan-ignore argument.type
22+
return new CompositeRule([
23+
new CatchWithUnthrownExceptionRule(
24+
new DefaultExceptionTypeResolver(
25+
self::createReflectionProvider(),
26+
[],
27+
[],
28+
[],
29+
[],
30+
),
31+
true,
32+
self::getContainer()->getByType(ConstantConditionInTraitHelper::class),
33+
),
34+
new ConstantConditionInTraitRule(),
35+
]);
2536
}
2637

2738
public function testRule(): void
@@ -97,6 +108,11 @@ public function testBug7799(): void
97108
]);
98109
}
99110

111+
public function testBug10315(): void
112+
{
113+
$this->analyse([__DIR__ . '/data/bug-10315.php'], []);
114+
}
115+
100116
public static function getAdditionalConfigFiles(): array
101117
{
102118
return array_merge(

‎tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleStubsTest.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace PHPStan\Rules\Exceptions;
44

5+
use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper;
56
use PHPStan\Rules\Rule;
67
use PHPStan\Testing\RuleTestCase;
78

@@ -19,7 +20,7 @@ protected function getRule(): Rule
1920
[],
2021
[],
2122
[],
22-
), true);
23+
), true, self::getContainer()->getByType(ConstantConditionInTraitHelper::class));
2324
}
2425

2526
public function testRule(): void

‎tests/PHPStan/Rules/Exceptions/CatchWithUnthrownExceptionRuleTest.php‎

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,15 @@
44

55
use Error;
66
use InvalidArgumentException;
7+
use PHPStan\Rules\Comparison\ConstantConditionInTraitHelper;
8+
use PHPStan\Rules\Comparison\ConstantConditionInTraitRule;
79
use PHPStan\Rules\Rule;
10+
use PHPStan\Testing\CompositeRule;
811
use PHPStan\Testing\RuleTestCase;
912
use PHPUnit\Framework\Attributes\RequiresPhp;
1013

1114
/**
12-
* @extends RuleTestCase<CatchWithUnthrownExceptionRule>
15+
* @extends RuleTestCase<CompositeRule>
1316
*/
1417
class CatchWithUnthrownExceptionRuleTest extends RuleTestCase
1518
{
@@ -21,13 +24,21 @@ class CatchWithUnthrownExceptionRuleTest extends RuleTestCase
2124

2225
protected function getRule(): Rule
2326
{
24-
return new CatchWithUnthrownExceptionRule(new DefaultExceptionTypeResolver(
25-
self::createReflectionProvider(),
26-
[],
27-
$this->uncheckedExceptionClasses,
28-
[],
29-
[],
30-
), $this->reportUncheckedExceptionDeadCatch);
27+
// @phpstan-ignore argument.type
28+
return new CompositeRule([
29+
new CatchWithUnthrownExceptionRule(
30+
new DefaultExceptionTypeResolver(
31+
self::createReflectionProvider(),
32+
[],
33+
$this->uncheckedExceptionClasses,
34+
[],
35+
[],
36+
),
37+
$this->reportUncheckedExceptionDeadCatch,
38+
self::getContainer()->getByType(ConstantConditionInTraitHelper::class),
39+
),
40+
new ConstantConditionInTraitRule(),
41+
]);
3142
}
3243

3344
public function testRule(): void
@@ -842,4 +853,9 @@ public function testBug9826(): void
842853
$this->analyse([__DIR__ . '/data/bug-9826.php'], []);
843854
}
844855

856+
public function testBug10315(): void
857+
{
858+
$this->analyse([__DIR__ . '/data/bug-10315.php'], []);
859+
}
860+
845861
}

‎tests/PHPStan/Rules/Exceptions/data/bug-10315.php‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace Bug10315;
4+
5+
class PhpfastcacheUnsupportedMethodException extends \Exception
6+
{
7+
8+
}
9+
10+
trait DriverPoolAbstractTrait
11+
{
12+
13+
/**
14+
* @throws PhpfastcacheUnsupportedMethodException
15+
*/
16+
protected function driverReadMultiple(): array
17+
{
18+
throw new PhpfastcacheUnsupportedMethodException();
19+
}
20+
21+
}
22+
23+
trait CacheItemPoolTrait
24+
{
25+
26+
public function getItems(): array
27+
{
28+
try {
29+
return $this->driverReadMultiple();
30+
} catch (PhpfastcacheUnsupportedMethodException $e) {
31+
return [];
32+
}
33+
}
34+
35+
}
36+
37+
class Redis
38+
{
39+
40+
use DriverPoolAbstractTrait, CacheItemPoolTrait;
41+
42+
protected function driverReadMultiple(): array
43+
{
44+
return [];
45+
}
46+
47+
}
48+
49+
class Memcached
50+
{
51+
52+
use DriverPoolAbstractTrait, CacheItemPoolTrait;
53+
54+
}

0 commit comments

Comments
 (0)