Avoid dead catch false positive for inconsistently overridden trait methods

[peter17]

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

Edit: routing trait catches through the collector changes where they are reported. A dead catch inside a trait used to be reported once per class using the trait, decorated with (in context of class …); it is now reported once on the trait itself:

before

probe.php (in context of class P6199\OnlyUser):97:Dead catch - P6199\MyException is never thrown in the try block.

after

probe.php:97:Dead catch - P6199\MyException is never thrown in the try block.

This matches how isset/empty/?? in traits already behave and removes the duplication for a trait used by many classes, but it is user-visible: baseline entries carrying (in context of class …) for catch.neverThrown will stop matching and need regenerating. Worth a release-notes line.

[peter17]

Hi again @staabm here is a proposal to fix phpstan/phpstan#10315
I'm not sure about the benchmark failing however... Fixed!
Regards

[peter17]

@staabm any comment on this? Thanks!

[staabm]

Hey,

thanks for the PR.

I will come back to this PR when time allows. there is quite a bit of work in my queue atm

[staabm]

@SanderMuller please review

[SanderMuller]

Reviewed at d103c6dd6. The mechanism is the right one and it is wired up correctly - I traced how the suppression actually happens: the collector groups by [rule][trait][key][var_export($value)] and ConstantConditionInTraitRule skips a key with more than one value group, so null from a live catch and true from a dead one collide and cancel. Fail-first confirmed: with the four source files reverted, AbilityToDisableImplicitThrowsTest::testBug10315 fails.

Four things, one of which I would want resolved before merge.

1. CatchWithUnthrownExceptionNode is @api and its meaning changes. It now also fires for catches that are not dead (in traits), so any third-party rule registered for it starts seeing live catches and will report false positives unless it calls the new isMatched(). In-tree only CatchWithUnthrownExceptionRule listens, so the blast radius is external - but the node is explicitly @api, and its name says "unthrown". Cleanest would be a separate node for the trait bookkeeping so the existing one keeps meaning "dead catch"; failing that, it needs an UPGRADING.md entry and a line in the class docblock.

2. Trait errors lose their "in context of class" decoration. Routing through the collector means a dead catch inside a trait is now reported once on the trait instead of once per using class:

# 2.2.x
probe.php (in context of class P6199\OnlyUser):97:Dead catch - P6199\MyException is never thrown in the try block.
# this branch
probe.php:97:Dead catch - P6199\MyException is never thrown in the try block.

Consistent with how isset/empty already behave, and arguably nicer (no duplicates for a trait used by twenty classes) - but it is user-visible, and baseline entries that carry (in context of class …) for catch.neverThrown will stop matching after an upgrade. Worth a line in the description, and probably in the release notes.

3. The collector key is built from describe().

$key = sprintf('%s:%d', $node->getOriginalCaughtType()->describe(VerbosityLevel::typeOnly()), $node->getOriginalNode()->getStartLine());

describe() is for error messages, not for keys - the codebase rule is to never compare or index types by it. implode('|', $node->getOriginalCaughtType()->getObjectClassNames()) plus the line would do the same job without depending on how a type happens to print. The line-plus-type key itself is otherwise sound: same trait file for every using class, and A|B in one catch yields two distinct keys.

4. Only one of the two new tests is a regression test. CatchWithUnthrownExceptionRuleTest::testBug10315 passes with the source reverted; AbilityToDisableImplicitThrowsTest::testBug10315 is the one that fails. That is fine - the former documents the default-config behaviour - but it is worth knowing which one guards the fix.

CI, worth a look before merge. Tests with old PHPUnit (8.0, ubuntu-latest) fails on ExpressionResultTest::testIsAlwaysTerminating with data set #15 ((fn() => exit())();). I could not attribute it to anything in the diff and it does not reproduce on PHP 8.5 locally, but: 2.2.x at exactly this PR's base (8f7898505) was green on that job 17 minutes before this run, the same job is green on #6235, #6237 and #6240, and it was green on this branch's two earlier pushes on the old base. So it is either this PR or a flake, and it is not one of the jobs that are red for everyone right now (Benchmark, the two Symplify integrations, Turbo/macos).

Otherwise: full suite 21343 green, self-analysis clean, phpcs clean on the four changed files, and the branch is up to date with 2.2.x.

[peter17]

Thanks — this was a very useful review. All four addressed; 1 and 3 in code.

