fix(classifiers): detect transitively-unreachable dead code, not just fanIn===0 - #2258
Conversation
… fanIn===0 roles --role dead treated any inbound `calls` edge as sufficient evidence of liveness, without checking whether the caller itself is reachable from a confirmed-live root. A function called only by another unreachable function kept a non-dead role forever (#2032). Add a worklist/BFS reachability pass (both engines) that runs after the existing per-node classification and can only downgrade a fan-in-based verdict to dead, never upgrade one — entry/test-only/hasActiveFileSiblings rescues/interface-type-members are untouched. Roots are exported/ framework-dispatched/Commander-dispatch function/method nodes, plus every `calls`-edge source that isn't itself a function/method (module-level constants, file-scope bare assignments) and `method`-kind interface- dispatch implementations, which can never be call-edge targets by construction. Scoped to the full-classification path only: the incremental path cannot safely compute whole-graph reachability from a changed-files-scoped window without reintroducing the full-scan cost its caching was built to avoid (tracked in #2255, along with two other follow-ups filed from self-build validation: #2256, #2257). docs check acknowledged: internal classifier fix, no README/CLAUDE.md/ROADMAP.md changes needed. Impact: 6 functions changed, 5 affected
Greptile SummaryThe PR changes full role classification to downgrade functions and methods that have direct fan-in but are unreachable from confirmed-live roots.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains in the current fixes for the previously reported threads. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Classify nodes by fan shape] --> B[Collect confirmed-live roots]
B --> C[Traverse calls edges]
C --> D{Fan-shape function or method reachable?}
D -->|Yes| E[Keep existing role]
D -->|No| F[Downgrade to dead sub-role]
Reviews (3): Last reviewed commit: "fix(classifiers): scope reexport-derived..." | Re-trigger Greptile |
| if (node.kind !== 'function' && node.kind !== 'method') return false; | ||
| if (node.isExported) return true; | ||
| return !!( | ||
| node.file && |
There was a problem hiding this comment.
Cross-file references become roots
When an unexported function is called across files only by an unreachable function, the existing cross-file-reference heuristic sets isExported, and isLiveRoot consequently seeds the callee as a BFS root. The callee and its descendants retain live fan-shape roles instead of being classified as dead, so cross-file dead call chains remain undetected.
Knowledge Base Used:
There was a problem hiding this comment.
Valid, fixed in 7360d4f. Added a narrower isPublicSurface field (explicit export keyword + confirmed production-reachable reexport chains only), and switched isLiveRoot to check that instead of the broad isExported (which also fires from an unverified cross-file caller). Mirrored in the Rust classifier. Added regression tests in both engines confirming a symbol reachable only through an unreachable cross-file caller now correctly stays dead.
| return ( | ||
| node.kind === 'method' && | ||
| node.fanIn === 0 && | ||
| node.fanOut > 0 && | ||
| !!node.hasActiveFileSiblings && | ||
| !isTypeDeclarationMember(node, typeDefNamesByFile) | ||
| ); |
There was a problem hiding this comment.
Ordinary methods become live roots
When an unused class method calls a helper and shares its file with another called symbol, it satisfies this predicate because class methods use kind: 'method', receive hasActiveFileSiblings, and are not type-declaration members. The method is therefore treated as an interface-dispatch root, causing helpers reachable only through that dead method to retain live roles.
Knowledge Base Used:
There was a problem hiding this comment.
Valid, fixed in 7360d4f. isInterfaceDispatchMethodRoot now additionally requires the candidate method's bare name to match an actual interface/type declaration member somewhere in the codebase (e.g. enterNode against interface Visitor { enterNode?(...): ...; } in src/types.ts), not just the fanIn=0/fanOut>0/hasActiveFileSiblings shape alone. An unrelated dead class method now needs a coincidental name collision with some interface's member to slip through, instead of qualifying by default. Mirrored in Rust. Added regression tests in both engines for both the still-correct Visitor-pattern case and the now-fixed ordinary-dead-method case. (Re-validating against this repo's own self-build surfaced one further narrow gap this uncovers — genuine EventEmitter-style method callbacks with no matching interface declaration — filed as #2259 rather than trying to solve it inline.)
Codegraph Impact Analysis9 functions changed → 9 callers affected across 4 files
|
…s grant roots
Two root-identification gaps found by Greptile review:
1. isLiveRoot checked the broad isExported flag, which also fires when
SOME caller in a different file calls a symbol, regardless of whether
that caller is itself reachable. A cross-file dead call chain could
therefore evade the whole reachability check. Added a narrower
isPublicSurface (explicit `export` keyword + confirmed
production-reachable reexport chains only) and switched isLiveRoot to
use it instead.
2. isInterfaceDispatchMethodRoot promoted any method with fanIn===0,
fanOut>0, and an active file sibling — indistinguishable from an
ordinary dead class method that happens to call a helper. Added a
requirement that the method's bare name match an actual declared
interface/type member somewhere in the codebase (e.g. `enterNode`
against `interface Visitor { enterNode?(...): ...; }`), so an
unrelated dead method needs a coincidental name collision to slip
through instead of qualifying by default.
Both fixes mirrored in crates/codegraph-core/src/graph/classifiers/roles.rs.
Re-validated against this repo's own self-build; filed #2259 for a
narrower remaining gap (EventEmitter-style method callbacks with no
matching interface declaration).
docs check acknowledged: internal classifier fix, no README/CLAUDE.md/ROADMAP.md changes needed.
Impact: 8 functions changed, 5 affected
| // `isPublicSurface` doc comment for why that component must not grant | ||
| // automatic root status). | ||
| const publicSurfaceIds = new Set<number>(); | ||
| for (const r of reexportExported) publicSurfaceIds.add(r.id); |
There was a problem hiding this comment.
Private reexport targets become roots
When a production-reachable barrel names one export from a module that also contains private functions, reexportExported includes every eligible symbol in that module and this loop adds them all to publicSurfaceIds. Those private functions become unconditional BFS roots, causing unreachable call chains beneath them to retain live roles instead of being classified as dead.
Knowledge Base Used:
There was a problem hiding this comment.
Valid, fixed in d08ffe7. publicSurfaceIds no longer reuses the whole-file reexportExported query. Named reexports already get their own symbol-level 'reexports' edge (target = the specific symbol, not the file) — now used directly; whole-file marking is scoped to a genuine export * from (kind 'reexports-wildcard') only. The pre-existing exportedIds/entry-classification query (#837) is untouched. Mirrored in Rust. Added regression tests in both engines (a named-reexport fixture where a private sibling reachable only through an unreachable caller correctly stays dead despite sharing a file with the re-exported symbol) — validating against this repo's own self-build actually caught a real live instance of exactly this bug (src/extractors/groovy.ts's AST-dispatch-table handlers), now fixed; filed #2260 for the underlying computed-dispatch-table extraction gap that instance also exposed.
…named symbol
publicSurfaceIds reused the pre-existing whole-file reexport query, which
treats ANY 'reexports' edge as "every symbol in the target file is
exported" — correct for `export * from './b'` but wrong for `export {
specificThing } from './b'`, which only re-exports specificThing. A
private, unreachable sibling in that same file (Greptile review: e.g.
groovy.ts's AST-node-type dispatch handlers, previously masked by this
exact bug) was becoming an automatic BFS root merely by sharing a file
with the one actually re-exported symbol.
Named reexports already get their own symbol-level 'reexports' edge
(target = the specific symbol, not the file); use that directly for
publicSurfaceIds, and only fall back to whole-file marking for a genuine
`export * from` (kind 'reexports-wildcard'). The pre-existing whole-file
exportedIds query (used for classifyNodeRole's own `entry` classification,
#837) is left untouched — this only narrows the new #2032 root signal.
Mirrored in crates/codegraph-core/src/graph/classifiers/roles.rs.
Re-validated against this repo's own self-build; filed #2260 for the
underlying gap this uncovered (computed/bracket-access dispatch-table
lookups still lack a real calls edge, unlike dot-property value-refs).
docs check acknowledged: internal classifier fix, no README/CLAUDE.md/ROADMAP.md changes needed.
Impact: 2 functions changed, 1 affected
Summary
roles --role deadtreated any inboundcallsedge as sufficient evidence of liveness, without checking whether the caller itself is reachable from a confirmed-live root. A function called only by another unreachable function kept a non-dead role forever.applyReachabilityDowngradeinsrc/graph/classifiers/roles.ts, mirrored asapply_reachability_downgradeincrates/codegraph-core/src/graph/classifiers/roles.rs) that runs after the existing per-node classification. It is strictly downgrading — never upgrades a verdict, never touchesentry/test-only/hasActiveFileSiblingsrescues/interface-type-members — it only reconsidersfunction/methodnodes whosecore/utility/adapter/leafverdict came from direct fan-in (classifyByFanShape).function/methodnodes on the genuinely public surface (explicitexportkeyword, or the SPECIFIC symbol named in a confirmed production-reachable reexport, or every symbol in a file reached via a genuineexport * from— deliberately NOT the broader "some cross-file caller exists" signal, nor "shares a file with a named-reexported sibling", see Greptile fixes below) / framework-dispatched (route:/event:/command:) / Commander-dispatch, plus everycalls-edge SOURCE that isn't itself afunction/method(module-top-levelconstantdeclarations, bare file-scope assignments like Lua'srequire = tracedFn) andmethod-kind interface-dispatch implementations whose name matches a declared interface/type member (Visitor-pattern methods invoked only via generic property access) — none of these can ever be the target of acallsedge by construction, so they must count as unconditionally live.function-kind logical-or-fallback rescues to roots — doing so would silently re-rescue genuinely-dead intermediate functions, defeating the fix for the common case (see code comments for the reasoning and a regression test guarding it).Scope decision
Implemented for the full-classification path only (
classifyNodeRolesFull/ Rustdo_classify_full). The incremental path cannot safely compute whole-graph reachability from a changed-files-scoped window — a node's only live path in can run through files entirely outside that window — without either reintroducing a full-graph scan on every incremental build (undermining the perf work that path was built around) or a real incremental-reachability-maintenance design (handles edge additions fine, edge removals need decremental reachability). Filed as a tracked follow-up: #2255.Root-identification bugs found and fixed
Running
codegraph roles --role dead -Ton this repo's ownsrc/before/after (each fix re-validated the same way) surfaced real root-identification bugs, all fixed before landing:NativeDatabase.countNodesinsrc/types.ts) get'leaf'unconditionally fromisTypeDeclarationMember, independent offanIn— indistinguishable fromclassifyByFanShape's'leaf'by role string alone. Fixed by explicitly excluding type-declaration members from the downgrade pass.method-kind interface-dispatch implementations (e.g.enterNode/exitNodeinsrc/ast-analysis/visitors/*.ts, dispatched viaif (v.enterNode) v.enterNode(...)) can never be the target of acallsedge by construction, so they could never be BFS-reachable — wrongly starving their own callees. Fixed by addingisInterfaceDispatchMethodRoot.isLiveRootused the broadisExportedflag, which also fires when some caller in a different file calls a symbol regardless of whether that caller is itself reachable — letting a cross-file dead call chain evade the whole check. Fixed with a narrowerisPublicSurface(explicit export / confirmed reexport chain only).isInterfaceDispatchMethodRootpromoted any method withfanIn===0,fanOut>0, and an active file sibling — indistinguishable from an ordinary dead class method that happens to call a helper. Fixed by requiring the method's bare name to match an actual declared interface/type member somewhere in the codebase.publicSurfaceIdsreused the pre-existing whole-file reexport query, which treats ANYreexportsedge (named or wildcard) as "every symbol in the target file is exported". Fixed by using the symbol-levelreexportsedge a named reexport already gets, falling back to whole-file marking only for a genuineexport * from(reexports-wildcard). Caught a real live instance in this repo's own self-build (src/extractors/groovy.ts's AST-node-dispatch handlers).Re-validating after fixes 3-5 surfaced two further narrow, out-of-scope gaps — filed as #2259 and #2260 rather than solved inline.
Before/after on this repo's own self-build (
src/,-T)+194 delta. Diffed the actual symbol lists (not just totals) at every step of the fix, not just the final one: the large majority are pre-existing hand-authored parser/resolution test fixtures under
tests/benchmarks/**/tests/fixtures/**that-T's filename-only heuristic doesn't exclude (tracked as #2256) — these grew once the cross-file-root fix (#3 above) closed a loophole those fixtures relied on especially heavily (they're full of cross-file call-resolution test cases with no genuine exported entry point). The remaining realsrc/findings are the already-tracked #2257 (2 cases), #2259 (1 case, 4 downstream nodes), and #2260 (1 case, 4 downstream nodes — caught and fixed by review round 3, then re-surfaced as a distinct root cause once the reexport-scoping bug was corrected).Testing
npm run lint— cleannpm test— 271 files / 4395 tests pass (added TS unit + integration tests for the reachability pass, including regression tests for all three Greptile findings, + updated pre-existing fixtures whose only "root" was one of the now-narrower signals, to keep testing what they originally intended)cargo test -p codegraph-core --lib— 741 tests pass (mirrored Rust unit tests using an in-memoryNativeDatabase)codegraph diff-impact --staged -T: small, expected blast radius scoped to the classifier moduleFollow-up issues filed
-Tdoesn't excludetests/benchmarks/**/fixtures/**/tests/fixtures/**fixture directoriescallsedge (mirrors Dispatch-table function references (resolve: fn) inconsistently flagged dead-unresolved depending on unrelated fanOut #1771/roles --role dead: object-literal property-value references count as liveness without checking if the property is ever invoked (false negative) #1895 for a different value-ref shape)EventEmitter-style callback registrations aren't recognized as reachability roots (surfaced while fixing Greptile's review findings)table[key](...)) still lack a realcallsedge, unlike dot-property value-refs (surfaced while fixing the reexport-scoping bug)Closes #2032