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.

[peter17]

Can this be merged? Thanks 😄
I see I need to rebase; I will do that in a moment. Regards

[peter17]

I rebased, but with significant changes:
2.2.x landed a large refactor that split NodeScopeResolver apart (~5800 → ~2664 lines, 17 commits: Introduce StmtHandler, Extract …Processor, the fiber removal, etc.). PR's NodeScopeResolver.php hunk no longer exists there — the catch-emission code moved to src/Analyser/StmtHandler/TryCatchHandler.php, with $this->callNodeCallback(...) becoming $nodeScopeResolver->callNodeCallback(...).

So I had to apply some of the the PR's changes to TryCatchHandler.php instead.

[SanderMuller]

Reviewed the rebase at f8c5ebcbc. It is faithful - nothing to change from my side.

• Only the emission site moved. The other ten files are byte-identical to 7d163030b, the revision I checked last week, so everything verified then carries over. The new TryCatchHandler.php block is structurally the same as the old NodeScopeResolver one: same $matched then $scope->isInTrait() gate, same node, same position before the continue, only $this-> becoming $nodeScopeResolver->.
• CatchWithUnthrownExceptionNode is still byte-identical to 2.2.x after the split, so the @api node came through the refactor untouched.
• TryCatchHandler is not #[ShadowedByTurboExtension], so the move does not pull in a native mirror obligation - worth checking given what the refactor is for.

Re-ran the behavioural checks on the new base rather than assuming the rebase was inert:

• the trait fixture still reports exactly 36 and 67, both without an (in context of class …) decoration;
• collapsing DeadCatchInTraitKey::create() to line-only still makes the line 67 error vanish, so the granularity guard survived;
• suppressing the new emission still fails AbilityToDisableImplicitThrowsTest::testBug10315 while CatchWithUnthrownExceptionRuleTest::testBug10315 stays green;
• 122 tests in tests/PHPStan/Rules/Exceptions, full suite 21146, self-analysis clean, phpcs clean on all six changed source files. (21146 against last week's 21346 is the base refactor, not this PR.)

The 11 red checks are all base fallout, none of them yours. #6246, an unrelated PR sharing this base, fails the same cluster - and its Rector job fails on exactly the same two tests (SimplifyEmptyCheckOnEmptyArrayRectorTest, RemoveUnusedPrivateMethodRectorTest). The four phpstan-doctrine … make phpstan jobs fail on Call to deprecated method toMutatingScope() of interface PHPStan\Analyser\Scope, which the refactor introduced, and Test (PHP 7.4) / Test (PHP 8.5) are Benchmark against stale committed baselines (bug-7901.php +31.95%, bug-8147.php +31.12%). The only PRs that look green on this base are the ones whose paths skip those jobs.

[SanderMuller]

Approving. The rebase is faithful and I closed the last two questions I had, both of which the test suite structurally cannot answer.

The rebase itself: ten of the eleven files are byte-identical to 7d163030b, the revision I verified before the split, so that verification carries over. The relocated block in TryCatchHandler keeps the same $matched then isInTrait() gate, the same node and the same position before the continue; CatchWithUnthrownExceptionNode is still byte-identical to 2.2.x; and TryCatchHandler is not #[ShadowedByTurboExtension], so nothing native follows. Re-ran the behavioural checks on the new base: fixture still exactly 36 and 67, the line-only key still makes the line 67 error vanish, and suppressing the emission still fails AbilityToDisableImplicitThrowsTest::testBug10315 while the other stays green.

Does the rule run outside the test harness? Your tests build CatchWithThrownExceptionInTraitRule by hand in a CompositeRule, so a green suite says nothing about the DI registration. Instrumented a real analysis of a symfony + doctrine vendor tree (3862 files): the rule is registered and invoked 43 times.

Do the verdicts survive a warm run? This is the one that worried me, because the mechanism needs verdicts from every using class in one run and RuleTestCase never touches the result cache - a warm-run-only false positive would escape the suite entirely. They are persisted: 43 collector entries in the cache file, for +7,779 bytes (+0.012%) on that corpus.

One discrepancy, resolved: the handler emits 52 nodes for 43 rule invocations, which looked like dropped verdicts. Logging the occurrence at both ends shows 20 distinct occurrences on each side and an empty difference - the extra emissions are repeats of the same catches from re-analysis passes, so no unique verdict is lost.

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

CI: the eleven reds are all base fallout, not yours. #6246, an unrelated PR on this base, fails the same cluster with the same two Rector tests; the doctrine make phpstan jobs fail on Call to deprecated method toMutatingScope(); Test (PHP 7.4) / Test (PHP 8.5) are Benchmark against stale baselines. Also worth knowing for anyone comparing corpora here: this corpus reports 17685 or 17682 errors depending on the run, with FormErrorIterator generics flipping 6/3/6 across three identical cold runs - pre-existing non-determinism, unrelated to this PR.