fix: recognize same-file constant consumption in dead code detector#859
fix: recognize same-file constant consumption in dead code detector#859carlos-alm merged 5 commits intomainfrom
Conversation
Constants consumed via identifier reference (not function calls) had no inbound call edges, causing fanIn=0 and incorrect dead-leaf classification. Add hasActiveFileSiblings flag so constants in files with active callables are classified as leaf instead of dead-leaf. Closes #841
|
Claude finished @carlos-alm's task —— View job Reviewing PR: fix: recognize same-file constant consumption in dead code detectorI've completed a comprehensive 5-phase review of this dead code detection fix: Todo List:
Root Cause AnalysisProblem Being Solved: Constants consumed via identifier reference (not function calls) had no inbound Why This Approach: The fix correctly identifies when a constant shares a file with callable symbols that have Risk Assessment: Low risk. The change is surgical and well-contained within the classification logic. Backlog Compliance
Code Quality AssessmentStrengths:
Architecture:
Technical Verification
Final Recommendation
This PR delivers exactly what good bug fixes should: identifies a real false positive, implements a principled solution, applies it consistently across all code paths, and validates it with thorough tests. |
Codegraph Impact Analysis4 functions changed → 3 callers affected across 1 files
|
Greptile SummaryThis PR fixes incorrect Key changes:
Both previous review concerns (narrow Confidence Score: 5/5Safe to merge — logic is correct, both full and incremental paths are consistently updated, and all prior review concerns have been addressed All previous P1 findings (narrow fan_out-only criterion, hasActiveFileSiblings leaking to non-constant nodes) were resolved in f91fde0. No new P1 issues found. The implementation is O(n) with no extra DB queries, and the classifier remains a pure function. Three targeted tests validate the fix including the pure-sink edge case. No files require special attention — all three changed files are in good shape Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[classifyNodeRolesFull / Incremental\nfetch rows from DB] --> B[Build activeFiles set\nfor r in rows\nif fan_in>0 OR fan_out>0\nAND kind != constant]
B --> C[Map rows to classifierInput\nhasActiveFileSiblings = kind=='constant'\n? activeFiles.has(file) : undefined]
C --> D[classifyRoles node loop]
D --> E{isFrameworkEntry?}
E -- yes --> F[role = entry]
E -- no --> G{fanIn==0 AND not exported?}
G -- no --> H[core / utility / adapter / leaf\nbased on median thresholds]
G -- yes --> I{kind=='constant' AND\nhasActiveFileSiblings?}
I -- yes --> J[role = leaf\n✓ constant used locally\nvia identifier reference]
I -- no --> K{testOnlyFanIn > 0?}
K -- yes --> L[role = test-only]
K -- no --> M[classifyDeadSubRole\ndead-leaf / dead-entry /\ndead-ffi / dead-unresolved]
Reviews (3): Last reviewed commit: "fix: update stale JSDoc on hasActiveFile..." | Re-trigger Greptile |
| // Constants in these files are likely consumed locally via identifier reference. | ||
| const activeFiles = new Set<string>(); | ||
| for (const r of rows) { | ||
| if (r.fan_out > 0 && r.kind !== 'constant') { | ||
| activeFiles.add(r.file); | ||
| } | ||
| } |
There was a problem hiding this comment.
fan_out > 0 criterion misses pure-sink siblings
The activeFiles heuristic only rescues constants whose file contains at least one callable with outbound calls. It does not rescue constants in files where all sibling functions are pure sinks (i.e. fan_in > 0 but fan_out === 0).
Concrete example:
// validators.ts
const MAX_LENGTH = 255; // fanIn = 0, fanOut = 0 — stays dead-leaf
function validate(s: string): boolean { // fanIn = 10, fanOut = 0 (no outbound calls)
return s.length <= MAX_LENGTH; // MAX_LENGTH is used, but via identifier ref
}Here validate has fan_out === 0, so validators.ts is never added to activeFiles, and MAX_LENGTH remains dead-leaf even though it is consumed.
Widening the criterion to (r.fan_in > 0 || r.fan_out > 0) && r.kind !== 'constant' would cover this common pattern without meaningfully increasing false-negative rate — any callable that is actually connected to the graph (either called or calling) is a good proxy for an "active" file.
| // Constants in these files are likely consumed locally via identifier reference. | |
| const activeFiles = new Set<string>(); | |
| for (const r of rows) { | |
| if (r.fan_out > 0 && r.kind !== 'constant') { | |
| activeFiles.add(r.file); | |
| } | |
| } | |
| const activeFiles = new Set<string>(); | |
| for (const r of rows) { | |
| if ((r.fan_in > 0 || r.fan_out > 0) && r.kind !== 'constant') { | |
| activeFiles.add(r.file); | |
| } | |
| } |
The same change applies to the symmetric block in classifyNodeRolesIncremental (lines 733–737).
There was a problem hiding this comment.
Good catch. Widened the criterion to (r.fan_in > 0 || r.fan_out > 0) && r.kind !== 'constant' in both the full and incremental paths, so pure-sink siblings (like validate with fan_in=10, fan_out=0) now correctly mark their file as active. Added a dedicated test for this pattern. Fixed in f91fde0.
| @@ -592,6 +601,7 @@ function classifyNodeRolesFull(db: BetterSqlite3Database, emptySummary: RoleSumm | |||
| fanOut: r.fan_out, | |||
| isExported: exportedIds.has(r.id), | |||
| productionFanIn: prodFanInMap.get(r.id) || 0, | |||
| hasActiveFileSiblings: activeFiles.has(r.file), | |||
| })); | |||
There was a problem hiding this comment.
hasActiveFileSiblings set for every node, consumed only for constants
hasActiveFileSiblings is propagated to every entry in classifierInput — including functions, classes, methods, etc. — but classifyRoles only reads it when node.kind === 'constant'. For all other kinds the field is silently ignored.
This is harmless today, but it makes the intent of the flag harder to reason about and could mislead a future reader into thinking the flag affects non-constant classification.
Consider scoping the assignment so the flag is only passed when actually meaningful:
| const classifierInput = rows.map((r) => ({ | |
| id: String(r.id), | |
| name: r.name, | |
| kind: r.kind, | |
| file: r.file, | |
| fanIn: r.fan_in, | |
| fanOut: r.fan_out, | |
| isExported: exportedIds.has(r.id), | |
| productionFanIn: prodFanInMap.get(r.id) || 0, | |
| hasActiveFileSiblings: r.kind === 'constant' ? activeFiles.has(r.file) : undefined, | |
| })); |
The same applies to the incremental path (lines 740–750).
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Agreed — scoped the assignment with a ternary: r.kind === 'constant' ? activeFiles.has(r.file) : undefined in both the full and incremental paths. Non-constant nodes no longer carry the flag. Fixed in f91fde0.
…e flag to constants Widen activeFiles criterion from fan_out > 0 to (fan_in > 0 || fan_out > 0) so constants in files with pure-sink functions (called but making no outbound calls) are correctly rescued from dead-leaf. Scope hasActiveFileSiblings assignment to constant nodes only for clarity.
|
@greptile-apps Good catch on the stale JSDoc for All Greptile findings are now addressed: |
Summary
callsedges, causingfanIn=0and incorrectdead-leafclassificationhasActiveFileSiblingsflag toRoleClassificationNode— when a constant shares a file with callable symbols that havefanOut > 0, it's classified asleafinstead ofdead-leafstructure.tsBefore / After
Test plan
classifyRolestests pass (constant without siblings stilldead-leaf)leafleaf(notdead-entry)src/graph/classifiers/risk.tsCloses #841