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..693d798 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -80,6 +80,43 @@ 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. 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`, 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 + 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. @@ -109,7 +146,7 @@ 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 + `"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`, @@ -687,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. @@ -835,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/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/Chain/ChainTraversal.php b/src/Chain/ChainTraversal.php index b5b8dd9..d160046 100644 --- a/src/Chain/ChainTraversal.php +++ b/src/Chain/ChainTraversal.php @@ -226,15 +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; - } - - 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 c573146..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|&-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/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/src/Index/BranchArmPath.php b/src/Index/BranchArmPath.php new file mode 100644 index 0000000..bf4ac9c --- /dev/null +++ b/src/Index/BranchArmPath.php @@ -0,0 +1,118 @@ + $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. + * + * 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 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 + { + $armByBranch = []; + $child = $node; + + while (($parent = $child->getAttribute('parent')) instanceof Node) { + $arm = self::armOf($parent, $child); + if ($arm !== null) { + [$branch, $label] = $arm; + $armByBranch[spl_object_id($branch)] = $label; + } + $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; + } + + /** + * @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_ => 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, int|string}|null */ + private static function ternaryArm(Ternary $branch, Node $child): ?array + { + if ($child === $branch->if) { + return [$branch, 'then']; + } + + return $child === $branch->else ? [$branch, 'else'] : null; + } + + /** + * 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 array $body + * + * @return array{Node, int|string}|null + */ + private static function armNode(Node $arm, array $body, Node $child): ?array + { + $branch = $arm->getAttribute('parent'); + if (! $branch instanceof Node || ! in_array($child, $body, true)) { + return null; + } + + return [$branch, spl_object_id($arm)]; + } +} 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..b2b1b98 --- /dev/null +++ b/src/Index/FanOutDetector.php @@ -0,0 +1,47 @@ + */ + private array $forwards = []; + + public function record(ForwardSite $site, BranchArmPath $branchArms): void + { + $this->forwards[] = new ForwardOccurrence($site, $branchArms); + } + + /** + * 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 (self::areDistinctOnOnePath($forward, $other)) { + return true; + } + } + } + + return false; + } + + private static function areDistinctOnOnePath(ForwardOccurrence $one, ForwardOccurrence $other): bool + { + 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 2858467..fa52f0e 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, BranchArmPath::of($node)); } private function markUse(bool $isStore): void diff --git a/src/Index/ForwardOccurrence.php b/src/Index/ForwardOccurrence.php new file mode 100644 index 0000000..93a7dee --- /dev/null +++ b/src/Index/ForwardOccurrence.php @@ -0,0 +1,18 @@ +forwards, false]; + return [ + $collector->fansOut() ? ParamFate::FanOut : ParamFate::PureForward, + $collector->forwards, + false, + ]; } private function typeToString(?Node $type): ?string diff --git a/tests/Chain/ChainBuilderTest.php b/tests/Chain/ChainBuilderTest.php index 02e05a4..3b17c44 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(); } }'; @@ -146,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 = '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 = '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/Index/BranchArmPathTest.php b/tests/Index/BranchArmPathTest.php new file mode 100644 index 0000000..a15106b --- /dev/null +++ b/tests/Index/BranchArmPathTest.php @@ -0,0 +1,51 @@ +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..f3fb460 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,182 @@ 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 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']; + 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 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']; + 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, + ); + } } 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); + } +} 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']); + } +} 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(); - } }