From 7a81d518aff76c94e5f8b3f09a67dad18be9cf0a Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Sun, 6 Sep 2026 07:59:45 +0200 Subject: [PATCH 1/7] test(#26): pin same-callee repeat and exclusive-branch dispatch ahead of fan-out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two existing tests happened to forward one parameter to two distinct callees on one execution path — the exact shape #26's fan-out rule will turn into a terminal. Left as-is they would silently stop pinning what they were written for once that rule lands. - same-callee-two-callsites: Divergent::forwardTwice now calls Terminal::first twice (same callee, two call sites) instead of two different Terminal methods, matching what the fixture's name and the #17 commit that introduced it actually describe. - ChainBuilderTest::testBranchToTwoCalleesYieldsTwoFindings is renamed to testForwardsInExclusiveBranchesYieldOneFindingPerBranch and now puts the two forwards in mutually exclusive if/else arms, so it keeps pinning "two findings from one origin" via branches that can't both execute, rather than via unconditional fan-out. --- tests/Chain/ChainBuilderTest.php | 5 +++-- .../same-callee-two-callsites/expected-findings.json | 4 ++-- tests/fixtures/same-callee-two-callsites/src/Demo.php | 7 +------ 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/tests/Chain/ChainBuilderTest.php b/tests/Chain/ChainBuilderTest.php index 02e05a4..b1ec79d 100644 --- a/tests/Chain/ChainBuilderTest.php +++ b/tests/Chain/ChainBuilderTest.php @@ -76,10 +76,11 @@ public function testUseAtSecondHopEndsChain(): void self::assertSame(TerminalKind::Used, $findings[0]->terminalKind); } - public function testBranchToTwoCalleesYieldsTwoFindings(): void + public function testForwardsInExclusiveBranchesYieldOneFindingPerBranch(): void { $code = 'left($p); (new C())->right($p); } } ' + . 'class A { public function go(Cfg $p): void { ' + . 'if (random_int(0, 1) === 1) { (new B())->left($p); } else { (new C())->right($p); } } } ' . 'class B { public function left(Cfg $p): void { $p->x(); } } ' . 'class C { public function right(Cfg $p): void { $p->y(); } }'; diff --git a/tests/fixtures/same-callee-two-callsites/expected-findings.json b/tests/fixtures/same-callee-two-callsites/expected-findings.json index d51a917..5eb6530 100644 --- a/tests/fixtures/same-callee-two-callsites/expected-findings.json +++ b/tests/fixtures/same-callee-two-callsites/expected-findings.json @@ -23,7 +23,7 @@ "param": "config", "severity": "error", "origin": "Demo\\Origin::run", - "terminal": "Demo\\Terminal::second", + "terminal": "Demo\\Terminal::first", "terminalKind": "used", "hops": 3, "classes": 4, @@ -31,7 +31,7 @@ {"method": "Demo\\Origin::run", "role": "origin", "file": "src/Demo.php", "line": 11, "forwardLine": 13}, {"method": "Demo\\Middle::relay", "role": "hop", "file": "src/Demo.php", "line": 19, "forwardLine": 21}, {"method": "Demo\\Divergent::forwardTwice", "role": "hop", "file": "src/Demo.php", "line": 27, "forwardLine": 30}, - {"method": "Demo\\Terminal::second", "role": "terminal", "file": "src/Demo.php", "line": 41, "forwardLine": null} + {"method": "Demo\\Terminal::first", "role": "terminal", "file": "src/Demo.php", "line": 36, "forwardLine": null} ], "notes": [] } diff --git a/tests/fixtures/same-callee-two-callsites/src/Demo.php b/tests/fixtures/same-callee-two-callsites/src/Demo.php index bb8d477..43f8b9b 100644 --- a/tests/fixtures/same-callee-two-callsites/src/Demo.php +++ b/tests/fixtures/same-callee-two-callsites/src/Demo.php @@ -27,7 +27,7 @@ class Divergent public function forwardTwice(Cfg $config): void { Terminal::first($config); - Terminal::second($config); + Terminal::first($config); } } @@ -37,9 +37,4 @@ public static function first(Cfg $config): void { $config->go(); } - - public static function second(Cfg $config): void - { - $config->go(); - } } From 6e2c62b010cb37ae77815169cea266fb589abde8 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Sun, 6 Sep 2026 08:12:51 +0200 Subject: [PATCH 2/7] feat(#26): classify fan-out to distinct callees as its own parameter fate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A method that forwards a parameter to two or more distinct callees on one execution path does not pass the value on — it combines what those callees return. That is consumption, not a hop, so it gets its own ParamFate::FanOut instead of being counted as another pass-through link in a chain. Forwards that reach distinct callees only in mutually exclusive branch arms stay PureForward: a dispatcher picks one callee per run and really does hand the value on untouched. BranchArmPath records, per forward, which arm of each enclosing if/elseif/else, match, switch and ternary it sits in; two forwards are exclusive when some branch names them in different arms. A branch condition is not an arm, and `??`, loops and statements after a dispatcher join every arm, so those all fan out. This amends frozen rule 1 (every occurrence a whole-argument forward => PureForward): whole-argument forwards to distinct callees on one path are now a terminal use. Classifier only — the chain traversal already treats any non-PureForward fate as a terminal, so a fan-out parameter renders as `used` until its own terminal kind and the documentation follow. --- src/Index/BranchArmPath.php | 110 +++++++++++++++++++++ src/Index/CalleeRef.php | 7 ++ src/Index/FanOutDetector.php | 51 ++++++++++ src/Index/ForwardCollector.php | 14 ++- src/Index/ParamFate.php | 5 + src/Index/UsageClassifier.php | 6 +- tests/Index/BranchArmPathTest.php | 49 ++++++++++ tests/Index/UsageClassifierTest.php | 146 +++++++++++++++++++++++++++- 8 files changed, 384 insertions(+), 4 deletions(-) create mode 100644 src/Index/BranchArmPath.php create mode 100644 src/Index/FanOutDetector.php create mode 100644 tests/Index/BranchArmPathTest.php diff --git a/src/Index/BranchArmPath.php b/src/Index/BranchArmPath.php new file mode 100644 index 0000000..92d9b08 --- /dev/null +++ b/src/Index/BranchArmPath.php @@ -0,0 +1,110 @@ + $armByBranch arm label keyed by branching node object id */ + private function __construct(private readonly array $armByBranch) + { + } + + /** + * Requires the `parent` attribute to be set on the node and its ancestors. + * + * Object ids are safe as keys here because the whole method body stays + * alive for as long as any path derived from it is compared, so no id can + * be recycled onto a different node in between. + */ + public static function of(Node $node): self + { + $armByBranch = []; + $child = $node; + + while (($parent = $child->getAttribute('parent')) instanceof Node) { + $arm = self::armOf($parent, $child); + if ($arm !== null) { + $armByBranch[spl_object_id($parent)] = $arm; + } + $child = $parent; + } + + return new self($armByBranch); + } + + public function isExclusiveWith(self $other): bool + { + foreach ($this->armByBranch as $branch => $arm) { + if (isset($other->armByBranch[$branch]) && $other->armByBranch[$branch] !== $arm) { + return true; + } + } + + return false; + } + + private static function armOf(Node $parent, Node $child): ?string + { + return match (true) { + $parent instanceof If_ => self::ifArm($parent, $child), + $parent instanceof Match_ => self::indexedArm($child, $parent->arms), + $parent instanceof Switch_ => self::indexedArm($child, $parent->cases), + $parent instanceof Ternary => self::ternaryArm($parent, $child), + default => null, + }; + } + + private static function ifArm(If_ $branch, Node $child): ?string + { + if (in_array($child, $branch->stmts, true)) { + return 'then'; + } + if ($child instanceof Else_) { + return 'else'; + } + + $elseif = self::indexedArm($child, $branch->elseifs); + + return $elseif === null ? null : 'elseif:' . $elseif; + } + + private static function ternaryArm(Ternary $branch, Node $child): ?string + { + if ($child === $branch->if) { + return 'then'; + } + + return $child === $branch->else ? 'else' : null; + } + + /** + * @param array $arms + * + * @return string|null the arm's index, or null when the child is the + * branch's subject rather than one of its arms + */ + private static function indexedArm(Node $child, array $arms): ?string + { + foreach ($arms as $index => $arm) { + if ($arm === $child) { + return (string) $index; + } + } + + return null; + } +} diff --git a/src/Index/CalleeRef.php b/src/Index/CalleeRef.php index 3840cc1..eba45e3 100644 --- a/src/Index/CalleeRef.php +++ b/src/Index/CalleeRef.php @@ -22,4 +22,11 @@ public function __construct( public readonly ?string $receiverHint = null, ) { } + + public function isSameAs(self $other): bool + { + return $this->kind === $other->kind + && $this->name === $other->name + && $this->receiverHint === $other->receiverHint; + } } diff --git a/src/Index/FanOutDetector.php b/src/Index/FanOutDetector.php new file mode 100644 index 0000000..cc65108 --- /dev/null +++ b/src/Index/FanOutDetector.php @@ -0,0 +1,51 @@ + */ + private array $forwards = []; + + public function record(CalleeRef $callee, BranchArmPath $path): void + { + $this->forwards[] = ['callee' => $callee, 'path' => $path]; + } + + /** + * Compares every ordered pair, self-pairs included: a forward is never + * distinct from itself, so those answer false on their own. There are only + * ever a handful of forwards per parameter. + */ + public function fansOut(): bool + { + foreach ($this->forwards as $forward) { + foreach ($this->forwards as $other) { + if ($this->combineDistinctCallees($forward, $other)) { + return true; + } + } + } + + return false; + } + + /** + * @param array{callee: CalleeRef, path: BranchArmPath} $one + * @param array{callee: CalleeRef, path: BranchArmPath} $other + */ + private function combineDistinctCallees(array $one, array $other): bool + { + return ! $one['callee']->isSameAs($other['callee']) + && ! $one['path']->isExclusiveWith($other['path']); + } +} diff --git a/src/Index/ForwardCollector.php b/src/Index/ForwardCollector.php index 2858467..a5846ee 100644 --- a/src/Index/ForwardCollector.php +++ b/src/Index/ForwardCollector.php @@ -31,7 +31,9 @@ * either a pure whole-argument forward or a real use. Subtrees that open a new * scope (closures, arrow functions, nested classes/functions) are not * descended into; instead the frozen closure/arrow-capture rules are applied - * at the boundary. Requires the `parent` attribute to be set on visited nodes. + * at the boundary. Alongside the forwards it detects whether they fan out to + * distinct callees on one execution path. Requires the `parent` attribute to be + * set on visited nodes. */ final class ForwardCollector extends NodeVisitorAbstract { @@ -44,10 +46,19 @@ final class ForwardCollector extends NodeVisitorAbstract private bool $sawNonStore = false; + private readonly FanOutDetector $fanOutDetector; + public function __construct( private readonly string $name, private readonly bool $variadic, ) { + $this->fanOutDetector = new FanOutDetector(); + } + + /** True when the forwards reach two or more distinct callees on one path. */ + public function fansOut(): bool + { + return $this->fanOutDetector->fansOut(); } /** True when every occurrence of the parameter is a `$this->prop = $p` store. */ @@ -112,6 +123,7 @@ private function handleVariable(Variable $node): void $this->forwards[] = $forward; $this->sawNonStore = true; + $this->fanOutDetector->record($forward->callee, BranchArmPath::of($node)); } private function markUse(bool $isStore): void diff --git a/src/Index/ParamFate.php b/src/Index/ParamFate.php index fb81948..2666ee9 100644 --- a/src/Index/ParamFate.php +++ b/src/Index/ParamFate.php @@ -8,6 +8,10 @@ * How a single parameter is treated inside the body of one method. * * - PureForward: every occurrence is a whole-argument forward to another call. + * - FanOut: every occurrence is a whole-argument forward, but to two or more + * distinct callees on one execution path — the method reads the value by + * combining what the callees return, so it is a terminal use, not a hop + * (issue #26). * - Used: at least one occurrence is a real use (read, write, return, ...). * - ByRefTerminated: declared `&$p`; the method may write to it, so never a hop. * - Unused: the parameter is never mentioned in the body. @@ -15,6 +19,7 @@ enum ParamFate { case PureForward; + case FanOut; case Used; case ByRefTerminated; case Unused; diff --git a/src/Index/UsageClassifier.php b/src/Index/UsageClassifier.php index e7d1b06..3205eab 100644 --- a/src/Index/UsageClassifier.php +++ b/src/Index/UsageClassifier.php @@ -14,7 +14,7 @@ use PhpParser\NodeVisitor\ParentConnectingVisitor; /** - * Classifies each parameter of one method as PureForward, Used, + * Classifies each parameter of one method as PureForward, FanOut, Used, * ByRefTerminated, or Unused, following the frozen core semantics. The method * body must already have been name-resolved by the Indexer's main traversal. */ @@ -87,6 +87,10 @@ private function analyzeBody(string $name, bool $variadic, array $stmts): array return [ParamFate::Unused, [], false]; } + if ($collector->fansOut()) { + return [ParamFate::FanOut, $collector->forwards, false]; + } + return [ParamFate::PureForward, $collector->forwards, false]; } diff --git a/tests/Index/BranchArmPathTest.php b/tests/Index/BranchArmPathTest.php new file mode 100644 index 0000000..f11e15e --- /dev/null +++ b/tests/Index/BranchArmPathTest.php @@ -0,0 +1,49 @@ +armPathsOfIfElse(); + + self::assertTrue($thenArm->isExclusiveWith($elseArm)); + self::assertTrue($elseArm->isExclusiveWith($thenArm)); + } + + /** @return array{BranchArmPath, BranchArmPath} paths of `$p` in the then and else arm */ + private function armPathsOfIfElse(): array + { + $parser = (new ParserFactory())->createForNewestSupportedVersion(); + $statements = $parser->parse('addVisitor(new ParentConnectingVisitor()); + $statements = $traverser->traverse($statements); + + $occurrences = (new NodeFinder())->find( + $statements, + static fn (Node $node): bool => $node instanceof Variable && $node->name === 'p', + ); + self::assertCount(2, $occurrences); + + return [BranchArmPath::of($occurrences[0]), BranchArmPath::of($occurrences[1])]; + } +} diff --git a/tests/Index/UsageClassifierTest.php b/tests/Index/UsageClassifierTest.php index 773750d..62d37f7 100644 --- a/tests/Index/UsageClassifierTest.php +++ b/tests/Index/UsageClassifierTest.php @@ -106,9 +106,9 @@ public function testPositionalForwardRecordsPosition(): void self::assertSame(1, $p->forwards[0]->argKey); } - public function testTwoForwardsRecordTwoSites(): void + public function testTwoForwardsToTheSameCalleeRecordTwoSites(): void { - $p = $this->classify('a($p); b($p);')['p']; + $p = $this->classify('a($p); a($p);')['p']; self::assertSame(ParamFate::PureForward, $p->fate); self::assertCount(2, $p->forwards); } @@ -245,6 +245,11 @@ public function testPureForwardIsNotStoredOnly(): void self::assertFalse($this->classify('other($p);')['p']->storedOnly); } + public function testFanOutIsNotStoredOnly(): void + { + self::assertFalse($this->classify('a($p); b($p);')['p']->storedOnly); + } + public function testPlainForwardingUseIsNotStoredOnly(): void { self::assertFalse($this->classify('log($p);')['p']->storedOnly); @@ -319,4 +324,141 @@ public function testPromotedConstructorPropertyIsUsed(): void unlink($file); } } + + public function testTwoDistinctFunctionCalleesAreFanOut(): void + { + $p = $this->classify('a($p); b($p);')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + + public function testTwoDistinctMethodsOnThisAreFanOut(): void + { + $p = $this->classify('$this->a($p); $this->b($p);')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + + public function testSameMethodOnTwoReceiversIsFanOut(): void + { + $p = $this->classify('$x->save($p); $y->save($p);', 'Cfg $p, Repo $x, Repo $y')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + + public function testTwoDistinctInstantiationsAreFanOut(): void + { + $p = $this->classify('new A($p); new B($p);')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + + public function testConditionalForwardBesideUnconditionalOneIsFanOut(): void + { + $p = $this->classify('if ($x) { a($p); } b($p);')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + + public function testForwardInBranchConditionIsFanOutWithForwardInItsBody(): void + { + // The condition is not an arm: it runs before whichever arm is taken. + $p = $this->classify('if (a($p)) { b($p); }')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + + public function testTwoDistinctCalleesInsideOneArmAreFanOut(): void + { + $p = $this->classify('if ($x) { a($p); b($p); }')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + + public function testForwardAfterADispatcherIsFanOutWithEveryArm(): void + { + $p = $this->classify('match ($x) { 1 => a($p), default => b($p) }; c($p);')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(3, $p->forwards); + } + + public function testCoalesceOperandsAreFanOut(): void + { + // `??` short-circuits but does not branch: both operands are one path. + $p = $this->classify('$r = a($p) ?? b($p);')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + + public function testForwardInsideLoopBesideForwardAfterItIsFanOut(): void + { + $p = $this->classify('foreach ($xs as $x) { a($p); } b($p);')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + + public function testVariadicSpreadToDistinctCalleesIsFanOut(): void + { + $p = $this->classify('a(...$p); b(...$p);', 'Cfg ...$p')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + + public function testSameCalleeWithOtherArgumentsDifferingIsPureForward(): void + { + self::assertSame(ParamFate::PureForward, $this->classify('a($p, 1); a($p, 2);')['p']->fate); + } + + public function testForwardsInIfAndElseArmsArePureForward(): void + { + self::assertSame( + ParamFate::PureForward, + $this->classify('if ($x) { a($p); } else { b($p); }')['p']->fate, + ); + } + + public function testForwardsInIfElseifAndElseArmsArePureForward(): void + { + self::assertSame( + ParamFate::PureForward, + $this->classify('if ($x) { a($p); } elseif ($y) { b($p); } else { c($p); }')['p']->fate, + ); + } + + public function testForwardsInTwoDifferentElseifArmsArePureForward(): void + { + self::assertSame( + ParamFate::PureForward, + $this->classify('if ($x) { a($p); } elseif ($y) { b($p); } elseif ($z) { c($p); }')['p']->fate, + ); + } + + public function testForwardsInMatchArmsArePureForward(): void + { + self::assertSame( + ParamFate::PureForward, + $this->classify('match ($x) { 1 => a($p), default => b($p) };')['p']->fate, + ); + } + + public function testForwardsInSwitchCasesArePureForward(): void + { + self::assertSame( + ParamFate::PureForward, + $this->classify('switch ($x) { case 1: a($p); break; default: b($p); }')['p']->fate, + ); + } + + public function testForwardsInTernaryArmsArePureForward(): void + { + self::assertSame(ParamFate::PureForward, $this->classify('$r = $x ? a($p) : b($p);')['p']->fate); + } + + public function testForwardsInNestedExclusiveArmsArePureForward(): void + { + self::assertSame( + ParamFate::PureForward, + $this->classify('if ($x) { if ($y) { a($p); } else { b($p); } } else { c($p); }')['p']->fate, + ); + } } From a227b1dd73b450630257255001d0c3c4ca24115c Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Sun, 6 Sep 2026 08:26:20 +0200 Subject: [PATCH 3/7] fix(#26): branch conditions share the path with every arm; bump index cache format MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BranchArmPath labelled the whole ElseIf_, MatchArm and Case_ node as an arm of its enclosing branch, so a forward inside `elseif (check($p))`, a match arm's condition or a `case a($p):` expression looked exclusive with the other arms and the parameter was misclassified as a pure forward. A condition is evaluated on the way into the arms that follow it, so it can never be exclusive with any of them — exactly as the `if`, `match`, `switch` and ternary conditions were already treated. Arm labels are now recorded at the arm node, keyed by the enclosing branching node, and only for the arm's body; a condition records nothing and therefore shares the path with every arm. That is also the conservative direction: a shared path can only yield fewer findings. FileIndexCache::FORMAT goes 2 -> 3 because an entry written by an earlier 0.2.0-dev build reports a fan-out parameter as PureForward, and the cache promises drift is a miss, never a stale result. Also: correct the false rationale in BranchArmPathTest (the detector compares every ordered pair, so the test stands on its own as a symmetry pin), rename FanOutDetector::combineDistinctCallees to the predicate name areDistinctOnOnePath, and replace its thrice-repeated array shape with the ForwardOccurrence value object. --- src/Cache/FileIndexCache.php | 6 +- src/Index/BranchArmPath.php | 130 +++++++++++++++++++++++----- src/Index/FanOutDetector.php | 16 ++-- src/Index/ForwardOccurrence.php | 18 ++++ tests/Index/BranchArmPathTest.php | 6 +- tests/Index/UsageClassifierTest.php | 24 +++++ 6 files changed, 166 insertions(+), 34 deletions(-) create mode 100644 src/Index/ForwardOccurrence.php diff --git a/src/Cache/FileIndexCache.php b/src/Cache/FileIndexCache.php index a3730fd..48ca7a7 100644 --- a/src/Cache/FileIndexCache.php +++ b/src/Cache/FileIndexCache.php @@ -29,10 +29,12 @@ final class FileIndexCache { /** - * Bump when {@see FileIndex} or any nested value object changes shape; a + * Bump when {@see FileIndex} or any nested value object changes shape, and + * whenever the classifier would give the same source a different answer — + * an entry is only ever allowed to be a miss, never a stale result. A * cached entry written under an older format is silently ignored. */ - private const FORMAT = 2; + private const FORMAT = 3; /** Directory permission mode for lazily-created cache directories. */ private const DIRECTORY_MODE = 0o755; diff --git a/src/Index/BranchArmPath.php b/src/Index/BranchArmPath.php index 92d9b08..a903a72 100644 --- a/src/Index/BranchArmPath.php +++ b/src/Index/BranchArmPath.php @@ -7,7 +7,10 @@ use PhpParser\Node; use PhpParser\Node\Expr\Match_; use PhpParser\Node\Expr\Ternary; +use PhpParser\Node\MatchArm; +use PhpParser\Node\Stmt\Case_; use PhpParser\Node\Stmt\Else_; +use PhpParser\Node\Stmt\ElseIf_; use PhpParser\Node\Stmt\If_; use PhpParser\Node\Stmt\Switch_; @@ -26,6 +29,14 @@ private function __construct(private readonly array $armByBranch) /** * Requires the `parent` attribute to be set on the node and its ancestors. * + * Only an arm's *body* counts as an arm. A branch condition — `if (…)`, + * `elseif (…)`, a `match` or `switch` subject, a match arm's or case's + * condition, a ternary condition — is evaluated on the way into the arms + * that follow it, so it can never be exclusive with any of them. Recording + * nothing for a condition lets it share the path with every arm, which is + * also the conservative direction: a shared path can only yield fewer + * findings. + * * Object ids are safe as keys here because the whole method body stays * alive for as long as any path derived from it is compared, so no id can * be recycled onto a different node in between. @@ -38,7 +49,8 @@ public static function of(Node $node): self while (($parent = $child->getAttribute('parent')) instanceof Node) { $arm = self::armOf($parent, $child); if ($arm !== null) { - $armByBranch[spl_object_id($parent)] = $arm; + [$branch, $label] = $arm; + $armByBranch[spl_object_id($branch)] = $label; } $child = $parent; } @@ -57,50 +69,128 @@ public function isExclusiveWith(self $other): bool return false; } - private static function armOf(Node $parent, Node $child): ?string + /** + * @return array{Node, string}|null the branching node the label belongs to + * and the label, or null when the child + * does not sit in an arm of the parent + */ + private static function armOf(Node $parent, Node $child): ?array { return match (true) { - $parent instanceof If_ => self::ifArm($parent, $child), - $parent instanceof Match_ => self::indexedArm($child, $parent->arms), - $parent instanceof Switch_ => self::indexedArm($child, $parent->cases), + $parent instanceof If_ => self::thenArm($parent, $child), + $parent instanceof ElseIf_ => self::elseifArm($parent, $child), + $parent instanceof Else_ => self::elseArm($parent, $child), + $parent instanceof MatchArm => self::matchArm($parent, $child), + $parent instanceof Case_ => self::caseArm($parent, $child), $parent instanceof Ternary => self::ternaryArm($parent, $child), default => null, }; } - private static function ifArm(If_ $branch, Node $child): ?string + /** @return array{Node, string}|null */ + private static function thenArm(If_ $branch, Node $child): ?array + { + return in_array($child, $branch->stmts, true) ? [$branch, 'then'] : null; + } + + /** @return array{Node, string}|null */ + private static function elseifArm(ElseIf_ $arm, Node $child): ?array { - if (in_array($child, $branch->stmts, true)) { - return 'then'; + if (! in_array($child, $arm->stmts, true)) { + return null; } - if ($child instanceof Else_) { - return 'else'; + + $branch = self::enclosingBranch($arm, If_::class); + if ($branch === null) { + return null; } - $elseif = self::indexedArm($child, $branch->elseifs); + $index = self::indexOf($arm, $branch->elseifs); - return $elseif === null ? null : 'elseif:' . $elseif; + return $index === null ? null : [$branch, 'elseif:' . $index]; } - private static function ternaryArm(Ternary $branch, Node $child): ?string + /** @return array{Node, string}|null */ + private static function elseArm(Else_ $arm, Node $child): ?array + { + if (! in_array($child, $arm->stmts, true)) { + return null; + } + + $branch = self::enclosingBranch($arm, If_::class); + + return $branch === null ? null : [$branch, 'else']; + } + + /** @return array{Node, string}|null */ + private static function matchArm(MatchArm $arm, Node $child): ?array + { + if ($child !== $arm->body) { + return null; + } + + $branch = self::enclosingBranch($arm, Match_::class); + if ($branch === null) { + return null; + } + + $index = self::indexOf($arm, $branch->arms); + + return $index === null ? null : [$branch, $index]; + } + + /** @return array{Node, string}|null */ + private static function caseArm(Case_ $arm, Node $child): ?array + { + if (! in_array($child, $arm->stmts, true)) { + return null; + } + + $branch = self::enclosingBranch($arm, Switch_::class); + if ($branch === null) { + return null; + } + + $index = self::indexOf($arm, $branch->cases); + + return $index === null ? null : [$branch, $index]; + } + + /** @return array{Node, string}|null */ + private static function ternaryArm(Ternary $branch, Node $child): ?array { if ($child === $branch->if) { - return 'then'; + return [$branch, 'then']; } - return $child === $branch->else ? 'else' : null; + return $child === $branch->else ? [$branch, 'else'] : null; + } + + /** + * @template TBranch of Node + * + * @param class-string $branchType + * + * @return TBranch|null the branching node the arm belongs to, or null when + * the arm is not attached to one + */ + private static function enclosingBranch(Node $arm, string $branchType): ?Node + { + $branch = $arm->getAttribute('parent'); + + return $branch instanceof $branchType ? $branch : null; } /** * @param array $arms * - * @return string|null the arm's index, or null when the child is the - * branch's subject rather than one of its arms + * @return string|null the arm's position among its siblings, or null when + * it is not one of them */ - private static function indexedArm(Node $child, array $arms): ?string + private static function indexOf(Node $arm, array $arms): ?string { - foreach ($arms as $index => $arm) { - if ($arm === $child) { + foreach ($arms as $index => $sibling) { + if ($sibling === $arm) { return (string) $index; } } diff --git a/src/Index/FanOutDetector.php b/src/Index/FanOutDetector.php index cc65108..cb0181f 100644 --- a/src/Index/FanOutDetector.php +++ b/src/Index/FanOutDetector.php @@ -13,12 +13,12 @@ */ final class FanOutDetector { - /** @var list */ + /** @var list */ private array $forwards = []; public function record(CalleeRef $callee, BranchArmPath $path): void { - $this->forwards[] = ['callee' => $callee, 'path' => $path]; + $this->forwards[] = new ForwardOccurrence($callee, $path); } /** @@ -30,7 +30,7 @@ public function fansOut(): bool { foreach ($this->forwards as $forward) { foreach ($this->forwards as $other) { - if ($this->combineDistinctCallees($forward, $other)) { + if (self::areDistinctOnOnePath($forward, $other)) { return true; } } @@ -39,13 +39,9 @@ public function fansOut(): bool return false; } - /** - * @param array{callee: CalleeRef, path: BranchArmPath} $one - * @param array{callee: CalleeRef, path: BranchArmPath} $other - */ - private function combineDistinctCallees(array $one, array $other): bool + private static function areDistinctOnOnePath(ForwardOccurrence $one, ForwardOccurrence $other): bool { - return ! $one['callee']->isSameAs($other['callee']) - && ! $one['path']->isExclusiveWith($other['path']); + return ! $one->callee->isSameAs($other->callee) + && ! $one->branchArms->isExclusiveWith($other->branchArms); } } diff --git a/src/Index/ForwardOccurrence.php b/src/Index/ForwardOccurrence.php new file mode 100644 index 0000000..bbed6f8 --- /dev/null +++ b/src/Index/ForwardOccurrence.php @@ -0,0 +1,18 @@ +forwards); } + public function testForwardInElseifConditionIsFanOutWithForwardInAnotherArm(): void + { + // An elseif condition is evaluated on the way into the else arm. + $p = $this->classify('if ($a) { } elseif (check($p)) { } else { fallback($p); }')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + + public function testForwardInMatchArmConditionIsFanOutWithForwardInAnotherArm(): void + { + // A match arm's condition is evaluated on the way into every later arm. + $p = $this->classify('match ($x) { probe($p) => 1, default => handle($p) };')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + + public function testForwardInSwitchCaseConditionIsFanOutWithForwardInAnotherCase(): void + { + // A case expression is evaluated on the way into every later case. + $p = $this->classify('switch ($x) { case a($p): break; default: b($p); }')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + public function testTwoDistinctCalleesInsideOneArmAreFanOut(): void { $p = $this->classify('if ($x) { a($p); b($p); }')['p']; From bd1c668635d1b870b03f2e6e822004207a65baca Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Sun, 6 Sep 2026 08:36:51 +0200 Subject: [PATCH 4/7] feat(#26): report fan-out to distinct callees as a fan-out terminal, not a hop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A parameter that a method forwards to two or more distinct callees on one execution path is already classified as ParamFate::FanOut, but the chain traversal still rendered such a terminal as `used`. It now gets its own TerminalKind::FanOut, printed `(fan-out)` and selectable via `--exclude-terminal fan-out`, so a team can tell "the value arrived at a method that reads it through accessors" apart from an ordinary use. The issue's example — BackupReader::toDto dispatching to SubscriptionLine::fromLine, which builds its DTO from LineField::string/int/date — stops reporting a 3-hop chain through the accessors and reports the one hop that is really there: toDto -> fromLine (fan-out). Both boundaries the issue draws are pinned by fixtures: a `match` dispatcher that forwards to one factory per arm stays a hop (match-dispatcher-is-a-hop), and the same callee twice is still a pure forward (same-callee-two-callsites). Amends frozen rule 1 (documented in docs/plan.md), which now also records the two deliberate simplifications of the syntactic arm test: `switch` fallthrough counts as exclusive arms, and an early-return dispatcher counts as fan-out. --- README.md | 2 +- docs/plan.md | 18 ++++++ src/Chain/ChainTraversal.php | 4 ++ src/Chain/Finding.php | 2 +- src/Chain/TerminalKind.php | 9 +-- src/Console/Application.php | 2 +- tests/Chain/ChainBuilderTest.php | 34 +++++++++++ tests/Chain/TerminalKindFilterTest.php | 2 +- tests/Chain/TerminalKindTest.php | 1 + tests/Console/ApplicationTest.php | 4 +- .../fan-out-terminal/expected-findings.json | 49 +++++++++++++++ .../fan-out-terminal/phptramp-args.json | 1 + tests/fixtures/fan-out-terminal/src/Demo.php | 61 +++++++++++++++++++ .../expected-findings.json | 35 +++++++++++ .../phptramp-args.json | 1 + .../match-dispatcher-is-a-hop/src/Demo.php | 38 ++++++++++++ 16 files changed, 253 insertions(+), 10 deletions(-) create mode 100644 tests/fixtures/fan-out-terminal/expected-findings.json create mode 100644 tests/fixtures/fan-out-terminal/phptramp-args.json create mode 100644 tests/fixtures/fan-out-terminal/src/Demo.php create mode 100644 tests/fixtures/match-dispatcher-is-a-hop/expected-findings.json create mode 100644 tests/fixtures/match-dispatcher-is-a-hop/phptramp-args.json create mode 100644 tests/fixtures/match-dispatcher-is-a-hop/src/Demo.php diff --git a/README.md b/README.md index 3519ba6..d37b2dc 100644 --- a/README.md +++ b/README.md @@ -107,7 +107,7 @@ phptramp [options] --format text|pretty|json|github|checkstyle|sarif|summary (default: pretty on TTY, text otherwise) --color always|auto|never (default: auto; honors NO_COLOR in auto mode) --explain Show why chains ended (call resolution trace) - --exclude-terminal Do not report chains ending in (repeatable; used|stored|&-terminated|unused-end|external|truncated -- quote '&-terminated', or the shell backgrounds the command) + --exclude-terminal Do not report chains ending in (repeatable; used|stored|fan-out|&-terminated|unused-end|external|truncated -- quote '&-terminated', or the shell backgrounds the command) --changed-only Only report chains touching changed lines --git-base Diff base for --changed-only (default: origin/main) --diff Read the diff from a file, or stdin with '-' (implies --changed-only) diff --git a/docs/plan.md b/docs/plan.md index 753265c..380d5f8 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -80,6 +80,24 @@ implementation. Every bullet is a fixture. method call on `$p`, array access, use in an expression, assignment to or from `$p`, `return $p`, capture in a closure — makes the method a **terminal use** and ends the chain there. + + A method whose every occurrence of `$p` is a forward, but to two or more *distinct* + callees on one execution path, is a **fan-out terminal** (`fan-out`) rather than a + hop: it consumes the value by combining what those callees return (issue #26). + Forwards that reach distinct callees only in mutually exclusive arms (`if`/`elseif`/ + `else`, `match`, `switch`, ternary) are not fan-out — a dispatcher picks one callee + per run and is a genuine pass-through — and the same callee twice is not fan-out + either. A branch *condition* (an `elseif` condition, a `match` arm's condition, a + `switch` case expression included) is never an arm: a forward there shares the path + with every arm. The test is syntactic, so two shapes are decided by where a forward + sits rather than by what actually runs: `switch` fallthrough + (`case 1: a($p); case 2: b($p);` with no `break`) counts as exclusive arms and leaves + the method a hop, and an early-return dispatcher + (`if ($x) { return a($p); } return b($p);`) counts as fan-out because the second + forward sits in no arm. Both are deliberate simplifications of the shape test; + the early-return one is the conservative direction (the chain ends there, so fewer + findings), the fallthrough one leaves the pre-amendment classification in place. + `--exclude-terminal fan-out` targets the kind. 2. **By-ref receipt terminates.** A parameter declared `&$p` is `ByRefTerminated`: the method may write to it, so it is never a mindless hop. Reported with an `&-terminated` note. diff --git a/src/Chain/ChainTraversal.php b/src/Chain/ChainTraversal.php index b5b8dd9..536faa1 100644 --- a/src/Chain/ChainTraversal.php +++ b/src/Chain/ChainTraversal.php @@ -234,6 +234,10 @@ private function terminalKind(ParamInfo $param): TerminalKind return TerminalKind::Unused; } + if ($param->fate === ParamFate::FanOut) { + return TerminalKind::FanOut; + } + return $param->storedOnly ? TerminalKind::Stored : TerminalKind::Used; } diff --git a/src/Chain/Finding.php b/src/Chain/Finding.php index c573146..1262f87 100644 --- a/src/Chain/Finding.php +++ b/src/Chain/Finding.php @@ -14,7 +14,7 @@ final class Finding { /** * @param list $chain hops in order; includes the terminal node only when - * terminalKind is used|stored|&-terminated|unused-end + * terminalKind is used|stored|fan-out|&-terminated|unused-end * @param list $notes human-readable, e.g. "truncated: 2 implementations" * @param list $trace per-edge resolution trace, rendered only by --explain */ diff --git a/src/Chain/TerminalKind.php b/src/Chain/TerminalKind.php index a294f05..0e34c77 100644 --- a/src/Chain/TerminalKind.php +++ b/src/Chain/TerminalKind.php @@ -6,14 +6,15 @@ /** * How a tramp-data chain ends. The backing values are the tokens the reporters - * print. `used`/`stored`/`&-terminated`/`unused-end` keep the terminal node in - * the chain; `external`/`truncated` do not (the chain left analyzed code or was - * conservatively cut short). + * print. `used`/`stored`/`fan-out`/`&-terminated`/`unused-end` keep the terminal + * node in the chain; `external`/`truncated` do not (the chain left analyzed code + * or was conservatively cut short). */ enum TerminalKind: string { case Used = 'used'; case Stored = 'stored'; + case FanOut = 'fan-out'; case ByRef = '&-terminated'; case Unused = 'unused-end'; case External = 'external'; @@ -29,7 +30,7 @@ enum TerminalKind: string public function keepsTerminalNode(): bool { return match ($this) { - self::Used, self::Stored, self::ByRef, self::Unused => true, + self::Used, self::Stored, self::FanOut, self::ByRef, self::Unused => true, self::External, self::Truncated => false, }; } diff --git a/src/Console/Application.php b/src/Console/Application.php index b254add..e65832a 100644 --- a/src/Console/Application.php +++ b/src/Console/Application.php @@ -441,7 +441,7 @@ private static function helpText(): string --format text|pretty|json|github|checkstyle|sarif|summary (default: pretty on TTY, text otherwise) --color always|auto|never (default: auto; honors NO_COLOR in auto mode) --explain Show why chains ended (call resolution trace) - --exclude-terminal Do not report chains ending in (repeatable; used|stored|&-terminated|unused-end|external|truncated) + --exclude-terminal Do not report chains ending in (repeatable; used|stored|fan-out|&-terminated|unused-end|external|truncated) Diff-aware mode: --changed-only Only report chains touching changed lines diff --git a/tests/Chain/ChainBuilderTest.php b/tests/Chain/ChainBuilderTest.php index b1ec79d..4e6d34e 100644 --- a/tests/Chain/ChainBuilderTest.php +++ b/tests/Chain/ChainBuilderTest.php @@ -147,6 +147,40 @@ public function testUnusedCalleeTerminatesAsUnused(): void self::assertSame(TerminalKind::Unused, $findings[0]->terminalKind); } + public function testFanOutCalleeTerminatesAsFanOut(): void + { + $code = 'consume($p); } } ' + . 'class B { public function consume(Cfg $p): void { (new C())->x($p); (new D())->y($p); } } ' + . 'class C { public function x(Cfg $p): void { $p->go(); } } ' + . 'class D { public function y(Cfg $p): void { $p->go(); } }'; + + $findings = $this->build($code); + self::assertCount(1, $findings); + + $finding = $findings[0]; + self::assertSame('Demo\A::go', $finding->origin); + self::assertSame('Demo\B::consume', $finding->terminal); + self::assertSame(TerminalKind::FanOut, $finding->terminalKind); + self::assertSame(1, $finding->hops); + self::assertCount(2, $finding->chain); + self::assertSame('Demo\B::consume', $finding->chain[1]->fqmn); + } + + /** + * A fan-out method consumes its own parameter, so a chain that would start + * there has no pass-through node at all and is not tramp data. + */ + public function testOriginThatFansOutIsNotReported(): void + { + $code = 'x($p); (new C())->y($p); } } ' + . 'class B { public function x(Cfg $p): void { $p->go(); } } ' + . 'class C { public function y(Cfg $p): void { $p->go(); } }'; + + self::assertSame([], $this->build($code)); + } + public function testForwardIntoUnknownUserFunctionTerminatesAsExternal(): void { $code = 'getMessage(), ); } diff --git a/tests/Chain/TerminalKindTest.php b/tests/Chain/TerminalKindTest.php index 1a9ead3..922a244 100644 --- a/tests/Chain/TerminalKindTest.php +++ b/tests/Chain/TerminalKindTest.php @@ -17,6 +17,7 @@ public static function terminalKindProvider(): iterable { yield 'used keeps its node' => [TerminalKind::Used, true]; yield 'stored keeps its node' => [TerminalKind::Stored, true]; + yield 'fan-out keeps its node' => [TerminalKind::FanOut, true]; yield 'by-ref keeps its node' => [TerminalKind::ByRef, true]; yield 'unused-end keeps its node' => [TerminalKind::Unused, true]; yield 'external has no node' => [TerminalKind::External, false]; diff --git a/tests/Console/ApplicationTest.php b/tests/Console/ApplicationTest.php index 641a730..c1db406 100644 --- a/tests/Console/ApplicationTest.php +++ b/tests/Console/ApplicationTest.php @@ -773,8 +773,8 @@ public function testUnknownExcludeTerminalKindExitsTwoBeforeTheIndexIsBuilt(): v self::assertSame(2, $exitCode); self::assertStringContainsString( - 'phptramp: unknown terminal kind: bogus (expected used|stored|&-terminated' - . '|unused-end|external|truncated)', + 'phptramp: unknown terminal kind: bogus (expected used|stored|fan-out' + . '|&-terminated|unused-end|external|truncated)', self::contents($this->stderr), ); } diff --git a/tests/fixtures/fan-out-terminal/expected-findings.json b/tests/fixtures/fan-out-terminal/expected-findings.json new file mode 100644 index 0000000..a6d3d7c --- /dev/null +++ b/tests/fixtures/fan-out-terminal/expected-findings.json @@ -0,0 +1,49 @@ +{ + "limit": 1, + "warnLimit": null, + "minClasses": 0, + "findings": [ + { + "param": "key", + "severity": "error", + "origin": "Demo\\LineField::date", + "terminal": "Demo\\LineField::string", + "terminalKind": "used", + "hops": 1, + "classes": 1, + "chain": [ + {"method": "Demo\\LineField::date", "role": "origin", "file": "src/Demo.php", "line": 46, "forwardLine": 48}, + {"method": "Demo\\LineField::string", "role": "terminal", "file": "src/Demo.php", "line": 26, "forwardLine": null} + ], + "notes": [] + }, + { + "param": "line", + "severity": "error", + "origin": "Demo\\LineField::date", + "terminal": "Demo\\LineField::string", + "terminalKind": "used", + "hops": 1, + "classes": 1, + "chain": [ + {"method": "Demo\\LineField::date", "role": "origin", "file": "src/Demo.php", "line": 46, "forwardLine": 48}, + {"method": "Demo\\LineField::string", "role": "terminal", "file": "src/Demo.php", "line": 26, "forwardLine": null} + ], + "notes": [] + }, + { + "param": "decoded", + "severity": "error", + "origin": "Demo\\BackupReader::toDto", + "terminal": "Demo\\SubscriptionLine::fromLine", + "terminalKind": "fan-out", + "hops": 1, + "classes": 2, + "chain": [ + {"method": "Demo\\BackupReader::toDto", "role": "origin", "file": "src/Demo.php", "line": 54, "forwardLine": 57}, + {"method": "Demo\\SubscriptionLine::fromLine", "role": "terminal", "file": "src/Demo.php", "line": 14, "forwardLine": null} + ], + "notes": [] + } + ] +} diff --git a/tests/fixtures/fan-out-terminal/phptramp-args.json b/tests/fixtures/fan-out-terminal/phptramp-args.json new file mode 100644 index 0000000..0c7bbe6 --- /dev/null +++ b/tests/fixtures/fan-out-terminal/phptramp-args.json @@ -0,0 +1 @@ +["--limit", "1", "--warn-limit", "0"] diff --git a/tests/fixtures/fan-out-terminal/src/Demo.php b/tests/fixtures/fan-out-terminal/src/Demo.php new file mode 100644 index 0000000..53f1176 --- /dev/null +++ b/tests/fixtures/fan-out-terminal/src/Demo.php @@ -0,0 +1,61 @@ + SubscriptionLine::fromLine($decoded), + default => throw new \LogicException($kind), + }; + } +} diff --git a/tests/fixtures/match-dispatcher-is-a-hop/expected-findings.json b/tests/fixtures/match-dispatcher-is-a-hop/expected-findings.json new file mode 100644 index 0000000..5dbac83 --- /dev/null +++ b/tests/fixtures/match-dispatcher-is-a-hop/expected-findings.json @@ -0,0 +1,35 @@ +{ + "limit": 1, + "warnLimit": null, + "minClasses": 0, + "findings": [ + { + "param": "line", + "severity": "error", + "origin": "Demo\\Dispatcher::toDto", + "terminal": "Demo\\FooLine::fromLine", + "terminalKind": "used", + "hops": 1, + "classes": 2, + "chain": [ + {"method": "Demo\\Dispatcher::toDto", "role": "origin", "file": "src/Demo.php", "line": 7, "forwardLine": 10}, + {"method": "Demo\\FooLine::fromLine", "role": "terminal", "file": "src/Demo.php", "line": 22, "forwardLine": null} + ], + "notes": [] + }, + { + "param": "line", + "severity": "error", + "origin": "Demo\\Dispatcher::toDto", + "terminal": "Demo\\BarLine::fromLine", + "terminalKind": "used", + "hops": 1, + "classes": 2, + "chain": [ + {"method": "Demo\\Dispatcher::toDto", "role": "origin", "file": "src/Demo.php", "line": 7, "forwardLine": 11}, + {"method": "Demo\\BarLine::fromLine", "role": "terminal", "file": "src/Demo.php", "line": 34, "forwardLine": null} + ], + "notes": [] + } + ] +} diff --git a/tests/fixtures/match-dispatcher-is-a-hop/phptramp-args.json b/tests/fixtures/match-dispatcher-is-a-hop/phptramp-args.json new file mode 100644 index 0000000..0c7bbe6 --- /dev/null +++ b/tests/fixtures/match-dispatcher-is-a-hop/phptramp-args.json @@ -0,0 +1 @@ +["--limit", "1", "--warn-limit", "0"] diff --git a/tests/fixtures/match-dispatcher-is-a-hop/src/Demo.php b/tests/fixtures/match-dispatcher-is-a-hop/src/Demo.php new file mode 100644 index 0000000..c52c89f --- /dev/null +++ b/tests/fixtures/match-dispatcher-is-a-hop/src/Demo.php @@ -0,0 +1,38 @@ + FooLine::fromLine($line), + 'bar' => BarLine::fromLine($line), + }; + } +} + +class FooLine +{ + public function __construct(private string $x) + { + } + + public static function fromLine(array $line): self + { + return new self($line['x']); + } +} + +class BarLine +{ + public function __construct(private string $x) + { + } + + public static function fromLine(array $line): self + { + return new self($line['x']); + } +} From 14ee64afe88fbe6daef1d39e976a5ae14f817b1a Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Sun, 6 Sep 2026 08:53:34 +0200 Subject: [PATCH 5/7] test(#26): pin parent:: fan-out, kill the walk() continue mutant, dump FanOut in the smoke index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three gaps the fan-out work left uncovered. `parent::` forwards already count as distinct callees, but nothing pinned it. Rule 4 exempts `parent::` from the hop and class counts because it hands the value to the same object; that exemption deliberately does not extend to fan-out, since the base-class chain is re-reported from the base method as its own origin. Two cases now hold the ruling in place. Infection reported an escaped `continue` -> `break` mutant in ChainTraversal::walk(): with only one forward per method in the suite, stopping after an internal-function use looked identical to skipping it. A hop whose two exclusive arms forward into `count()` and into a user method distinguishes them — the second finding disappears under `break`. The classifier-smoke fixture pinned every fate token in `--dump-index` except FanOut. A fan-out method in its source, with both forwards spelled out, closes that. Its findings output is unchanged: a FanOut node is not a pure-forward node, so it starts no chain and adds no incoming edge. --- tests/Chain/ChainBuilderTest.php | 24 +++++++++++++++++++ tests/Index/UsageClassifierTest.php | 17 +++++++++++++ .../classifier-smoke/expected-index.json | 4 ++++ tests/fixtures/classifier-smoke/src/Demo.php | 9 +++++++ 4 files changed, 54 insertions(+) diff --git a/tests/Chain/ChainBuilderTest.php b/tests/Chain/ChainBuilderTest.php index 4e6d34e..3b17c44 100644 --- a/tests/Chain/ChainBuilderTest.php +++ b/tests/Chain/ChainBuilderTest.php @@ -242,6 +242,30 @@ public function testLongChainEndingInInternalFunctionCountsOneFewerHop(): void ], $finding->trace); } + public function testInternalFunctionUseInOneArmDoesNotStopTheOtherArmsForward(): void + { + // B::relay stays a hop (its two forwards sit in exclusive arms), and one + // of them lands in an internal function. Recording that use terminates + // only its own branch: the walk must go on to the remaining forward. + $code = 'relay($p); } } ' + . 'class B { public function relay(Cfg $p): void { ' + . 'if (random_int(0, 1) === 1) { count($p); } else { (new C())->y($p); } } } ' + . 'class C { public function y(Cfg $p): void { $p->z(); } }'; + + $findings = $this->build($code); + self::assertCount(2, $findings); + + $terminals = []; + foreach ($findings as $finding) { + self::assertSame('Demo\A::go', $finding->origin); + self::assertSame(TerminalKind::Used, $finding->terminalKind); + $terminals[] = $finding->terminal; + } + sort($terminals); + self::assertSame(['Demo\B::relay', 'Demo\C::y'], $terminals); + } + public function testOriginForwardingOnlyToInternalFunctionIsNotReported(): void { $code = 'forwards); } + public function testParentCallBesideACallOnThisIsFanOut(): void + { + // Conservative: rule 4's "same object" exemption spares `parent::` from + // the hop count, it does not spare it from fan-out. The base-class chain + // is reported from the base method as its own origin, so nothing is lost. + $p = $this->classify('parent::__construct($p); $this->init($p);')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + + public function testTwoDistinctParentCallsAreFanOut(): void + { + $p = $this->classify('parent::__construct($p); parent::init($p);')['p']; + self::assertSame(ParamFate::FanOut, $p->fate); + self::assertCount(2, $p->forwards); + } + public function testSameMethodOnTwoReceiversIsFanOut(): void { $p = $this->classify('$x->save($p); $y->save($p);', 'Cfg $p, Repo $x, Repo $y')['p']; diff --git a/tests/fixtures/classifier-smoke/expected-index.json b/tests/fixtures/classifier-smoke/expected-index.json index cd8891c..76cd668 100644 --- a/tests/fixtures/classifier-smoke/expected-index.json +++ b/tests/fixtures/classifier-smoke/expected-index.json @@ -15,6 +15,10 @@ "Demo\\Mailer::__construct": { "file": "src/Demo.php", "params": [{"name": "config", "fate": "Used", "forwards": []}] + }, + "Demo\\Auditor::record": { + "file": "src/Demo.php", + "params": [{"name": "config", "fate": "FanOut", "forwards": ["method:process@0", "method:run@0"]}] } } } diff --git a/tests/fixtures/classifier-smoke/src/Demo.php b/tests/fixtures/classifier-smoke/src/Demo.php index 81a7977..4a3bbcb 100644 --- a/tests/fixtures/classifier-smoke/src/Demo.php +++ b/tests/fixtures/classifier-smoke/src/Demo.php @@ -39,3 +39,12 @@ public function __construct(Cfg $config) $this->config = $config; } } + +class Auditor +{ + public function record(Cfg $config): void + { + (new ServiceA())->process($config); + (new ServiceB())->run($config); + } +} From ffefc298ba5ac5999cdcee6d3e7006b1d3935a64 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Sun, 6 Sep 2026 08:55:02 +0200 Subject: [PATCH 6/7] docs(#26): record parent::, raw-receiver, and dispatcher edges of the fan-out rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fan-out amendment named the shapes it was designed against; review turned up four more the frozen text did not settle, each now written down with its direction of error. `parent::` forwards count as distinct callees — the reader who knows rule 4 exempts `parent::` from the hop and class counts would otherwise assume the exemption carries over. Callee identity is a `CalleeRef` triple, so it errs in both directions and the record now says where. `self::a($p); Foo::a($p);` over-counts (two callees, one method); two property-fetch receivers under-count, because both hint `raw` and collapse into one callee. The second is the direction that yields more findings than truth, so it is worth naming that it is bounded: the resolver truncates at a `raw` receiver anyway. Every guard-clause dispatcher is fan-out, not just the `if/return` the text already showed — `instanceof` ladders and `try`/`catch` land the same way. The `switch` fallthrough sentence lacked its direction; it is the one simplification that yields more findings than truth. Also adds the missing `FanOut` -> `fan-out` row to the Phase 4 terminal mapping, and disambiguates rule 6's "fan-out count", which means implementations behind a truncated call and predates the terminal kind entirely. --- docs/plan.md | 45 ++++++++++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/docs/plan.md b/docs/plan.md index 380d5f8..0ef1881 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -89,15 +89,33 @@ implementation. Every bullet is a fixture. per run and is a genuine pass-through — and the same callee twice is not fan-out either. A branch *condition* (an `elseif` condition, a `match` arm's condition, a `switch` case expression included) is never an arm: a forward there shares the path - with every arm. The test is syntactic, so two shapes are decided by where a forward - sits rather than by what actually runs: `switch` fallthrough - (`case 1: a($p); case 2: b($p);` with no `break`) counts as exclusive arms and leaves - the method a hop, and an early-return dispatcher - (`if ($x) { return a($p); } return b($p);`) counts as fan-out because the second - forward sits in no arm. Both are deliberate simplifications of the shape test; - the early-return one is the conservative direction (the chain ends there, so fewer - findings), the fallthrough one leaves the pre-amendment classification in place. - `--exclude-terminal fan-out` targets the kind. + with every arm. A `parent::` forward counts as a callee like any other: + `parent::__construct($p); $this->init($p);` is fan-out. Rule 4's "same object" + exemption spares `parent::` from the hop and class counts only — it does not extend + to fan-out, and nothing is lost by ending the chain there, because the base-class + chain is reported from the base method as its own origin. + + Callee identity is syntactic: a `CalleeRef`'s kind, name, and receiver hint. Two + consequences are on the record. `self::a($p); Foo::a($p);` inside `Foo` counts as two + callees even though both reach the same method (conservative — the chain ends there). + `$this->left->save($p); $this->right->save($p);` collapses into *one* callee, because + every property-fetch receiver hints `raw`: same-named methods on two collaborators + leave the method a hop, which is more findings than truth. That over-reporting is + bounded — the resolver cannot follow a `raw` receiver either, so it truncates at that + forward. Different method names on `raw` receivers do fan out. + + The test is syntactic, so two shapes are decided by where a forward sits rather than + by what actually runs. Any early-return or guard-clause dispatcher counts as fan-out, + because no two of its forwards sit in different arms of the same branch: + `if ($x) { return a($p); } return b($p);`, the `instanceof` ladder + (`if ($x instanceof A) { return a($p); } if ($x instanceof B) { return b($p); } + throw …`), and `try { a($p); } catch (\Throwable) { b($p); }` alike. `switch` + fallthrough (`case 1: a($p); case 2: b($p);` with no `break`) goes the other way and + counts as exclusive arms. Both are deliberate simplifications of the shape test; the + early-return one is the conservative direction (the chain ends there, so fewer + findings), the fallthrough one leaves the pre-amendment classification in place — the + method keeps being a hop, so chains run on through it, which is more findings than + truth. `--exclude-terminal fan-out` targets the kind. 2. **By-ref receipt terminates.** A parameter declared `&$p` is `ByRefTerminated`: the method may write to it, so it is never a mindless hop. Reported with an `&-terminated` note. @@ -127,8 +145,9 @@ implementation. Every bullet is a fixture. (`function a(...$args) { b(...$args); }` is a hop). 6. **Interfaces/abstract types:** follow the call **only when exactly one implementation exists in the analyzed code**; otherwise truncate with note - `"N implementations, chain truncated"`. Record the fan-out count (future - `--follow-all-implementations`). + `"N implementations, chain truncated"`. Record the fan-out count — the + implementation count, not the `fan-out` terminal of rule 1 — for a future + `--follow-all-implementations`. 7. **Out of scope v0.1** (truncate with a note where detectable): closures / arrow functions, first-class callable syntax, `call_user_func*`, `__call`/`__callStatic`, dynamic method names, `func_get_args()`. @@ -705,8 +724,8 @@ Tasks (expanded in `docs/plans/phase-2.md`): excludes lines by design). 4. **Terminals:** child fate `Used` → terminal kind `used` (or `stored` when the body is a single property assignment); `ByRefTerminated` → `&-terminated`; - `Unused` → `unused-end` (dead forwarding — the value goes nowhere); leaf from - resolution → `external` or `truncated:`. + `Unused` → `unused-end` (dead forwarding — the value goes nowhere); + `FanOut` → `fan-out`; leaf from resolution → `external` or `truncated:`. 5. **Hops** = count of `PureForward` nodes on the path (terminal never counted). `Finding{param, origin, terminal, terminalKind, hops, chain[], classes, notes}`; `classes` = distinct declaring classes across chain incl. terminal. From 2bdb89c53d9df9b06a1612f10e8251f3df53ce35 Mon Sep 17 00:00:00 2001 From: Lars Pohlmann Date: Sun, 6 Sep 2026 09:10:03 +0200 Subject: [PATCH 7/7] refactor(#26): label arm nodes by identity, dispatch terminal kind with match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplification pass over the fan-out change: - BranchArmPath labelled `elseif`/`else`/match-arm/case arms by their position among siblings, which needed an enclosing-branch lookup and a sibling scan per arm, copied four times with one word changed. Labels are only ever compared between arms of one branch, so the arm node's own object id is a label that needs no scan — one helper replaces four. - ChainTraversal::terminalKind() had grown into three sequential guards with a silent catch-all; an exhaustive match on ParamFate makes the next fate a compile-time omission instead of a silently `used` chain. - ForwardOccurrence wraps the ForwardSite the collector already records instead of re-holding its CalleeRef. - UsageClassifier returns one tuple for the two forward-only fates. - Finding's docblock points at TerminalKind::keepsTerminalNode() instead of restating its list; docs/plan.md stops overloading "fan-out count" for the interface implementation count and records `$this->go($p); $c->go($p);` next to the `self::`/`Foo::` identity note. No behaviour change; composer check green, diff-scoped MSI 99%. --- docs/plan.md | 12 +-- src/Chain/ChainTraversal.php | 19 ++--- src/Chain/Finding.php | 2 +- src/Index/BranchArmPath.php | 126 ++++++-------------------------- src/Index/FanOutDetector.php | 6 +- src/Index/ForwardCollector.php | 2 +- src/Index/ForwardOccurrence.php | 6 +- src/Index/UsageClassifier.php | 10 +-- 8 files changed, 47 insertions(+), 136 deletions(-) diff --git a/docs/plan.md b/docs/plan.md index 0ef1881..693d798 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -96,8 +96,9 @@ implementation. Every bullet is a fixture. chain is reported from the base method as its own origin. Callee identity is syntactic: a `CalleeRef`'s kind, name, and receiver hint. Two - consequences are on the record. `self::a($p); Foo::a($p);` inside `Foo` counts as two - callees even though both reach the same method (conservative — the chain ends there). + consequences are on the record. `self::a($p); Foo::a($p);` inside `Foo`, like + `$this->go($p); $c->go($p);` with `$c` typed as the own class, counts as two callees + even though both reach the same method (conservative — the chain ends there). `$this->left->save($p); $this->right->save($p);` collapses into *one* callee, because every property-fetch receiver hints `raw`: same-named methods on two collaborators leave the method a hop, which is more findings than truth. That over-reporting is @@ -145,9 +146,8 @@ implementation. Every bullet is a fixture. (`function a(...$args) { b(...$args); }` is a hop). 6. **Interfaces/abstract types:** follow the call **only when exactly one implementation exists in the analyzed code**; otherwise truncate with note - `"N implementations, chain truncated"`. Record the fan-out count — the - implementation count, not the `fan-out` terminal of rule 1 — for a future - `--follow-all-implementations`. + `"N implementations, chain truncated"`. Record the implementation count (future + `--follow-all-implementations`). 7. **Out of scope v0.1** (truncate with a note where detectable): closures / arrow functions, first-class callable syntax, `call_user_func*`, `__call`/`__callStatic`, dynamic method names, `func_get_args()`. @@ -872,7 +872,7 @@ Explicit maybe-laters recorded during Phases 0–6; each waits on a concrete con ask unless the measurement already says "not yet". - **`--follow-all-implementations`:** follow all N implementations of an interface - instead of the single-implementation default — fan-out counts are already recorded + instead of the single-implementation default — implementation counts are already recorded since Phase 2 (see the single-implementation follow-through bullet in the Appendix), so this is a small flag, not a rewrite. Origin: Phase 2 frozen-semantics decision. - **Field/property tramping:** constructor-stored values re-forwarded via properties diff --git a/src/Chain/ChainTraversal.php b/src/Chain/ChainTraversal.php index 536faa1..d160046 100644 --- a/src/Chain/ChainTraversal.php +++ b/src/Chain/ChainTraversal.php @@ -226,19 +226,12 @@ private function terminalUse(MethodInfo $method, ParamInfo $param): Terminal private function terminalKind(ParamInfo $param): TerminalKind { - if ($param->fate === ParamFate::ByRefTerminated) { - return TerminalKind::ByRef; - } - - if ($param->fate === ParamFate::Unused) { - return TerminalKind::Unused; - } - - if ($param->fate === ParamFate::FanOut) { - return TerminalKind::FanOut; - } - - return $param->storedOnly ? TerminalKind::Stored : TerminalKind::Used; + return match ($param->fate) { + ParamFate::ByRefTerminated => TerminalKind::ByRef, + ParamFate::Unused => TerminalKind::Unused, + ParamFate::FanOut => TerminalKind::FanOut, + ParamFate::Used, ParamFate::PureForward => $param->storedOnly ? TerminalKind::Stored : TerminalKind::Used, + }; } private function record(PartialChain $chain, Terminal $terminal): void diff --git a/src/Chain/Finding.php b/src/Chain/Finding.php index 1262f87..e591da1 100644 --- a/src/Chain/Finding.php +++ b/src/Chain/Finding.php @@ -14,7 +14,7 @@ final class Finding { /** * @param list $chain hops in order; includes the terminal node only when - * terminalKind is used|stored|fan-out|&-terminated|unused-end + * `terminalKind->keepsTerminalNode()` * @param list $notes human-readable, e.g. "truncated: 2 implementations" * @param list $trace per-edge resolution trace, rendered only by --explain */ diff --git a/src/Index/BranchArmPath.php b/src/Index/BranchArmPath.php index a903a72..bf4ac9c 100644 --- a/src/Index/BranchArmPath.php +++ b/src/Index/BranchArmPath.php @@ -5,14 +5,12 @@ namespace PhpTramp\Index; use PhpParser\Node; -use PhpParser\Node\Expr\Match_; use PhpParser\Node\Expr\Ternary; use PhpParser\Node\MatchArm; use PhpParser\Node\Stmt\Case_; use PhpParser\Node\Stmt\Else_; use PhpParser\Node\Stmt\ElseIf_; use PhpParser\Node\Stmt\If_; -use PhpParser\Node\Stmt\Switch_; /** * Which arm of every enclosing branching construct a syntax node sits in. Two @@ -21,7 +19,7 @@ */ final class BranchArmPath { - /** @param array $armByBranch arm label keyed by branching node object id */ + /** @param array $armByBranch arm label keyed by branching node object id */ private function __construct(private readonly array $armByBranch) { } @@ -37,9 +35,9 @@ private function __construct(private readonly array $armByBranch) * also the conservative direction: a shared path can only yield fewer * findings. * - * Object ids are safe as keys here because the whole method body stays - * alive for as long as any path derived from it is compared, so no id can - * be recycled onto a different node in between. + * Object ids are safe as keys and labels here because the whole method + * body stays alive for as long as any path derived from it is compared, so + * no id can be recycled onto a different node in between. */ public static function of(Node $node): self { @@ -70,93 +68,24 @@ public function isExclusiveWith(self $other): bool } /** - * @return array{Node, string}|null the branching node the label belongs to + * @return array{Node, int|string}|null the branching node the label belongs to * and the label, or null when the child * does not sit in an arm of the parent */ private static function armOf(Node $parent, Node $child): ?array { return match (true) { - $parent instanceof If_ => self::thenArm($parent, $child), - $parent instanceof ElseIf_ => self::elseifArm($parent, $child), - $parent instanceof Else_ => self::elseArm($parent, $child), - $parent instanceof MatchArm => self::matchArm($parent, $child), - $parent instanceof Case_ => self::caseArm($parent, $child), + $parent instanceof If_ => in_array($child, $parent->stmts, true) ? [$parent, 'then'] : null, $parent instanceof Ternary => self::ternaryArm($parent, $child), + $parent instanceof ElseIf_, + $parent instanceof Else_, + $parent instanceof Case_ => self::armNode($parent, $parent->stmts, $child), + $parent instanceof MatchArm => self::armNode($parent, [$parent->body], $child), default => null, }; } - /** @return array{Node, string}|null */ - private static function thenArm(If_ $branch, Node $child): ?array - { - return in_array($child, $branch->stmts, true) ? [$branch, 'then'] : null; - } - - /** @return array{Node, string}|null */ - private static function elseifArm(ElseIf_ $arm, Node $child): ?array - { - if (! in_array($child, $arm->stmts, true)) { - return null; - } - - $branch = self::enclosingBranch($arm, If_::class); - if ($branch === null) { - return null; - } - - $index = self::indexOf($arm, $branch->elseifs); - - return $index === null ? null : [$branch, 'elseif:' . $index]; - } - - /** @return array{Node, string}|null */ - private static function elseArm(Else_ $arm, Node $child): ?array - { - if (! in_array($child, $arm->stmts, true)) { - return null; - } - - $branch = self::enclosingBranch($arm, If_::class); - - return $branch === null ? null : [$branch, 'else']; - } - - /** @return array{Node, string}|null */ - private static function matchArm(MatchArm $arm, Node $child): ?array - { - if ($child !== $arm->body) { - return null; - } - - $branch = self::enclosingBranch($arm, Match_::class); - if ($branch === null) { - return null; - } - - $index = self::indexOf($arm, $branch->arms); - - return $index === null ? null : [$branch, $index]; - } - - /** @return array{Node, string}|null */ - private static function caseArm(Case_ $arm, Node $child): ?array - { - if (! in_array($child, $arm->stmts, true)) { - return null; - } - - $branch = self::enclosingBranch($arm, Switch_::class); - if ($branch === null) { - return null; - } - - $index = self::indexOf($arm, $branch->cases); - - return $index === null ? null : [$branch, $index]; - } - - /** @return array{Node, string}|null */ + /** @return array{Node, int|string}|null */ private static function ternaryArm(Ternary $branch, Node $child): ?array { if ($child === $branch->if) { @@ -167,34 +96,23 @@ private static function ternaryArm(Ternary $branch, Node $child): ?array } /** - * @template TBranch of Node + * An arm that is a node of its own (`elseif`, `else`, a match arm, a case) + * hangs off the branching node as its parent and is labelled by its own + * identity: labels are only ever compared between arms of one branch, so + * any value that differs per arm will do, and the object id needs no scan + * of the arm's siblings. * - * @param class-string $branchType + * @param array $body * - * @return TBranch|null the branching node the arm belongs to, or null when - * the arm is not attached to one + * @return array{Node, int|string}|null */ - private static function enclosingBranch(Node $arm, string $branchType): ?Node + private static function armNode(Node $arm, array $body, Node $child): ?array { $branch = $arm->getAttribute('parent'); - - return $branch instanceof $branchType ? $branch : null; - } - - /** - * @param array $arms - * - * @return string|null the arm's position among its siblings, or null when - * it is not one of them - */ - private static function indexOf(Node $arm, array $arms): ?string - { - foreach ($arms as $index => $sibling) { - if ($sibling === $arm) { - return (string) $index; - } + if (! $branch instanceof Node || ! in_array($child, $body, true)) { + return null; } - return null; + return [$branch, spl_object_id($arm)]; } } diff --git a/src/Index/FanOutDetector.php b/src/Index/FanOutDetector.php index cb0181f..b2b1b98 100644 --- a/src/Index/FanOutDetector.php +++ b/src/Index/FanOutDetector.php @@ -16,9 +16,9 @@ final class FanOutDetector /** @var list */ private array $forwards = []; - public function record(CalleeRef $callee, BranchArmPath $path): void + public function record(ForwardSite $site, BranchArmPath $branchArms): void { - $this->forwards[] = new ForwardOccurrence($callee, $path); + $this->forwards[] = new ForwardOccurrence($site, $branchArms); } /** @@ -41,7 +41,7 @@ public function fansOut(): bool private static function areDistinctOnOnePath(ForwardOccurrence $one, ForwardOccurrence $other): bool { - return ! $one->callee->isSameAs($other->callee) + return ! $one->site->callee->isSameAs($other->site->callee) && ! $one->branchArms->isExclusiveWith($other->branchArms); } } diff --git a/src/Index/ForwardCollector.php b/src/Index/ForwardCollector.php index a5846ee..fa52f0e 100644 --- a/src/Index/ForwardCollector.php +++ b/src/Index/ForwardCollector.php @@ -123,7 +123,7 @@ private function handleVariable(Variable $node): void $this->forwards[] = $forward; $this->sawNonStore = true; - $this->fanOutDetector->record($forward->callee, BranchArmPath::of($node)); + $this->fanOutDetector->record($forward, BranchArmPath::of($node)); } private function markUse(bool $isStore): void diff --git a/src/Index/ForwardOccurrence.php b/src/Index/ForwardOccurrence.php index bbed6f8..93a7dee 100644 --- a/src/Index/ForwardOccurrence.php +++ b/src/Index/ForwardOccurrence.php @@ -5,13 +5,13 @@ namespace PhpTramp\Index; /** - * One forward of a parameter as the fan-out analysis sees it: the callee that - * received it and the branch arms it sits in. + * One forward of a parameter as the fan-out analysis sees it: the site that + * forwarded it and the branch arms that site sits in. */ final class ForwardOccurrence { public function __construct( - public readonly CalleeRef $callee, + public readonly ForwardSite $site, public readonly BranchArmPath $branchArms, ) { } diff --git a/src/Index/UsageClassifier.php b/src/Index/UsageClassifier.php index 3205eab..2071b2a 100644 --- a/src/Index/UsageClassifier.php +++ b/src/Index/UsageClassifier.php @@ -87,11 +87,11 @@ private function analyzeBody(string $name, bool $variadic, array $stmts): array return [ParamFate::Unused, [], false]; } - if ($collector->fansOut()) { - return [ParamFate::FanOut, $collector->forwards, false]; - } - - return [ParamFate::PureForward, $collector->forwards, false]; + return [ + $collector->fansOut() ? ParamFate::FanOut : ParamFate::PureForward, + $collector->forwards, + false, + ]; } private function typeToString(?Node $type): ?string