Feat: show what exists, and what nothing reaches - #131
Conversation
Benchmarkbase What the scan detectsDeterministic: both arms scan the same generated application on the same PHP version, and every repetition is checked to produce the same figures. A difference here is a change in what Brain detects, not machine noise — which is worth looking at rather than automatically worth fixing, since detecting more legitimately moves these.
No count changed on either corpus, including the per-node-type breakdown. TimingMedian wall clock on a shared CI runner. Noise is how far the base arm's own repetitions sat from its median, discounting the single worst on each side, with their full spread beside it; a delta inside the noise is not a measurable effect.
Phase split — Full scan — 1,188 filesA phase carries at least as much runner noise as the scan it is part of, so each one gets its own noise floor from the base arm's repetitions of that phase. Useful for locating a large move; a phase that is a few percent of the scan cannot be read at a few percent of accuracy.
Scans a synthetic Laravel application generated by |
SanderMuller
left a comment
SanderMuller
left a comment
There was a problem hiding this comment.
Reviewed at 50ba08b. composer test in a fresh worktree: 430 passed (1196 assertions), PHPStan No errors, Pint clean, 15/15 CI. Your numbers.
A tool that tells people which classes nothing reaches is one wrong entry away from someone deleting live code, so the review went almost entirely at the false-positive question.
The gate: purely additive, which is the best result this could have had
Null arm first, base against base on three applications — identical. Then the branch:
| full-graph nodes | edges | tabs | changed | |
|---|---|---|---|---|
| A | 3,568 → 3,568 | 8,686 → 8,686 | 537 → 538 | nothing |
| B | 2,889 → 2,889 | 5,218 → 5,218 | 370 → 371 | nothing |
| C | 2,573 → 2,573 | 5,286 → 5,286 | 424 → 425 | nothing |
Not one existing node, edge, subgraph or manifest entry moves on any of the three. The only difference is one new reachability--inventory tab. For a GraphSplitter diff of +238 lines that is the answer you want and rarely get.
The claim I went after: is the walk right, or does it just look right
I did not take the traversal on faith. For every class the tab reports as unreached, I checked whether the full graph holds a node carrying that FQCN, and then whether that node is reachable from any route node by a plain breadth-first walk over every edge — my own walk, not yours:
| unreached classes that ARE nodes in the full graph | of those, reachable from a route by plain BFS | |
|---|---|---|
| A | 84 | 0 |
| B | 95 | 0 |
| C | 49 | 0 |
Zero on all three. Every class the tab calls unreached really is outside the forward closure of the entry points, including the 77 models on A and the 18 policies on B that made me suspect a bug in the first place. The seeding — signature-built node ids for routes and commands, FQCN matching for queued listeners, and the entry point's own class counted as reached without a node — holds up on real applications. That suspicion is where most of this review went, and it dissolved; saying so is more useful to you than not mentioning it.
Four mutations, whole suite each, anchors asserted, restores verified:
| mutation | result |
|---|---|
| fold flag off for entry-point groups | 1 failed |
| fold flag off for unreached groups | 1 failed |
| entry point's own class no longer seeded as reached | 1 failed |
| a class's own file counts as a class-string reference | 1 failed |
Your description says removing the flag from one group kind turns a test red. Both kinds do, separately — I checked them one at a time rather than together, because a single mutation covering both would have proved neither.
Three things worth knowing, none blocking
1. The pass costs more than the code comment suggests, and it is on by default. ProjectAnalyzer says "A second traversal of the same files, not a second parse: PhpFileParser shares its results process-wide, so both passes read one AST per file." The sharing part is true and works — on B the cache serves 4,518 hits with the pass off and 7,478 with it on, so ClassStringIndex really does ride on what ClassInventory already parsed. What the sentence hides is that the inventory opens files the rest of the build never had a reason to open, which is exactly its job. Counting real parses with the switch on and off, three interleaved reps on B and deterministic counts throughout:
| real parses off → on | scan wall off → on | |||
|---|---|---|---|---|
| A | 733 → 1,195 | +63% | 1.9 s → 2.5 s | +32% |
| B | 1,098 → 2,110 | +92% | 1.35 s → 2.1 s | +56% |
| C | 746 → 4,416 | ×5.9 | 1.5 s → 4.5 s | ×3.0 |
C is a modular monolith: the build parses 746 files, the inventory parses 4,416. Tripling a scan is a real price for a feature that ships on. The config block is the honest one — it does say "one extra parse pass ... Turn it off if you do not want it" — so this is a numbers-in-the-description request rather than a design objection. I would put the C row in the PR body and soften the code comment, which currently reads as though the pass were free.
2. Seventeen classes on B are listed with no reason at all while Brain's own graph holds an edge pointing straight at them. All seventeen are policies. unfollowableReferences covers container bindings, facades, config strings, class strings and inheritance, but not "the graph already attaches this to a model by a dedicated edge". A policy wired to a model is precisely the "here is why it might still be referenced" case the design exists to serve, and it currently lands in the worst bucket. B's observers and listeners are in the list too and do carry a reason, so this is one gap rather than a category of them.
3. The scale on a modular monolith is worth stating plainly. On C: 4,303 classes declared, 449 reached, 3,854 unreached — 90% of everything the application declares, of which 1,629 carry no reason at all. The tab is 4,318 nodes. Your own figure (3,111 on a 60-module application) is the same shape. That is not wrong — the note inside the tab says exactly the right thing, and the config comment says it twice — but at 90% the list stops being a finding about the application and becomes a measurement of the tracer's reach. A line in the README or the tab note along the lines of "on a modular application expect most classes here; read the reasons column, not the count" would set the expectation before someone opens it and concludes their codebase is dead.
Checked and fine
The frontend registries. ALL_TYPES in App.tsx carries all four new types with a comment explaining why membership there is what makes them filterable and foldable at all — that is the fix, and it is in the right place. Sweeping every type list in frontend/src, the three that still omit them are ComplexityPanel (an unreached class is not a complexity subject), sequenceUtils (an inventory has no sequence) and exportUtils, whose TYPE_ORDER is a preferred order with everything else appended by [...new Set([...TYPE_ORDER, ...byType.keys()])] — it omits six existing types on main too, so it is neither exhaustive nor meant to be.
The fold, as it actually lands. Three section roots open — entry points, unreached, and outside-what-the-tracer-follows — and every per-kind group under them is folded: 21 of 24 on A, 23 of 26 on B, 22 of 25 on C. That matches what you describe rather than approximately matching it.
The bundle reproduces (npm ci && npm run build leaves git status empty) with a clean one-in, one-out swap of both the JS and the CSS chunk. No orphan left behind, which is worth saying since one of the other open branches did leave one.
The switch is a real switch: with reachability.enabled off the pass does not run at all, which is visible in the parse counts above rather than inferred from the code.
One housekeeping note: this is now the fourth open branch rewriting line 11 of index.blade.php and deleting index-B5C0iocj.js, alongside #128, #129 and #130. Whichever merges last resolves with npm run build and commits what it writes, never by picking a hash out of the conflict.
On the frontend-harness point — same answer I left on #130, so I will not repeat it here beyond the pointer: the components can be run in this repository with the project's own esbuild plus react-dom/server or jsdom, without adding a test runner to package.json. Worth a look for the fold behaviour whenever you next touch it.
Verdict
Approving. Purely additive on three applications, the walk survives an independent traversal, the mutations bite on both branches of the one claim you made about them, and the tab tells the reader what it is a statement about. The cost table belongs in the description before this ships on by default.
|
Following up on my approval rather than reversing it. The tab is right, the walk survived an independent traversal, and the switch works in both directions. This is about the default, and it is one word. The two instruments agree, and the real one is worseThe Benchmark comment on this branch, at the current head:
with I measured the same thing a different way before I had read that comment, by toggling
The two agree on the shape, and the parse doubling on B (+92%) is almost exactly what CI reports on the large corpus. The part worth your attention is C: a modular monolith where the build parses 746 files and the inventory parses 4,416, and the scan takes three times as long. The synthetic corpus has no application with 4,300 declared classes, so CI is measuring the mild case. Why I think the default is the question, and not the costThe cost is honest work — the inventory has to open every declared class, and that is the feature, not an inefficiency. I am not asking you to make it faster. What makes it different from the other branches is the ratio of cost to what a reader gets on the applications that pay the most:
So the application that pays three times the scan time is the one whose list is least readable. The other expensive features here ship on by default and cost a few percent; this one is a different order of magnitude, and it is the only one where the worst case and the least useful case are the same application. It also stacksEvery open branch measures against the same merge base
More than half of the whole train's cost is this one pass. That is an estimate assuming independence; the shared parse cache means they overlap, so the real number needs The ask
If you and the maintainer would rather keep it on, that is a fine answer and I will not argue it twice. In that case what I would ask for instead is the C row in the description: the +55%/+62% from CI is the mild case, and someone deciding whether to accept the default should see the ×3.0 next to it. The config comment is already the most honest thing in this PR — it says outright that this is not a dead-code report — and the cost deserves the same treatment. |
|
Done in A second application, measured the same wayToggling
About +2%, inside the run-to-run spread. The phase self-reports 1.03–1.19 s of a ~21 s scan, while finding 3,111 unreached classes and 843 entry points — the same order of output as your C, at a fraction of the cost. The difference is the parse cache, and your own numbers point at it: on C the build parses 746 files and the inventory 4,416, so almost everything the inventory opens is new. Here So the cost is not a property of the pass. It is the ratio between what the rest of the scan already parsed and what the inventory has to open — which is why one modular monolith sees ×3.0 and another sees nothing. Both config comment and commit message now carry both rows, because either one alone is misleading. Why the default still flipsYour cost argument does not survive my measurement, but the other one does, and it is the stronger of the two: on an application this size 79–90% of declared classes come back unreached, and a list that long is read once and then ignored. That is true here as well — 3,111 of roughly 3,950. A feature whose worst case is ×3.0, whose typical case may be nothing, and whose output is mostly noise at the sizes where it costs most, is one to turn on deliberately. Reversing a default is cheap; paying ×3.0 unasked is not. The test that reports what the pass finds now turns it on explicitly rather than leaning on the default — which is the small honest side effect of the change, and arguably how it should have been written anyway. Gate: One thing you may want to know about, unrelated to the askSetting |
SanderMuller
left a comment
SanderMuller
left a comment
There was a problem hiding this comment.
Re-reviewed at a1cc541, against 50ba08b. One commit: the default is now off.
The output gate is byte-identical on all three applications I measure with, null arm run first. That is the strongest statement available about a default flip — the pass does not run, so nothing it would have produced can leak into a scan that did not ask for it.
And it still works when asked. Switching the flag on against the same application: the tab returns, and the report reads 1,951 classes declared, 1,178 unreached — the same figures I measured before the default moved. Nothing was traded away to get the cheap default.
Suite green on this head: 430 passed (1196 assertions), PHPStan no errors, Pint clean.
On the benchmark comment at this head, which reads +1.5% (±0.7%) large and +2.7% (±1.2%) small, down from +55.2% and +62.0%: those residuals are not a cost. The gate says the output is byte-identical to the merge base, so nothing extra is computed — and the base arm itself moved from 980 ms to 896 ms between the two CI runs, which is several times the delta being reported. Worth saying out loud so nobody reads 1.5% as the price of the switch.
On the config comment
Turning the honest case into two rows with the mechanism next to them is the right way to write this down, and the sentence about what varies and what does not is the part that will save someone a decision. One caveat for a reader: the "+2% (within noise)" row is your application, not one I can reproduce — on the three I gate with, the cheapest case was +32% and the worst was ×3.0, because none of them had already parsed most of its own source. So the spread in that comment is real, and the good end of it depends on a property of the codebase rather than on the pass. The comment does say that, which is why this is a caveat rather than a correction.
The 79-90% figure matches what I measured independently: 90% of everything declared on one application, 1,629 of those with no reason attached at all.
Approving, and re-pointing the approval at this head. Thank you for taking the number seriously rather than arguing it.
eaa342f to
62d9e7e
Compare
|
Conflicts @webard |
1541f28 to
fda1048
Compare
Every tab Brain builds is grown forward from one entry point, so what the graph misses is invisible from inside it. Measured three times on a real application: it knew 45 of 211 event classes, 27 of 113 job classes, and of 102 route tabs carrying an event node, not one carried a listener. Each of those gaps took someone going looking. This adds the inverse view as its own tab, built the way the ERD tab is — independent of routes, assembled after the split, registered through the same manifest entry. Two halves. An inventory of entry points, grouped by kind: routes, console commands, scheduled entries, broadcast channels, queued listeners, Filament panels/resources/pages. Nothing is reachable except through one of these, so their inventory is the denominator for the other half — the classes declared under source_paths that no entry point's traced chain arrives at, grouped by kind and largest kind first, so "17 jobs nothing dispatches" is one glance. The hard part is the wording. A class the container resolves, a facade fronts, config names by string, or a reached class inherits from is running in production and still falls out of that subtraction, because there is no call to follow. So the tab never says "dead code": it says nothing reaches this from a traced entry point, and it shows the reader why that is a different sentence. Every reference Brain did find and could not follow travels with the class — container binding, facade, config/, inherited by a reached class, named as a class-string elsewhere — onto the node itself rather than a heading three levels up, because a reader clicks the class, not the group. Service providers and exceptions, kinds with no call edge at all, are filed in a section of their own; mixed in, they would be a hundred non-findings burying the real ones. A queued listener counts as a root and a synchronous one does not: the second runs inside its dispatcher's chain, the first is picked off the queue with no caller, and seeding from it is the only way the asynchronous half of an application counts as reached. Off with laravel-brain.reachability.enabled, on by default. It costs one extra traversal of source_paths and config/ — not an extra parse, since PhpFileParser shares its results process-wide.
Two defects, both found by rendering the tab against a real application rather than a fixture. The tab drew one node per class nothing reaches -- 3982 of them at 0% zoom, which no layout makes readable because a list of three thousand names was never a picture. Groups now ask the viewer to open folded, each saying how many it holds. That alone did nothing, because the four node types this tab introduces were registered in 2 of the 8 frontend type registries. ALL_TYPES was one of the six that missed them, and edgeVisible reads it -- so their edges were never visible, the fold found no children, and the type filters could not toggle them either. Registered in ALL_TYPES, the filter panel, the legend and the sidebar palette. Fitting the view also measured every node including the folded-away ones, framing a picture nobody is looking at. Measured on a 60-module application: 3982 nodes at 0% -> 28 at 19%.
The pass opens every declared class, and what that costs depends entirely on how much of the codebase the rest of the scan already parsed. Two modular monoliths, measured the same way: A build parses 746 files, inventory 4,416 scan x3.0 B 7,546 source files, nearly all already cached +2% (within noise) So the worst case is real and the typical case may be nothing. What does not vary is the shape of the answer: 79-90% of declared classes come back unreached on an application this size, and a list that long is read once and then ignored. Reversing a default is cheap; paying x3.0 unasked is not. The test that reports what the pass finds now turns it on explicitly rather than relying on the default.
fda1048 to
3d27b5f
Compare
3 participants
The graph could show what a route reaches. It could not answer the question underneath that: what exists that nothing reaches at all — the job nobody dispatches, the listener wired to an event never fired, the service left behind by a refactor.
What it reads
Every entry point the framework can start work from — routes, console commands, queued listeners, scheduled entries, broadcast channels, Filament pages — and every class declared in the scanned source. What the tracer reaches from an entry point is reachable; what is left is reported with why it might still be referenced: a container binding, a config value, a class string, inheritance. A class reached only that way is a different finding from one nothing mentions at all, and saying which is the difference between a list worth reading and a list worth ignoring.
On a 60-module application: 843 entry points, 3111 classes nothing reaches.
Three defects found by rendering it, not by testing it
The analysis was right from the start. Everything below is about the screen.
One node per unreached class — 3982 of them, drawn at 0% zoom. No layout fixes that; a list of three thousand names was never a picture. Groups now ask the viewer to open folded, each showing how many it holds, and the reader opens the one they came for.
That alone changed nothing, and the reason is worth recording. This tab introduces four node types, and they were registered in 2 of the 8 frontend type registries.
ALL_TYPESinApp.tsxwas one of the six that missed them — andedgeVisiblereads it, so those nodes had no visible edges, the fold walked out to no children, and the type filters could not toggle them either. Registered now inALL_TYPES, the filter panel labels and order, the legend, and the sidebar palette.Fitting the view measured every node, including the folded-away ones, framing a picture nobody is looking at. It now measures what is drawn — which fixes the same thing for every collapsed graph, not just this one.
Measured, at each step
Worth being plain about, since the question was whether to wait for #118: its layer wrapping does not fix this on its own — 3982 nodes stay unreadable however they are packed. The two are complementary rather than alternatives. This PR stands on its own at 19%; when #118 merges, the same screen becomes comfortable at 65%, because 28 groups in one row is exactly the case wrapping was built for.
Verification
pintclean,phpstanno errors, 430 tests passing.The fold is pinned by a test asserting both group kinds carry the flag and neither the members nor the roots do — folding a member would hide the thing it names, folding a root would close the screen. Removing the flag from one group kind turns it red.
The frontend fixes are verified by measurement and screenshot rather than by test: there is no frontend test harness in this repository, and I did not want to introduce one inside this PR without asking.