1. CatchWithUnthrownExceptionNode keeps meaning "dead catch". Took the separate-node route, so there is no @api change left at all: the node is back to its pre-PR form — same constructor, same three getters, no isMatched() — and it is still emitted only for catches that are actually dead. The trait bookkeeping moved to its own CatchWithThrownExceptionInTraitNode (emitted only inside traits, only for alive catches) with a small CatchWithThrownExceptionInTraitRule that records the alive verdict under CatchWithUnthrownExceptionRule::class, so both verdicts still land in the same collector bucket. Third-party rules registered for the @api node see exactly what they saw before, and no UPGRADING.md entry is needed.

3. No more describe() in the key. Extracted to DeadCatchInTraitKey::create(), shared by both rules, keyed on implode('|', $originalCaughtType->getObjectClassNames()) plus the line as you suggested.

2 + 3, now pinned by a test. There was no coverage of a dead catch in a trait at all, so both the new reporting shape and the key granularity could change unnoticed. Added data/dead-catch-in-trait.php, which reports exactly two errors:

• line 36 — AlphaException, dead in both FirstUser and SecondUser: reported once, on the trait, no (in context of class …) and no per-class duplication. That is the behaviour change from your point 2, now written down in a test.
• line 67 — BetaException from catch (AlphaException | BetaException $e), dead in both using classes; AlphaException on that same line is dead in ThrowsNeither but alive in ThrowsAlphaOnly, so it is correctly suppressed while BetaException is still reported.

The second case discriminates the key granularity: collapsing DeadCatchInTraitKey to line-only makes the line 67 error disappear, so a future regression there fails the suite instead of going quiet.

4. Which test guards the fix. Confirmed, and it still holds after the restructure: suppressing the new node's emission in NodeScopeResolver makes AbilityToDisableImplicitThrowsTest::testBug10315 fail with the original false positive (30: Dead catch - Bug10315\PhpfastcacheUnsupportedMethodException is never thrown in the try block.), while CatchWithUnthrownExceptionRuleTest::testBug10315 keeps passing — implicit throws keep the catch alive there, so it documents the default config rather than guarding the fix.

CI. I could not attribute the old-PHPUnit failure to the diff either, and after this restructure the analyser-side change is a strictly additive node emission behind if ($matched && $scope->isInTrait()) — nothing on the path to (fn() => exit())();. Locally ExpressionResultTest is green on three runs with different random seeds, and the full suite is green. Worth noting that job is PHP 8.0 with downgraded code and PHPUnit 9.6, and phpunit.xml has executionOrder="random", so an ordering flake is plausible. This push re-runs it, which should settle it.

Verification after the changes: 122 tests in tests/PHPStan/Rules/Exceptions green, full suite 21346 green, self-analysis via build/phpstan.neon clean, parallel-lint clean.

I updated the PR description above.

[SanderMuller]

Checked 7d163030b - all four addressed, and the two code changes are better than what I suggested. From my side this is ready.

• @api node: src/Node/CatchWithUnthrownExceptionNode.php is now byte-identical to 2.2.x (empty diff), and the new node is emitted only under $matched && $scope->isInTrait(), so the existing node keeps meaning exactly "dead catch". Nothing left to document in UPGRADING.md.
• Key: no describe(), and the granularity claim holds - collapsing DeadCatchInTraitKey::create() to line-only makes the line 67 error vanish and the test fails with exactly that diff, so a future regression there cannot go quiet.
• New fixture: blanking its expectations shows it reports exactly 36 and 67 and nothing else, both without an (in context of class …) decoration - so the reporting-shape change is pinned rather than just described.
• Fail-first: with the new node's emission suppressed, AbilityToDisableImplicitThrowsTest::testBug10315 fails while CatchWithUnthrownExceptionRuleTest::testBug10315 stays green, as you said.
• One thing I checked that is easy to get wrong: both rules are #[RegisteredRule(level: 4)], so there is no level at which the dead verdicts are collected without the alive ones.
• CI: Tests with old PHPUnit (8.0) is green on this push, so a flake it was. The four remaining reds are the ones red for everyone today - the two Symplify integrations, Turbo/macos make phpstan, and Benchmark.

Gates on my side: 122 tests in tests/PHPStan/Rules/Exceptions, full suite 21346, self-analysis clean, phpcs clean on all six changed source files.

Non-blocking note for later: in the dead path the unchecked-exception early return happens before the trait bookkeeping, so that case emits no verdict at all while the alive path always emits one. Harmless today - a lone "no error" group reports nothing, which matches current behaviour - but if the emit ever moves above that return, the two paths would start disagreeing about a catch neither of them reports.